[RFC 0/6] Driver core: Encourage use of const attributes

[Phil Carmody]

I know now's probably a bad time to be mentioning const issues
but I recently a reviewed a patch where 'const' was being removed
from a device_attribute structure, and I enquired why. The reason
was simply so that device_create_file() could be called, it taking
a non-const attribute pointer. Looking inside, all that function
did was pass a pointer to sysfs_create_file, which itself took a
const pointer. Non-constness was not required at all.

So here I offer a small patchset which I hope will permit and
encourage device and other attributes to be made const, and put
in read-only sections.

1-3 address the three attribute types which seemed to be trivially
const-able, and are the important part of the set.

4 adds a new macro to encourage the use of Const ATTRibutes,
and may need a better name. (I wanted to avoid RO, for example.)

5 and 6 are merely two quick examples of how easy it is to adopt
the new const convention. In reality, these structures have been
constant and treated as constant by the driver core all along, it's
just that one word was missing from a few important places.

I would hope to submit a patchset with 1-3 and a possibly modified
4. The migrations themselves will belong in different trees.

These have been compile/sparse tested against arm and x86_64 targets,
and through my usual suite of tests on the arm platform I use.

Regards,
Phil

Phil Carmody (6):
Driver core: device_attribute parameters can often be const*
Driver core: bin_attribute parameters can often be const*
Driver core: driver_attribute parameters can often be const*
Driver core: Easy macros to encourage const attributes
PM: Example of how easy it is to mark attributes const
gpio: Example of how easy it is to mark attributes const

drivers/base/core.c | 12 ++++++++----
drivers/base/driver.c | 4 ++--
drivers/gpio/gpiolib.c | 12 ++++++------
drivers/regulator/core.c | 38 +++++++++++++++++++-------------------
fs/sysfs/bin.c | 6 ++++--
include/linux/device.h | 20 ++++++++++++++------
include/linux/sysfs.h | 9 +++++----
7 files changed, 58 insertions(+), 43 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Greg KH]

On Thu, Dec 17, 2009 at 08:12:10PM +0200, Phil Carmody wrote:
> I know now's probably a bad time to be mentioning const issues
> but I recently a reviewed a patch where 'const' was being removed
> from a device_attribute structure, and I enquired why. The reason
> was simply so that device_create_file() could be called, it taking
> a non-const attribute pointer. Looking inside, all that function
> did was pass a pointer to sysfs_create_file, which itself took a
> const pointer. Non-constness was not required at all.
>
> So here I offer a small patchset which I hope will permit and
> encourage device and other attributes to be made const, and put
> in read-only sections.
>
> 1-3 address the three attribute types which seemed to be trivially
> const-able, and are the important part of the set.

I like these, very nice.

> 4 adds a new macro to encourage the use of Const ATTRibutes,
> and may need a better name. (I wanted to avoid RO, for example.)

Hm, is this really needed? How badly do things break if you change the
current attribute macros to use 'const'? What subsystems are not using
const?

> 5 and 6 are merely two quick examples of how easy it is to adopt
> the new const convention. In reality, these structures have been
> constant and treated as constant by the driver core all along, it's
> just that one word was missing from a few important places.
>
> I would hope to submit a patchset with 1-3 and a possibly modified
> 4. The migrations themselves will belong in different trees.

I'll be glad to take 1-3 now, and queue it up for .34.

thanks,

greg k-h

[Stephen Rothwell]

Hi Greg,

On Thu, 17 Dec 2009 15:16:21 -0800 Greg KH <gre...@suse.de> wrote:
>
> I'll be glad to take 1-3 now, and queue it up for .34.

Any reason not to ask Linus to take them for .33 - that way the
conversions of callers can be done adhoc (and in more than just your
tree) for .34. These API changes should be perfectly safe (and the
compiler should tell us if they are not).

--
Cheers,
Stephen Rothwell s...@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[Greg KH]

On Fri, Dec 18, 2009 at 11:42:23AM +1100, Stephen Rothwell wrote:
> Hi Greg,
>
> On Thu, 17 Dec 2009 15:16:21 -0800 Greg KH <gre...@suse.de> wrote:
> >
> > I'll be glad to take 1-3 now, and queue it up for .34.
>
> Any reason not to ask Linus to take them for .33 - that way the
> conversions of callers can be done adhoc (and in more than just your
> tree) for .34. These API changes should be perfectly safe (and the
> compiler should tell us if they are not).

