Re: [KJ] another potential project: using sysfs ATTR macros.

Re: [KJ] another potential project: using sysfs ATTR macros.

[Håkon Løvdal]

On 25/02/07, Greg KH <greg@kroah.com> wrote:> On Sun, Feb 25, 2007 at 02:46:12PM -0500, Robert P. J. Day wrote:> > #define __ATTR_RO(_name) { \> >         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \> >         .show   = _name##_show, \> > }...> >   the downside to this is that you can see that the __ATTR_RO macro> > expects a definite naming convention -- the "show" routine *must* be> > named "_name##_show".  in cases where that's not true, you're> > obviously screwed and you can't use that macro.  otherwise, you could> > have used *that* for this very same example.  grrrrrrrrrrrrrr.>> Any idea how to fix that in a sane manner?
The preprocessor can be used for this:
static ssize_t show_whatever(struct class_device *class_device, char *buf){	...}...#define whatever_show show_whatever
static struct class_device_attribute class_device_attrs[] = {	__ATTR_RO(whatever),};
BR Håkon Løvdal
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Robert P. J. Day]

On Sun, 25 Feb 2007, Greg KH wrote:

> On Sun, Feb 25, 2007 at 02:52:18PM -0500, Robert P. J. Day wrote:
> > On Sun, 25 Feb 2007, Robert P. J. Day wrote:
> >
> > >
> > >   (and while we wait and wait and wait for the new wiki to come back
> > > up ...)
> > >
> > >   the header file linux/sysfs.h defines a couple macros that make it
> > > easier to define sysfs attributes:
> > >
> > > ===================> > > #define __ATTR(_name,_mode,_show,_store) { \
> > >         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
> > >         .show   = _show,                                        \
> > >         .store  = _store,                                       \
> > > }
> > >
> > > #define __ATTR_RO(_name) { \
> > >         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
> > >         .show   = _name##_show, \
> > > }
> > > ====================
> >
> > as a short followup to my own post, are there any other common
> > combinations of creating a sysfs attribute that would justify another
> > flavour of macro?  just curious.
>
> You mean becides DEVICE_ATTR(), CLASS_ATTR() and other *_ATTR macros
> in device.h?  :)

actually, yes, besides those.  if you look at the definition of, say,
CLASS_DEVICE_ATTR:

#define CLASS_DEVICE_ATTR(_name,_mode,_show,_store)             \
struct class_device_attribute class_device_attr_##_name =       \
        __ATTR(_name,_mode,_show,_store)

it was clearly created to abbreviate initializing a particular kind of
attribute.  i was more asking whether there were other variations of
__ATTR or __ATTR_RO that would be useful.  why just those two?  why
not __ATTR_RW as well?  and so on.  it just seems awkward to have
defined the generic __ATTR macro, then added that incompatible
__ATTR_RO macro, and stopped there.

i think the first thing to do is agree on a fundamental and consistent
set of macros for this, if that's even possible at this point.

rday

p.s.  certainly, one possible cleanup (did i mention this already?)
would be to replace stuff like:

  #define EFI_ATTR(_name, _mode, _show, _store) \
  struct subsys_attribute efi_attr_##_name = { \
        .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE}, \
        .show = _show, \
        .store = _store, \
  };

with the simpler form of:

  #define EFI_ATTR(_name, _mode, _show, _store) \
  struct subsys_attribute efi_attr_##_name = __ATTR(etc etc)

that could be done right now, as long as there's no chance of __ATTR
being redefined any time soon.

-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Greg KH]

On Sun, Feb 25, 2007 at 02:52:18PM -0500, Robert P. J. Day wrote:
> On Sun, 25 Feb 2007, Robert P. J. Day wrote:
> 
> >
> >   (and while we wait and wait and wait for the new wiki to come back
> > up ...)
> >
> >   the header file linux/sysfs.h defines a couple macros that make it
> > easier to define sysfs attributes:
> >
> > ===================> > #define __ATTR(_name,_mode,_show,_store) { \
> >         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
> >         .show   = _show,                                        \
> >         .store  = _store,                                       \
> > }
> >
> > #define __ATTR_RO(_name) { \
> >         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
> >         .show   = _name##_show, \
> > }
> > ====================
> 
> as a short followup to my own post, are there any other common
> combinations of creating a sysfs attribute that would justify another
> flavour of macro?  just curious.

