Seeking help addressing maintainer objections
I have been maintaining an out of tree module for several years, and I'd like to take (yet another) run at getting it accepted. In order to do so, however, I need some advice/help to meet the requirements of the relevant subsystem maintainer. The module in question is an LED trigger for block devices. It allows the system administrator to create relationships between LEDS and block devices, so that activity (reads, writes, etc.) on those devices will cause the LEDs to blink. You can see the latest version (for 6.8 kernels) here[1]. The trigger is very configurable. It supports many-to-many relationships between block devices and LEDS, so a block device can be associated with more than one LED (potentially for different types of activity) and an LED can be associated with one or more block devices. These many-to-many relationships are part of the problem. LED triggers are traditionally configured with sysfs, and sysfs attributes don't really work all that well for configuring a list of things (such as the block devices associated with an LED). In order to avoid a read/modify/write cycle when adding or removing a device/LED association, and adhere to the one value per file guideline, the trigger provides write-only attributes for adding and removing associations. (Existing associations are represented as symbolic links.) The sysfs interface is documented here[2]. The maintainer of the LEDs subsystem considers this interface to be "crazy", but he has not suggested any alternative.[3] (I have not been able to find any existing example of configuring many-to-many relationships via sysfs.) The second problem is the use of device file paths (including symbolic link resolution), rather than kernel names, when creating associations. This limitation exists because the block subsystem does not provide an API to open a block device by its kernel name; only device file paths and device numbers (major & minor) are supported. Adding such an API to the block subsystem has been proposed many times over the years, and it has been denied (often vehemently) on every occasion. I believe it's accurate to say that the maintainers of the block subsystem feel that device file paths (and to a lesser extent device numbers) are *the* correct way to refer to block devices, and they are not willing to add APIs that use kernel names. Does anyone have any suggestions on how I can move forward with this? Thanks! [1] https://github.com/ipilcher/ledtrig-blkdev/blob/pre-6.9/drivers/leds/trigger... [2] https://github.com/ipilcher/ledtrig-blkdev/blob/pre-6.9/Documentation/leds/l... [3] https://lore.kernel.org/lkml/a7ff3338-3d5e-4402-aaba-16e740f4ed5b@gmail.com/... -- ======================================================================== If your user interface is intuitive in retrospect ... it isn't intuitive ========================================================================
Ian Pilcher <arequipeno@gmail.com> writes:
The maintainer of the LEDs subsystem considers this interface to be "crazy", but he has not suggested any alternative.[3]
I read a strong suggestion that one of the unlink interfaces is dropped. You should probably read "crazy" as "unjustified".
(I have not been able to find any existing example of configuring many-to-many relationships via sysfs.)
There's at least one such example, hidden under drivers/usb: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... See the initial commit for justification and some of the background for the choices made: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dr...
The second problem is the use of device file paths (including symbolic link resolution), rather than kernel names, when creating associations. This limitation exists because the block subsystem does not provide an API to open a block device by its kernel name; only device file paths and device numbers (major & minor) are supported.
So this explains why you can't have link_dev_by_path. It still does not explain why you need unlink_dev_by_name. And if file name with symlink resolution really is a problem, then why can't you use the major:minor for link/unlink? That's easy for userspace to look up whether the input is a device path or a sysfs path. And it avoids having to wait for an unrelated and unnecessary device path creation. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dr... Bjørn
On 4/3/24 06:38, Bjørn Mork wrote:
Ian Pilcher <arequipeno@gmail.com> writes:
The maintainer of the LEDs subsystem considers this interface to be "crazy", but he has not suggested any alternative.[3]
I read a strong suggestion that one of the unlink interfaces is dropped.
You should probably read "crazy" as "unjustified".
(I have not been able to find any existing example of configuring many-to-many relationships via sysfs.)
There's at least one such example, hidden under drivers/usb: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
See the initial commit for justification and some of the background for the choices made: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dr...
Thanks! Will take a look at that.
The second problem is the use of device file paths (including symbolic link resolution), rather than kernel names, when creating associations. This limitation exists because the block subsystem does not provide an API to open a block device by its kernel name; only device file paths and device numbers (major & minor) are supported.
So this explains why you can't have link_dev_by_path. It still does not explain why you need unlink_dev_by_name.
It's not absolutely needed, but it does make it easier to unlink an LED from all devices by using the names of the symlinks in the LED's linked_devices directory, which will be kernel names.
And if file name with symlink resolution really is a problem, then why can't you use the major:minor for link/unlink? That's easy for userspace to look up whether the input is a device path or a sysfs path. And it avoids having to wait for an unrelated and unnecessary device path creation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dr...
Personally, I don't think that using file paths is a problem, and it can be useful. ("/dev/vg_root/lv_root" is probably more useful than "dm-0".) OTOH, "sda" is slightly simpler than "/dev/sda", so I think that the ideal situation would be to have both interfaces available. I did propose using device numbers. I never received a response from the maintainer. Thanks for taking the time to respond to this! -- ======================================================================== If your user interface is intuitive in retrospect ... it isn't intuitive ========================================================================
Ian Pilcher <arequipeno@gmail.com> writes:
It's not absolutely needed, but it does make it easier to unlink an LED from all devices by using the names of the symlinks in the LED's linked_devices directory, which will be kernel names.
Yes, I agree that things are much easier if those names can be fed directly into the unlink attribute. And even better if the names in the linked_devices directory actually matched what you used to link them. So why not go for "major:minor" everywhere? I.e for link, unlink and also for the symlinks in linked_devices.
And if file name with symlink resolution really is a problem, then why can't you use the major:minor for link/unlink? That's easy for userspace to look up whether the input is a device path or a sysfs path. And it avoids having to wait for an unrelated and unnecessary device path creation. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dr...
Personally, I don't think that using file paths is a problem, and it can be useful. ("/dev/vg_root/lv_root" is probably more useful than "dm-0".) OTOH, "sda" is slightly simpler than "/dev/sda", so I think that the ideal situation would be to have both interfaces available.
I did propose using device numbers. I never received a response from the maintainer.
I believe that's how most maintainers work unless the proposal was in patch form :-) Bjørn
participants (2)
-
Bjørn Mork -
Ian Pilcher