Good idea, I'll queue them up to get in soon.

[Phil Carmody]

On 18/12/09 00:16 +0100, ext Greg KH wrote:
> On Thu, Dec 17, 2009 at 08:12:10PM +0200, Phil Carmody wrote:

[...]> > So here I offer a small patchset which I hope will permit and

> > encourage device and other attributes to be made const, and put
> > in read-only sections.
> >
> > 1-3 address the three attribute types which seemed to be trivially
> > const-able, and are the important part of the set.
>
> I like these, very nice.

Thanks, I can possibly sniff a little bit deeper, and see if there are
any other obvious throwing-away-of-consts nearby.

> > 4 adds a new macro to encourage the use of Const ATTRibutes,
> > and may need a better name. (I wanted to avoid RO, for example.)
>
> Hm, is this really needed? How badly do things break if you change the
> current attribute macros to use 'const'? What subsystems are not using
> const?

That was the first thing that went through my mind, but I didn't want
to be too brave. He who dares wins though, and I can certainly give
that a try. allmodconfig is my friend.

> > 5 and 6 are merely two quick examples of how easy it is to adopt
> > the new const convention. In reality, these structures have been
> > constant and treated as constant by the driver core all along, it's
> > just that one word was missing from a few important places.
> >
> > I would hope to submit a patchset with 1-3 and a possibly modified
> > 4. The migrations themselves will belong in different trees.
>
> I'll be glad to take 1-3 now, and queue it up for .34.

They cleave cleanly at that point, so I'll resend with a [PATCH] prefix.

Many thanks, and seasons greetings,

Phil

[Phil Carmody]

Many struct bin_attribute descriptors are purely read-only
structures, and there's no need to change them. Therefore
make the promise not to, which will let those descriptors
be put in a ro section.

Signed-off-by: Phil Carmody <ext-phil....@nokia.com>
---
drivers/base/core.c | 6 ++++--
fs/sysfs/bin.c | 6 ++++--
include/linux/device.h | 6 +++---
include/linux/sysfs.h | 9 +++++----
4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index c1e3cad..d0b2116 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -465,7 +465,8 @@ void device_remove_file(struct device *dev,
* @dev: device.
* @attr: device binary attribute descriptor.
*/
-int device_create_bin_file(struct device *dev, struct bin_attribute *attr)
+int device_create_bin_file(struct device *dev,
+ const struct bin_attribute *attr)
{
int error = -EINVAL;
if (dev)
@@ -479,7 +480,8 @@ EXPORT_SYMBOL_GPL(device_create_bin_file);
* @dev: device.
* @attr: device binary attribute descriptor.
*/
-void device_remove_bin_file(struct device *dev, struct bin_attribute *attr)
+void device_remove_bin_file(struct device *dev,
+ const struct bin_attribute *attr)
{
if (dev)
sysfs_remove_bin_file(&dev->kobj, attr);
diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index 60c702b..a0a500a 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -483,7 +483,8 @@ void unmap_bin_file(struct sysfs_dirent *attr_sd)
* @attr: attribute descriptor.
*/

-int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr)
+int sysfs_create_bin_file(struct kobject *kobj,
+ const struct bin_attribute *attr)
{
BUG_ON(!kobj || !kobj->sd || !attr);

@@ -497,7 +498,8 @@ int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr)
* @attr: attribute descriptor.
*/

-void sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr)
+void sysfs_remove_bin_file(struct kobject *kobj,
+ const struct bin_attribute *attr)
{
sysfs_hash_and_remove(kobj->sd, attr->attr.name);
}
diff --git a/include/linux/device.h b/include/linux/device.h
index 9ecd9b0..0823a29 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -319,13 +319,13 @@ struct device_attribute {
struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)

extern int __must_check device_create_file(struct device *device,
- const struct device_attribute *entry);
+ const struct device_attribute *entry);
extern void device_remove_file(struct device *dev,
const struct device_attribute *attr);
extern int __must_check device_create_bin_file(struct device *dev,
- struct bin_attribute *attr);
+ const struct bin_attribute *attr);
extern void device_remove_bin_file(struct device *dev,
- struct bin_attribute *attr);
+ const struct bin_attribute *attr);
extern int device_schedule_callback_owner(struct device *dev,
void (*func)(struct device *dev), struct module *owner);

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9d68fed..cfa8308 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -99,8 +99,9 @@ int __must_check sysfs_chmod_file(struct kobject *kobj, struct attribute *attr,
void sysfs_remove_file(struct kobject *kobj, const struct attribute *attr);

int __must_check sysfs_create_bin_file(struct kobject *kobj,
- struct bin_attribute *attr);
-void sysfs_remove_bin_file(struct kobject *kobj, struct bin_attribute *attr);
+ const struct bin_attribute *attr);
+void sysfs_remove_bin_file(struct kobject *kobj,
+ const struct bin_attribute *attr);