You mean becides DEVICE_ATTR(), CLASS_ATTR() and other *_ATTR macros in
device.h?  :)
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Robert P. J. Day]

On Sun, 25 Feb 2007, Greg KH wrote:
...
> Again, looks like a good job to clean up.

i'll write this up on the wiki ... if it ever comes back up again.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Greg KH]

On Sun, Feb 25, 2007 at 04:26:24PM -0500, Robert P. J. Day wrote:
> On Sun, 25 Feb 2007, Greg KH wrote:
> 
> > On Sun, Feb 25, 2007 at 02:46:12PM -0500, Robert P. J. Day wrote:
> ...
> > > ===================> > > #define __ATTR(_name,_mode,_show,_store) { \
> > >         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
> > >         .show   = _show,                                        \
> > >         .store  = _store,                                       \
> > > }
> > >
> > > #define __ATTR_RO(_name) { \
> > >         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
> > >         .show   = _name##_show, \
> > > }
> > > ====================
> ...
> > > static struct class_device_attribute ipr_fw_version_attr = {
> > >         .attr = {
> > >                 .name =         "fw_version",
> > >                 .mode =         S_IRUGO,
> > >         },
> > >         .show = ipr_show_fw_version,
> > > };
> > >
> > >   this could obviously be rewritten as the shorter:
> > >
> > > ... ipr_fw_version_attr = _ATTR(fw_version, S_IRUGO, ipr_show_fw_version);
> > >
> > > would this be worth putting any effort into?
> >
> > Yes, because that hand-rolled version above is buggy (they forgot the
> > .owner setting).
> 
> um ... you *had* noticed that there about a billion of those cases,
> where someone forgot to set the .owner value?  :-)

No I had not :(

> as a random example, consider fs/dlm/lockspace.c, which still
> re-invents the wheel (at least to some extent):
> 
> ...
> struct dlm_attr {
>         struct attribute attr;
>         ssize_t (*show)(struct dlm_ls *, char *);
>         ssize_t (*store)(struct dlm_ls *, const char *, size_t);
> };
> ...
> static struct dlm_attr dlm_attr_control = {
>         .attr  = {.name = "control", .mode = S_IWUSR},
>         .store = dlm_control_store
> };
> 
> static struct dlm_attr dlm_attr_event = {
>         .attr  = {.name = "event_done", .mode = S_IWUSR},
>         .store = dlm_event_store
> };
> ... etc etc etc ...  there's plenty of those all over the place.

Looks like a good job for people to start fixing up.

> > >   the downside to this is that you can see that the __ATTR_RO
> > > macro expects a definite naming convention -- the "show" routine
> > > *must* be named "_name##_show".  in cases where that's not true,
> > > you're obviously screwed and you can't use that macro.
> > > otherwise, you could have used *that* for this very same example.
> > > grrrrrrrrrrrrrr.
> >
> > Any idea how to fix that in a sane manner?
> 
> not offhand.  everyone seems to have invented their own naming
> standards, so i think you're stuck with the existing chaos.  i mean,
> the inherent inconsistency between the definitions of __ATTR and
> __ATTR_RO is enough to give you a headache.
> 
> at the very least, it might be nice to make a short list of the
> absolutely most common variations of defining one of these sysfs
> attribute structures and creating a macro for those situations.
> 
> rday
> 
> p.s.  even people who are trying to be efficient end up defining
> macros that conflict with the "standard" ones, like in
> arch/x86_64/kernel/mce_amd.c:
> 
> #define THRESHOLD_ATTR(_name,_mode,_show,_store) {            \
>         .attr = {.name = __stringify(_name), .mode = _mode }, \
>         .show = _show,                                        \
>         .store = _store,                                      \
> };
> 
> #define RW_ATTR(name)                                           \
> static struct threshold_attr name =                             \
>         THRESHOLD_ATTR(name, 0644, show_## name, store_## name)
> 
> RW_ATTR(interrupt_enable);
> RW_ATTR(threshold_limit);
> RW_ATTR(error_count);
> 
>   so ... still no assignment to .owner, and in RW_ATTR, the strings
> "_show" and "_store" come *after* the name, not before.
> 
>   it's just messy.

Again, looks like a good job to clean up.

thanks,

greg k-h
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Robert P. J. Day]

