| Message ID | 20210916202127.1216994-1-arequipeno@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce block device LED trigger | expand |
On Thu, Sep 16, 2021 at 03:21:26PM -0500, Ian Pilcher wrote: > +What: /sys/class/leds/<led>/link_device > +Date: September 2021 > +Contact: Ian Pilcher <arequipeno@gmail.com> > +Description: > + Associate a block device with this LED by writing the path to > + the device special file (e.g. /dev/sda) to this attribute. > + Symbolic links are followed. Optionally, the leading "/dev/" > + may be omitted. No, please don't follow symlinks, stick with kernel names here, otherwise you have a mismatch between that and the list of devices in this file: > +What: /sys/class/leds/<led>/linked_devices thanks, greg k-h
Combining 2 related threads ... On 9/17/21 01:19, Greg KH wrote: > On Thu, Sep 16, 2021 at 03:21:26PM -0500, Ian Pilcher wrote: >> +What: /sys/class/leds/<led>/link_device >> +Date: September 2021 >> +Contact: Ian Pilcher <arequipeno@gmail.com> >> +Description: >> + Associate a block device with this LED by writing the path to >> + the device special file (e.g. /dev/sda) to this attribute. >> + Symbolic links are followed. Optionally, the leading "/dev/" >> + may be omitted. > > No, please don't follow symlinks, stick with kernel names here, > otherwise you have a mismatch between that and the list of devices in > this file: > >> +What: /sys/class/leds/<led>/linked_devices I did update the documentation to mention that fact. Following symlinks is the behavior of blkdev_get_by_path(), not some- thing that my code is doing. As far as using kernel names, that would be my preference, but I simply don't know of any way to do so with the existing block API. To my knowledge, there simply isn't anything like a blkdev_get_by_name() API. This the reason that I added the "retry" logic to led_bdev_get(). It doesn't prevent the system administrator from using a symbolic link (or an oddly named special file), but it does make an unqualified name like "sda" work if the expected special file exists in /dev. However ... On 9/17/21 00:53, Christoph Hellwig wrote: > On Thu, Sep 16, 2021 at 03:21:27PM -0500, Ian Pilcher wrote: >> +static struct block_device *led_bdev_get(const char *const buf, >> + const size_t count, fmode_t mode) >> +{ >> + static const char dev[] = "/dev/"; >> + struct block_device *bdev; >> + char *dev_path, *path; >> + >> + /* sizeof(dev) includes terminating null */ >> + dev_path = kmalloc(sizeof(dev) + count, GFP_KERNEL); >> + if (dev_path == NULL) >> + return ERR_PTR(-ENOMEM); >> + >> + /* sizeof(dev) - 1 is compile-time equivalent of strlen(dev) */ >> + memcpy(dev_path, dev, sizeof(dev) - 1); >> + path = dev_path + sizeof(dev) - 1; >> + memcpy(path, buf, count + 1); /* include terminating null */ >> + strim(path); >> + >> +try_blkdev_get: >> + bdev = blkdev_get_by_path(path, mode, THIS_MODULE); >> + if (IS_ERR(bdev) && PTR_ERR(bdev) == -ENOENT && path != dev_path) { >> + path = dev_path; >> + goto try_blkdev_get; >> + } >> + >> + kfree(dev_path); >> + return bdev; > > Please just required the user to put in the whole path and remove all > this garbage. There is no need to build your own broken wrappers around > the VFS path resolution. Please be specific about what is broken. If you see an actual bug in the code, please tell me what it is. If (as I suspect) you disagree with the basic idea of retrying with "/dev/" prepended to the supplied path, please say that. Honestly, I wasn't particularly enthusiastic about it in the first place; it feels like something that should be done in user space. I wouldn't have included it if I didn't have to make a writable copy of the buffer anyway, in order to trim a trailing newline. I can certainly remove the re-check logic. The end result will be an API that is slightly less "user friendly" in return for saving a bit of pointer arithmetic and a 5-byte memcpy(). -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Fri, Sep 17, 2021 at 03:46:55PM -0500, Ian Pilcher wrote: > Combining 2 related threads ... > > On 9/17/21 01:19, Greg KH wrote: > > On Thu, Sep 16, 2021 at 03:21:26PM -0500, Ian Pilcher wrote: > > > +What: /sys/class/leds/<led>/link_device > > > +Date: September 2021 > > > +Contact: Ian Pilcher <arequipeno@gmail.com> > > > +Description: > > > + Associate a block device with this LED by writing the path to > > > + the device special file (e.g. /dev/sda) to this attribute. > > > + Symbolic links are followed. Optionally, the leading "/dev/" > > > + may be omitted. > > > > No, please don't follow symlinks, stick with kernel names here, > > otherwise you have a mismatch between that and the list of devices in > > this file: > > > > > +What: /sys/class/leds/<led>/linked_devices > > I did update the documentation to mention that fact. > > Following symlinks is the behavior of blkdev_get_by_path(), not some- > thing that my code is doing. > > As far as using kernel names, that would be my preference, but I simply > don't know of any way to do so with the existing block API. To my > knowledge, there simply isn't anything like a blkdev_get_by_name() API. > > This the reason that I added the "retry" logic to led_bdev_get(). It > doesn't prevent the system administrator from using a symbolic link (or > an oddly named special file), but it does make an unqualified name like > "sda" work if the expected special file exists in /dev. > > However ... > > On 9/17/21 00:53, Christoph Hellwig wrote: > > On Thu, Sep 16, 2021 at 03:21:27PM -0500, Ian Pilcher wrote: > > > +static struct block_device *led_bdev_get(const char *const buf, > > > + const size_t count, fmode_t mode) > > > +{ > > > + static const char dev[] = "/dev/"; > > > + struct block_device *bdev; > > > + char *dev_path, *path; > > > + > > > + /* sizeof(dev) includes terminating null */ > > > + dev_path = kmalloc(sizeof(dev) + count, GFP_KERNEL); > > > + if (dev_path == NULL) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + /* sizeof(dev) - 1 is compile-time equivalent of strlen(dev) */ > > > + memcpy(dev_path, dev, sizeof(dev) - 1); > > > + path = dev_path + sizeof(dev) - 1; > > > + memcpy(path, buf, count + 1); /* include terminating null */ > > > + strim(path); > > > + > > > +try_blkdev_get: > > > + bdev = blkdev_get_by_path(path, mode, THIS_MODULE); > > > + if (IS_ERR(bdev) && PTR_ERR(bdev) == -ENOENT && path != dev_path) { > > > + path = dev_path; > > > + goto try_blkdev_get; > > > + } > > > + > > > + kfree(dev_path); > > > + return bdev; > > > > Please just required the user to put in the whole path and remove all > > this garbage. There is no need to build your own broken wrappers around > > the VFS path resolution. > > Please be specific about what is broken. > > If you see an actual bug in the code, please tell me what it is. > > If (as I suspect) you disagree with the basic idea of retrying with > "/dev/" prepended to the supplied path, please say that. > > Honestly, I wasn't particularly enthusiastic about it in the first > place; it feels like something that should be done in user space. I > wouldn't have included it if I didn't have to make a writable copy of > the buffer anyway, in order to trim a trailing newline. > > I can certainly remove the re-check logic. The end result will be an > API that is slightly less "user friendly" in return for saving a bit of > pointer arithmetic and a 5-byte memcpy(). Just use the kernel block device name and that way you do not have to parse anything as it is unique and no paths are having to be followed. That's the way that other LED apis are working, right? thanks, greg k-h
On 9/18/21 02:07, Greg KH wrote: > On Fri, Sep 17, 2021 at 03:46:55PM -0500, Ian Pilcher wrote: >> As far as using kernel names, that would be my preference, but I simply >> don't know of any way to do so with the existing block API. To my >> knowledge, there simply isn't anything like a blkdev_get_by_name() API. ... > Just use the kernel block device name and that way you do not have to > parse anything as it is unique and no paths are having to be followed. > > That's the way that other LED apis are working, right? Greg - There are 2 existing LED triggers that have similar functionality (i.e. they allow LEDs to be "associated" with devices by name), and they both use subsystem-specific APIs to "resolve" those names to the actual kernel objects on which they operate. * The *netdev* trigger uses dev_get_by_name(), which is specific to network devices (despite its name). * The *tty* trigger uses tty_dev_name_to_number() and tty_kopen_shared(). As I've been saying, I simply don't know of any similar API for block devices. The block API provides blkdev_get_by_path(), which I am using, and blkdev_get_by_dev(), which takes the device number (dev_t). If you know of an API that will allow me to resolve a block device (or dev_t) by its kernel name, please share that information. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Sat, Sep 18, 2021 at 09:07:54AM +0200, Greg KH wrote: > > Honestly, I wasn't particularly enthusiastic about it in the first > > place; it feels like something that should be done in user space. I > > wouldn't have included it if I didn't have to make a writable copy of > > the buffer anyway, in order to trim a trailing newline. > > > > I can certainly remove the re-check logic. The end result will be an > > API that is slightly less "user friendly" in return for saving a bit of > > pointer arithmetic and a 5-byte memcpy(). > > Just use the kernel block device name and that way you do not have to > parse anything as it is unique and no paths are having to be followed. > > That's the way that other LED apis are working, right? The "kernel block device name" is the a block device special path that a normal VFS path lookup is done on. This is the preferred block device API used by everyone. And yes, this includes resolving symlinks. The only other API is by dev_t, but it is highly discouraged and should really not grow any new users.
On Mon, 20 Sep 2021 07:43:52 +0100 Christoph Hellwig <hch@infradead.org> wrote: > On Sat, Sep 18, 2021 at 09:07:54AM +0200, Greg KH wrote: > > > Honestly, I wasn't particularly enthusiastic about it in the first > > > place; it feels like something that should be done in user space. > > > I wouldn't have included it if I didn't have to make a writable > > > copy of the buffer anyway, in order to trim a trailing newline. > > > > > > I can certainly remove the re-check logic. The end result will > > > be an API that is slightly less "user friendly" in return for > > > saving a bit of pointer arithmetic and a 5-byte memcpy(). > > > > Just use the kernel block device name and that way you do not have > > to parse anything as it is unique and no paths are having to be > > followed. > > > > That's the way that other LED apis are working, right? > > The "kernel block device name" is the a block device special path > that a normal VFS path lookup is done on. This is the preferred block > device API used by everyone. And yes, this includes resolving > symlinks. The only other API is by dev_t, but it is highly > discouraged and should really not grow any new users. Christoph, /sys/class/block lists block devices' kernel object names. I don't understand why can't blk API provide a function returns a block device given such name as seen in /sys/class/block directory. Can you elaborate on this? It seems really strange to me to not be able to do cd /sys/class/leds/<LED> echo blkdev >trigger echo sda1 >block_device and instead having to do (as the last command) echo /dev/sda1 >block_device And whas should we show when /dev/sda1 is paried to the trigger, and userspace reads the block_device sysfs file? Should we show the full path which was given when pairing, even if it may not be valid anymore? (Such as when the device file is removed from /dev.) Marek
Changes from v3: ================ * Use blkdev_get_by_path() to "resolve" block devices (struct block_device). With this change, there are now no changes required to the block subsystem, so there are only 2 patches in this series. * link_device and unlink_device attributes now take paths to block device special files (e.g. /dev/sda), rather than kernel names. Symbolic links also work. If the path written to the attribute doesn't exist (-ENOENT), we re-try with /dev/ prepended, so "simple" names like sda will still work as long as the corresponding special file exists in /dev. * Fixed a bug that could cause "phantom" blinks because of old device activity that was not recognized at the correct time. * (Slightly) more detailed commit message for the patch that adds the trigger code. As with v3, the real details are found in the comments in the source file. Changes from v2: ================ * Allow LEDs to be "linked" to partitions, as well as whole devices. Internally, the trigger now works with block_device structs, rather than gendisk structs. (Investigating the lifecycle of block_device structs led me to discover the device resource API, so ...) * Use the device resource API to manage the trigger's per-block device data structure (struct led_bdev_bdi). The trigger now uses a release function to remove references to block devices that have been removed. Because the release function is automatically called by the driver core, there is no longer any need for the block layer to explictly call the trigger's cleanup function. * Since there is no need to provide a built-in "stub" cleanup function when the trigger is built as a module, I have removed the always built-in "core" portion of the trigger. * Without a built-in component, the module does need access to the block_class symbol. The second patch in this series exports the symbol to the LEDTRIG_BLKDEV namespace and explains the reason for doing so. * Changed the interval sysfs attribute from a device attribute to a class attribute. It's single value that applies to all LEDs, so it didn't make sense as a device atribute. * As requested, I am posting the trigger code (ledtrig-blkdev.c) as a single patch. This eliminates the commit messages that would otherwise describe sections of the code, so I have added fairly extensive comments to each function. Changes from v1: ================ * Use correct address for LKML. * Renamed the sysfs attributes used to manage and view the set of block devices associated ("linked") with an LED. - /sys/class/leds/<LED>/link_device to create associations - /sys/class/leds/<LED>/unlink_device to remove associations - /sys/class/leds/<LED>/linked_devices/ contains symlinks to all block devices associated with the LED - /sys/block/<DEVICE>/linked_leds (which only exists when the device is associated with at least one LED) contains symlinks to all LEDs with which the device is associated link_device and unlink_device are write-only attributes, each of which represents a single action, rather than any state. (The current state is shown by the symbolic links in the <LED>/linked_devices/ and <DEVICE>/linked_leds/ directories.) * Simplified sysfs attribute store functions. link_device and unlink_device no longer accept multiple devices at once, but this was really just an artifact of the way that sysfs repeatedly calls the store function when it doesn't "consume" all of its input, and it seemed to be confusing and unpopular anyway. * Use DEVICE_ATTR_* macros (rather than __ATTR) for the sysfs attributes. * Removed all pr_info() "system administrator error" messages. * Different minimum values for LED blink time (10 ms) and activity check interval (25 ms). v1 summary: =========== This patch series adds a new "blkdev" LED trigger for disk (or other block device) activity LEDs. It has the following functionality. * Supports all types of block devices, including virtual devices (unlike the existing disk trigger which only works with ATA devices). * LEDs can be configured to show read activity, write activity, or both. * Supports multiple devices and multiple LEDs in arbitrary many-to-many configurations. For example, it is possible to configure multiple devices with device-specific read activity LEDs and a shared write activity LED. (See Documentation/leds/ledtrig-blkdev.rst in the first patch.) * Doesn't add any overhead in the I/O path. Like the netdev LED trigger, it periodically checks the configured devices for activity and blinks its LEDs as appropriate. * Blink duration (per LED) and interval between activity checks (global) are configurable. * Requires minimal changes to the block subsystem. - Adds 1 pointer to struct gendisk, - Adds (inline function) call in device_add_disk() to ensure that the pointer is initialized to NULL (as protection against any drivers that allocate a gendisk themselves and don't use kzalloc()), and - Adds call in del_gendisk() to remove a device from the trigger when that device is being removed. These changes are all in patch #4, "block: Add block device LED trigger integrations." * The trigger can be mostly built as a module. When the trigger is modular, a small portion is built in to provide a "stub" function which can be called from del_gendisk(). The stub calls into the modular code via a function pointer when needed. The trigger also needs the ability to find gendisk's by name, which requires access to the un-exported block_class and disk_type symbols. Ian Pilcher (2): docs: Add block device (blkdev) LED trigger documentation leds: trigger: Add block device LED trigger Documentation/ABI/testing/sysfs-block | 9 + .../testing/sysfs-class-led-trigger-blkdev | 50 + Documentation/leds/index.rst | 1 + Documentation/leds/ledtrig-blkdev.rst | 149 +++ drivers/leds/trigger/Kconfig | 9 + drivers/leds/trigger/Makefile | 1 + drivers/leds/trigger/ledtrig-blkdev.c | 981 ++++++++++++++++++ 7 files changed, 1200 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev create mode 100644 Documentation/leds/ledtrig-blkdev.rst create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c base-commit: ff1ffd71d5f0612cf194f5705c671d6b64bf5f91