int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
const char *name);
@@ -175,13 +176,13 @@ static inline void sysfs_remove_file(struct kobject *kobj,
}

static inline int sysfs_create_bin_file(struct kobject *kobj,
- struct bin_attribute *attr)
+ const struct bin_attribute *attr)
{
return 0;
}

static inline void sysfs_remove_bin_file(struct kobject *kobj,
- struct bin_attribute *attr)
+ const struct bin_attribute *attr)
{
}

--
1.6.0.4

[Phil Carmody]

Most device_attributes are const, and are begging to be
put in a ro section. However, the create and remove
file interfaces were failing to propagate the const promise
which the only functions they call offer.

Signed-off-by: Phil Carmody <ext-phil....@nokia.com>
---

Documentation/filesystems/sysfs.txt | 8 ++++----
drivers/base/core.c | 6 ++++--
include/linux/device.h | 4 ++--
3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
index b245d52..a4ac2b9 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -91,8 +91,8 @@ struct device_attribute {
const char *buf, size_t count);
};

-int device_create_file(struct device *, struct device_attribute *);
-void device_remove_file(struct device *, struct device_attribute *);
+int device_create_file(struct device *, const struct device_attribute *);
+void device_remove_file(struct device *, const struct device_attribute *);

It also defines this helper for defining device attributes:

@@ -316,8 +316,8 @@ DEVICE_ATTR(_name, _mode, _show, _store);

Creation/Removal:

-int device_create_file(struct device *device, struct device_attribute * attr);
-void device_remove_file(struct device * dev, struct device_attribute * attr);
+int device_create_file(struct device *dev, const struct device_attribute * attr);
+void device_remove_file(struct device *dev, const struct device_attribute * attr);


- bus drivers (include/linux/device.h)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6bee6af..c1e3cad 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -439,7 +439,8 @@ struct kset *devices_kset;
* @dev: device.
* @attr: device attribute descriptor.
*/
-int device_create_file(struct device *dev, struct device_attribute *attr)
+int device_create_file(struct device *dev,
+ const struct device_attribute *attr)
{
int error = 0;
if (dev)
@@ -452,7 +453,8 @@ int device_create_file(struct device *dev, struct device_attribute *attr)
* @dev: device.
* @attr: device attribute descriptor.
*/
-void device_remove_file(struct device *dev, struct device_attribute *attr)
+void device_remove_file(struct device *dev,
+ const struct device_attribute *attr)
{
if (dev)
sysfs_remove_file(&dev->kobj, &attr->attr);
diff --git a/include/linux/device.h b/include/linux/device.h
index 2ea3e49..9ecd9b0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -319,9 +319,9 @@ struct device_attribute {

struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)

extern int __must_check device_create_file(struct device *device,

- struct device_attribute *entry);

+ const struct device_attribute *entry);
extern void device_remove_file(struct device *dev,

- struct device_attribute *attr);
+ const struct device_attribute *attr);