On Sun, 25 Feb 2007, Greg KH wrote:

> On Sun, Feb 25, 2007 at 02:46:12PM -0500, Robert P. J. Day wrote:
...
> > ===================> > #define __ATTR(_name,_mode,_show,_store) { \
> >         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
> >         .show   = _show,                                        \
> >         .store  = _store,                                       \
> > }
> >
> > #define __ATTR_RO(_name) { \
> >         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
> >         .show   = _name##_show, \
> > }
> > ====================
...
> > static struct class_device_attribute ipr_fw_version_attr = {
> >         .attr = {
> >                 .name =         "fw_version",
> >                 .mode =         S_IRUGO,
> >         },
> >         .show = ipr_show_fw_version,
> > };
> >
> >   this could obviously be rewritten as the shorter:
> >
> > ... ipr_fw_version_attr = _ATTR(fw_version, S_IRUGO, ipr_show_fw_version);
> >
> > would this be worth putting any effort into?
>
> Yes, because that hand-rolled version above is buggy (they forgot the
> .owner setting).

um ... you *had* noticed that there about a billion of those cases,
where someone forgot to set the .owner value?  :-)

as a random example, consider fs/dlm/lockspace.c, which still
re-invents the wheel (at least to some extent):

...
struct dlm_attr {
        struct attribute attr;
        ssize_t (*show)(struct dlm_ls *, char *);
        ssize_t (*store)(struct dlm_ls *, const char *, size_t);
};
...
static struct dlm_attr dlm_attr_control = {
        .attr  = {.name = "control", .mode = S_IWUSR},
        .store = dlm_control_store
};

static struct dlm_attr dlm_attr_event = {
        .attr  = {.name = "event_done", .mode = S_IWUSR},
        .store = dlm_event_store
};
... etc etc etc ...  there's plenty of those all over the place.

> >   the downside to this is that you can see that the __ATTR_RO
> > macro expects a definite naming convention -- the "show" routine
> > *must* be named "_name##_show".  in cases where that's not true,
> > you're obviously screwed and you can't use that macro.
> > otherwise, you could have used *that* for this very same example.
> > grrrrrrrrrrrrrr.
>
> Any idea how to fix that in a sane manner?

not offhand.  everyone seems to have invented their own naming
standards, so i think you're stuck with the existing chaos.  i mean,
the inherent inconsistency between the definitions of __ATTR and
__ATTR_RO is enough to give you a headache.

at the very least, it might be nice to make a short list of the
absolutely most common variations of defining one of these sysfs
attribute structures and creating a macro for those situations.

rday

p.s.  even people who are trying to be efficient end up defining
macros that conflict with the "standard" ones, like in
arch/x86_64/kernel/mce_amd.c:

#define THRESHOLD_ATTR(_name,_mode,_show,_store) {            \
        .attr = {.name = __stringify(_name), .mode = _mode }, \
        .show = _show,                                        \
        .store = _store,                                      \
};

#define RW_ATTR(name)                                           \
static struct threshold_attr name =                             \
        THRESHOLD_ATTR(name, 0644, show_## name, store_## name)

RW_ATTR(interrupt_enable);
RW_ATTR(threshold_limit);
RW_ATTR(error_count);

  so ... still no assignment to .owner, and in RW_ATTR, the strings
"_show" and "_store" come *after* the name, not before.

  it's just messy.

--
====================================
Robert P. J. Day Linux Consulting, Training and Annoying Kernel
Pedantry Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Greg KH]

