| Message ID | 20210903204548.2745354-16-arequipeno@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce block device LED trigger | expand |
On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote: > /sys/class/leds/<led>/add_blkdev - to create device/LED associations > > /sys/class/leds/<led>/delete_blkdev to remove device/LED associations > > For both attributes, accept multiple device names separated by whitespace > > Signed-off-by: Ian Pilcher <arequipeno@gmail.com> > --- > drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) Where are the Documentation/ABI/ updates for these new sysfs files? thanks, greg k-h
On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote: > /sys/class/leds/<led>/add_blkdev - to create device/LED associations > > /sys/class/leds/<led>/delete_blkdev to remove device/LED associations > > For both attributes, accept multiple device names separated by whitespace > > Signed-off-by: Ian Pilcher <arequipeno@gmail.com> > --- > drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c > index b2ec85b805d0..db82d37fc721 100644 > --- a/drivers/leds/trigger/ledtrig-blkdev.c > +++ b/drivers/leds/trigger/ledtrig-blkdev.c > @@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev) > > module_put(THIS_MODULE); > } > + > + > +/* > + * > + * sysfs attributes to add & delete devices from LEDs > + * > + */ > + > +static ssize_t blkdev_add_or_del(struct device *const dev, > + struct device_attribute *const attr, > + const char *const buf, const size_t count); > + > +static struct device_attribute ledtrig_blkdev_attr_add = > + __ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del); > + > +static struct device_attribute ledtrig_blkdev_attr_del = > + __ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del); DEVICE_ATTR_RO()? Or something like that? Do not use __ATTR() for device attributes if at all possible, worst case, use DEVICE_ATTR() here. And the mode settings are odd, are you sure you want that? > +static ssize_t blkdev_add_or_del(struct device *const dev, > + struct device_attribute *const attr, > + const char *const buf, const size_t count) > +{ > + struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); > + const char *const disk_name = blkdev_skip_space(buf); > + const char *const endp = blkdev_find_space(disk_name); > + const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */ > + int ret; > + > + if (name_len == 0) { > + pr_info("blkdev LED: empty block device name\n"); Looks like debugging code, please remove. And how can this ever happen? > + return -EINVAL; > + } > + > + if (attr == &ledtrig_blkdev_attr_del) { > + blkdev_disk_delete(led, disk_name, name_len); > + } else { /* attr == &ledtrig_blkdev_attr_add */ > + ret = blkdev_disk_add(led, disk_name, name_len); Why do you have a single attribute callback that does two totally different things? Just have 2 different callback functions please, it makes things much easier to review and maintain over time. thanks, greg k-h
On 9/4/21 12:57 AM, Greg KH wrote:
> Where are the Documentation/ABI/ updates for these new sysfs files?
They're in Documentation/ABI/testing/sysfs-class-led-trigger-blkdev,
which is added in the first patch in the series.
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
On 9/4/21 12:59 AM, Greg KH wrote: > DEVICE_ATTR_RO()? Or something like that? Do not use __ATTR() for > device attributes if at all possible, worst case, use DEVICE_ATTR() > here. For some reason, it didn't click until now that these are device attributes (because I was focused on the fact that I was working on the LED trigger). DEVICE_ATTR*() it is. > And the mode settings are odd, are you sure you want that? Yes. These are write-only attributes. >> + if (name_len == 0) { >> + pr_info("blkdev LED: empty block device name\n"); > > Looks like debugging code, please remove. It's really more of an error message for the system administrator. So as with my earlier note, dev_info() would be my preference. > And how can this ever happen? The blkdev_skip_space() and blkdev_find_space() calls effectively find the first non-whitespace token in the buffer (disk_name) and its length (name_len). If the buffer only contains whitespace (e.g. echo > $ATTR), then name_len will be 0. > >> + return -EINVAL; >> + } >> + >> + if (attr == &ledtrig_blkdev_attr_del) { >> + blkdev_disk_delete(led, disk_name, name_len); >> + } else { /* attr == &ledtrig_blkdev_attr_add */ >> + ret = blkdev_disk_add(led, disk_name, name_len); > > Why do you have a single attribute callback that does two totally > different things? Just have 2 different callback functions please, it > makes things much easier to review and maintain over time. Hmmm. All of the "real work" is done in blkdev_disk_delete() and blkdev_disk_add(). The store function's only purpose is to parse the token(s) from the buffer, and that logic is exactly the same for the two different attributes. So it's a choice between: > static ssize_t blkdev_add_or_del(struct device *const dev, > struct device_attribute *const attr, > const char *const buf, const size_t count) > { > struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); > const char *const disk_name = blkdev_skip_space(buf); > const char *const endp = blkdev_find_space(disk_name); > const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */ > int ret; > > if (name_len == 0) { > pr_info("blkdev LED: empty block device name\n"); > return -EINVAL; > } > > if (attr == &ledtrig_blkdev_attr_del) { > blkdev_disk_delete(led, disk_name, name_len); > } else { /* attr == &ledtrig_blkdev_attr_add */ > ret = blkdev_disk_add(led, disk_name, name_len); > if (ret != 0) > return ret; > } > > /* > * Consume everything up to the next non-whitespace token (or the end > * of the input). Avoids "empty block device name" error if there is > * whitespace (such as a newline) after the last token. > */ > return blkdev_skip_space(endp) - buf; > } Or: > static ssize_t blkdev_add(struct device *const dev, > struct device_attribute *const attr, > const char *const buf, const size_t count) > { > struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); > const char *const disk_name = blkdev_skip_space(buf); > const char *const endp = blkdev_find_space(disk_name); > const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */ > int ret; > > if (name_len == 0) { > pr_info("blkdev LED: empty block device name\n"); > return -EINVAL; > } > > ret = blkdev_disk_add(led, disk_name, name_len); > if (ret != 0) > return ret; > > /* > * Consume everything up to the next non-whitespace token (or the end > * of the input). Avoids "empty block device name" error if there is > * whitespace (such as a newline) after the last token. > */ > return blkdev_skip_space(endp) - buf; > } > > > static ssize_t blkdev_del(struct device *const dev, > struct device_attribute *const attr, > const char *const buf, const size_t count) > { > struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); > const char *const disk_name = blkdev_skip_space(buf); > const char *const endp = blkdev_find_space(disk_name); > const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */ > int ret; > > if (name_len == 0) { > pr_info("blkdev LED: empty block device name\n"); > return -EINVAL; > } > > blkdev_disk_delete(led, disk_name, name_len); > > /* See comment in blkdev_add() */ > return blkdev_skip_space(endp) - buf; > } Maybe the right thing to do is to simply add a comment to clarify the separation (and maybe rename the function as well)? Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Sat, Sep 04, 2021 at 05:35:29PM -0500, Ian Pilcher wrote: > On 9/4/21 12:59 AM, Greg KH wrote: > > DEVICE_ATTR_RO()? Or something like that? Do not use __ATTR() for > > device attributes if at all possible, worst case, use DEVICE_ATTR() > > here. > > For some reason, it didn't click until now that these are device > attributes (because I was focused on the fact that I was working on the > LED trigger). > > DEVICE_ATTR*() it is. > > > And the mode settings are odd, are you sure you want that? > > Yes. These are write-only attributes. DEVICE_ATTR_WO() then please. > > > + if (name_len == 0) { > > > + pr_info("blkdev LED: empty block device name\n"); > > > > Looks like debugging code, please remove. > > It's really more of an error message for the system administrator. So > as with my earlier note, dev_info() would be my preference. Nope, dev_err() for real errors please. > > And how can this ever happen? > > The blkdev_skip_space() and blkdev_find_space() calls effectively find > the first non-whitespace token in the buffer (disk_name) and its length > (name_len). If the buffer only contains whitespace (e.g. echo > $ATTR), > then name_len will be 0. That's a crazy interface, as others pointed out, don't do that please. thanks, greg k-h
On 9/5/21 9:51 AM, Greg KH wrote: >> It's really more of an error message for the system administrator. So >> as with my earlier note, dev_info() would be my preference. > > Nope, dev_err() for real errors please. Just to clarify, the error in this case is the system administrator writing an incorrect value to a sysfs attribute (likely via a udev rule), i.e. a "pilot error." One of the reviewers of one of my RFC patch sets commented that those should be INFO level at most. So dev_err() or dev_info() for that sort of thing (always given that only the root user has permission to write to trigger the error message)? >> The blkdev_skip_space() and blkdev_find_space() calls effectively find >> the first non-whitespace token in the buffer (disk_name) and its length >> (name_len). If the buffer only contains whitespace (e.g. echo > $ATTR), >> then name_len will be 0. > > That's a crazy interface, as others pointed out, don't do that please. As Pavel noted, it would be ideal to use symlink()/unlink() in the LED's block_devices directory for this. As far as I know however, sysfs doesn't support doing that. I'd be happy to learn otherwise. I would also welcome any other suggestions for a better interface for setting up the many-to-many relationships that the trigger supports. That said, I don't know what that has to do with blkdev_skip_space() and blkdev_find_space(), which are just helper functions that I use to parse the device name out of the buffer passed to the store function. Ultimately, the store function does need to handle the case where the system administrator (or a broken udev rule) writes an all-whitespace string to the attribute. I will try to restructure the code to make things more clear. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Sun, Sep 05, 2021 at 10:33:08AM -0500, Ian Pilcher wrote: > On 9/5/21 9:51 AM, Greg KH wrote: > > > It's really more of an error message for the system administrator. So > > > as with my earlier note, dev_info() would be my preference. > > > > Nope, dev_err() for real errors please. > > Just to clarify, the error in this case is the system administrator > writing an incorrect value to a sysfs attribute (likely via a udev > rule), i.e. a "pilot error." > > One of the reviewers of one of my RFC patch sets commented that those > should be INFO level at most. > > So dev_err() or dev_info() for that sort of thing (always given that > only the root user has permission to write to trigger the error > message)? Really you should not have any kernel log messages for invalid data sent to a sysfs file, just return -EINVAL and be done with it. > > > The blkdev_skip_space() and blkdev_find_space() calls effectively find > > > the first non-whitespace token in the buffer (disk_name) and its length > > > (name_len). If the buffer only contains whitespace (e.g. echo > $ATTR), > > > then name_len will be 0. > > > > That's a crazy interface, as others pointed out, don't do that please. > > As Pavel noted, it would be ideal to use symlink()/unlink() in the LED's > block_devices directory for this. As far as I know however, sysfs > doesn't support doing that. I'd be happy to learn otherwise. I would > also welcome any other suggestions for a better interface for setting up > the many-to-many relationships that the trigger supports. sysfs does not allow that as that is not what sysfs is for. Perhaps you want to use configfs, as that is exactly what that is for. > That said, I don't know what that has to do with blkdev_skip_space() and > blkdev_find_space(), which are just helper functions that I use to parse > the device name out of the buffer passed to the store function. > Ultimately, the store function does need to handle the case where the > system administrator (or a broken udev rule) writes an all-whitespace > string to the attribute. Handling invalid data is fine, but having to parse multiple values in a single sysfs file violates the rules of sysfs. Please use something else instead. thanks, greg k-h
diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c index b2ec85b805d0..db82d37fc721 100644 --- a/drivers/leds/trigger/ledtrig-blkdev.c +++ b/drivers/leds/trigger/ledtrig-blkdev.c @@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev) module_put(THIS_MODULE); } + + +/* + * + * sysfs attributes to add & delete devices from LEDs + * + */ + +static ssize_t blkdev_add_or_del(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count); + +static struct device_attribute ledtrig_blkdev_attr_add = + __ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del); + +static struct device_attribute ledtrig_blkdev_attr_del = + __ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del); + +static ssize_t blkdev_add_or_del(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count) +{ + struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); + const char *const disk_name = blkdev_skip_space(buf); + const char *const endp = blkdev_find_space(disk_name); + const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */ + int ret; + + if (name_len == 0) { + pr_info("blkdev LED: empty block device name\n"); + return -EINVAL; + } + + if (attr == &ledtrig_blkdev_attr_del) { + blkdev_disk_delete(led, disk_name, name_len); + } else { /* attr == &ledtrig_blkdev_attr_add */ + ret = blkdev_disk_add(led, disk_name, name_len); + if (ret != 0) + return ret; + } + + /* + * Consume everything up to the next non-whitespace token (or the end + * of the input). Avoids "empty block device name" error if there is + * whitespace (such as a newline) after the last token. + */ + return blkdev_skip_space(endp) - buf; +}
/sys/class/leds/<led>/add_blkdev - to create device/LED associations /sys/class/leds/<led>/delete_blkdev to remove device/LED associations For both attributes, accept multiple device names separated by whitespace Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)