extern int __must_check device_create_bin_file(struct device *dev,

struct bin_attribute *attr);
extern void device_remove_bin_file(struct device *dev,

--
1.6.0.4

[Phil Carmody]

I know now's probably a bad time to be mentioning const issues
but I recently a reviewed a patch where 'const' was being removed
from a device_attribute structure, and I enquired why. The reason
was simply so that device_create_file() could be called, it taking
a non-const attribute pointer. Looking inside, all that function
did was pass a pointer to sysfs_create_file, which itself took a
const pointer. Non-constness was not required at all.

So here I offer a small patchset which I hope will permit and

encourage device and other attributes to be made const, and put
in read-only sections.

These have been compile/sparse tested against arm and x86_64 targets,

and through my usual suite of tests on the arm platform I use.

In addition to the changes in the prior RFC, I have updated the
documentation appropriately.

Regards,
Phil

[Phil Carmody]

Many struct driver_attribute descriptors are purely read-only

structures, and there's no need to change them. Therefore make
the promise not to, which will let those descriptors be put in
a ro section.

Signed-off-by: Phil Carmody <ext-phil....@nokia.com>
---

Documentation/driver-model/driver.txt | 4 ++--
Documentation/filesystems/sysfs.txt | 4 ++--
drivers/base/driver.c | 4 ++--
include/linux/device.h | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/driver-model/driver.txt b/Documentation/driver-model/driver.txt
index 60120fb..d2cd6fb 100644
--- a/Documentation/driver-model/driver.txt
+++ b/Documentation/driver-model/driver.txt
@@ -226,5 +226,5 @@ struct driver_attribute driver_attr_debug;
This can then be used to add and remove the attribute from the
driver's directory using:

-int driver_create_file(struct device_driver *, struct driver_attribute *);
-void driver_remove_file(struct device_driver *, struct driver_attribute *);
+int driver_create_file(struct device_driver *, const struct driver_attribute *);
+void driver_remove_file(struct device_driver *, const struct driver_attribute *);
diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
index a4ac2b9..931c806 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -358,7 +358,7 @@ DRIVER_ATTR(_name, _mode, _show, _store)

Creation/Removal:

-int driver_create_file(struct device_driver *, struct driver_attribute *);
-void driver_remove_file(struct device_driver *, struct driver_attribute *);
+int driver_create_file(struct device_driver *, const struct driver_attribute *);
+void driver_remove_file(struct device_driver *, const struct driver_attribute *);


diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index f367885..90c9fff 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -98,7 +98,7 @@ EXPORT_SYMBOL_GPL(driver_find_device);
* @attr: driver attribute descriptor.
*/
int driver_create_file(struct device_driver *drv,
- struct driver_attribute *attr)
+ const struct driver_attribute *attr)
{
int error;
if (drv)
@@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(driver_create_file);
* @attr: driver attribute descriptor.
*/
void driver_remove_file(struct device_driver *drv,
- struct driver_attribute *attr)
+ const struct driver_attribute *attr)
{
if (drv)
sysfs_remove_file(&drv->p->kobj, &attr->attr);
diff --git a/include/linux/device.h b/include/linux/device.h
index 0823a29..d07f90f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -166,9 +166,9 @@ struct driver_attribute driver_attr_##_name = \
__ATTR(_name, _mode, _show, _store)

extern int __must_check driver_create_file(struct device_driver *driver,
- struct driver_attribute *attr);
+ const struct driver_attribute *attr);
extern void driver_remove_file(struct device_driver *driver,
- struct driver_attribute *attr);
+ const struct driver_attribute *attr);

extern int __must_check driver_add_kobj(struct device_driver *drv,
struct kobject *kobj,

[Greg KH]

On Fri, Dec 18, 2009 at 10:52:21AM +0200, Phil Carmody wrote:
> On 18/12/09 00:16 +0100, ext Greg KH wrote:
> > On Thu, Dec 17, 2009 at 08:12:10PM +0200, Phil Carmody wrote:
> [...]> > So here I offer a small patchset which I hope will permit and
> > > encourage device and other attributes to be made const, and put
> > > in read-only sections.
> > >
> > > 1-3 address the three attribute types which seemed to be trivially
> > > const-able, and are the important part of the set.
> >
> > I like these, very nice.
>
> Thanks, I can possibly sniff a little bit deeper, and see if there are
> any other obvious throwing-away-of-consts nearby.

Great.

> > > 4 adds a new macro to encourage the use of Const ATTRibutes,
> > > and may need a better name. (I wanted to avoid RO, for example.)
> >
> > Hm, is this really needed? How badly do things break if you change the
> > current attribute macros to use 'const'? What subsystems are not using
> > const?
>
> That was the first thing that went through my mind, but I didn't want
> to be too brave. He who dares wins though, and I can certainly give
> that a try. allmodconfig is my friend.

That would be nice to try, let me know what you find.

> > > 5 and 6 are merely two quick examples of how easy it is to adopt
> > > the new const convention. In reality, these structures have been
> > > constant and treated as constant by the driver core all along, it's
> > > just that one word was missing from a few important places.
> > >
> > > I would hope to submit a patchset with 1-3 and a possibly modified
> > > 4. The migrations themselves will belong in different trees.
> >
> > I'll be glad to take 1-3 now, and queue it up for .34.
>
> They cleave cleanly at that point, so I'll resend with a [PATCH] prefix.

Got them, I'll go queue them up.

thanks again,

greg k-h