On Sun, Feb 25, 2007 at 02:46:12PM -0500, Robert P. J. Day wrote:
> 
>   (and while we wait and wait and wait for the new wiki to come back
> up ...)
> 
>   the header file linux/sysfs.h defines a couple macros that make it
> easier to define sysfs attributes:
> 
> ===================> #define __ATTR(_name,_mode,_show,_store) { \
>         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
>         .show   = _show,                                        \
>         .store  = _store,                                       \
> }
> 
> #define __ATTR_RO(_name) { \
>         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
>         .show   = _name##_show, \
> }
> ====================
> 
>   while these macros are used frequently in the source tree:
> 
>   $ grep -rw __ATTR *
>   $ grep -rw __ATTR_RO *
> 
> there are definitely a lot of places that still initialize the
> structure manually which could be abbreviated, such as this from
> drivers/scsi/ipr.c:
> 
> static struct class_device_attribute ipr_fw_version_attr = {
>         .attr = {
>                 .name =         "fw_version",
>                 .mode =         S_IRUGO,
>         },
>         .show = ipr_show_fw_version,
> };
> 
>   this could obviously be rewritten as the shorter:
> 
> ... ipr_fw_version_attr = _ATTR(fw_version, S_IRUGO, ipr_show_fw_version);
> 
> would this be worth putting any effort into?

Yes, because that hand-rolled version above is buggy (they forgot the
.owner setting).

>   the downside to this is that you can see that the __ATTR_RO macro
> expects a definite naming convention -- the "show" routine *must* be
> named "_name##_show".  in cases where that's not true, you're
> obviously screwed and you can't use that macro.  otherwise, you could
> have used *that* for this very same example.  grrrrrrrrrrrrrr.

Any idea how to fix that in a sane manner?

thanks,

greg k-h
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] another potential project: using sysfs ATTR macros.

[Robert P. J. Day]

On Sun, 25 Feb 2007, Robert P. J. Day wrote:

>
>   (and while we wait and wait and wait for the new wiki to come back
> up ...)
>
>   the header file linux/sysfs.h defines a couple macros that make it
> easier to define sysfs attributes:
>
> ===================> #define __ATTR(_name,_mode,_show,_store) { \
>         .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
>         .show   = _show,                                        \
>         .store  = _store,                                       \
> }
>
> #define __ATTR_RO(_name) { \
>         .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
>         .show   = _name##_show, \
> }
> ====================

as a short followup to my own post, are there any other common
combinations of creating a sysfs attribute that would justify another
flavour of macro?  just curious.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] another potential project: using sysfs ATTR macros.

[Robert P. J. Day]

  (and while we wait and wait and wait for the new wiki to come back
up ...)

  the header file linux/sysfs.h defines a couple macros that make it
easier to define sysfs attributes:

===================#define __ATTR(_name,_mode,_show,_store) { \
        .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },     \
        .show   = _show,                                        \
        .store  = _store,                                       \
}

#define __ATTR_RO(_name) { \
        .attr   = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE },   \
        .show   = _name##_show, \
}
====================

  while these macros are used frequently in the source tree:

  $ grep -rw __ATTR *
  $ grep -rw __ATTR_RO *

there are definitely a lot of places that still initialize the
structure manually which could be abbreviated, such as this from
drivers/scsi/ipr.c:

static struct class_device_attribute ipr_fw_version_attr = {
        .attr = {
                .name =         "fw_version",
                .mode =         S_IRUGO,
        },
        .show = ipr_show_fw_version,
};

  this could obviously be rewritten as the shorter:

... ipr_fw_version_attr = _ATTR(fw_version, S_IRUGO, ipr_show_fw_version);

would this be worth putting any effort into?

  the downside to this is that you can see that the __ATTR_RO macro
expects a definite naming convention -- the "show" routine *must* be
named "_name##_show".  in cases where that's not true, you're
obviously screwed and you can't use that macro.  otherwise, you could
have used *that* for this very same example.  grrrrrrrrrrrrrr.

  anyway, thoughts?

rday
--
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors