| Message ID | 20240228051254.3988329-1-dominique.martinet@atmark-techno.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: industrialio-core: look for aliases to request device index | expand |
On 28/02/2024 06:12, Dominique Martinet wrote: > From: Syunya Ohshio <syunya.ohshio@atmark-techno.com> > > When using dtb overlays it can be difficult to predict which iio device > will get assigned what index, and there is no easy way to create > symlinks for /sys nodes through udev so to simplify userspace code make > it possible to request fixed indices for iio devices in device tree. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > > For platforms without device trees of_alias_get_id will just fail and > ida_alloc_range will behave as ida_alloc currently does. > > For platforms with device trees, they can not set an alias, for example > this would try to get 10 from the ida for the device corresponding to > adc2: > aliases { > iio10 = &adc2 > }; Sorry, that's why you have labels and compatibles. Best regards, Krzysztof
Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:16:03AM +0100: > On 28/02/2024 06:12, Dominique Martinet wrote: > > From: Syunya Ohshio <syunya.ohshio@atmark-techno.com> > > > > When using dtb overlays it can be difficult to predict which iio device > > will get assigned what index, and there is no easy way to create > > symlinks for /sys nodes through udev so to simplify userspace code make > > it possible to request fixed indices for iio devices in device tree. > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. Sorry, I assumed that was already the case and didn't think of checking that from what I was given, I'll fix the prefix to "iio: core: .." in v2 > Please run scripts/checkpatch.pl and fix reported warnings. Some > warnings can be ignored, but the code here looks like it needs a fix. > Feel free to get in touch if the warning is not clear. Hm, I did check that and do not get any warning about the code itself: $ git show --format=email | ./scripts/checkpatch.pl -q WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst total: 0 errors, 1 warnings, 61 lines checked What are you thinking of? Regarding the dt binding, I'm not actually changing a binding so I didn't think of rechecking after adding the note, but I guess it still ought to be separate; I'll split it in v2. > > For platforms without device trees of_alias_get_id will just fail and > > ida_alloc_range will behave as ida_alloc currently does. > > > > For platforms with device trees, they can not set an alias, for example > > this would try to get 10 from the ida for the device corresponding to > > adc2: > > aliases { > > iio10 = &adc2 > > }; > > Sorry, that's why you have labels and compatibles. I'm not sure I understand this comment -- would you rather this doesn't use aliases but instead add a new label (e.g. `iio,index = <10>` or whatever) to the iio node itself? Setting up a fixed alias seems to be precisely what aliases are about (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label would be more appropriate here, but perhaps I'm missing some context? Thanks,
On 28/02/2024 08:31, Dominique Martinet wrote: > Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:16:03AM +0100: >> On 28/02/2024 06:12, Dominique Martinet wrote: >>> From: Syunya Ohshio <syunya.ohshio@atmark-techno.com> >>> >>> When using dtb overlays it can be difficult to predict which iio device >>> will get assigned what index, and there is no easy way to create >>> symlinks for /sys nodes through udev so to simplify userspace code make >>> it possible to request fixed indices for iio devices in device tree. >> >> Please use subject prefixes matching the subsystem. You can get them for >> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >> your patch is touching. > > Sorry, I assumed that was already the case and didn't think of checking > that from what I was given, I'll fix the prefix to "iio: core: .." in v2 > >> Please run scripts/checkpatch.pl and fix reported warnings. Some >> warnings can be ignored, but the code here looks like it needs a fix. >> Feel free to get in touch if the warning is not clear. > > Hm, I did check that and do not get any warning about the code itself: > > $ git show --format=email | ./scripts/checkpatch.pl -q > WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst > > total: 0 errors, 1 warnings, 61 lines checked > > What are you thinking of? You have warning right there. > > Regarding the dt binding, I'm not actually changing a binding so I > didn't think of rechecking after adding the note, but I guess it still > ought to be separate; I'll split it in v2. > >>> For platforms without device trees of_alias_get_id will just fail and >>> ida_alloc_range will behave as ida_alloc currently does. >>> >>> For platforms with device trees, they can not set an alias, for example >>> this would try to get 10 from the ida for the device corresponding to >>> adc2: >>> aliases { >>> iio10 = &adc2 >>> }; >> >> Sorry, that's why you have labels and compatibles. > > I'm not sure I understand this comment -- would you rather this doesn't > use aliases but instead add a new label (e.g. `iio,index = <10>` or > whatever) to the iio node itself? No, the devices already have label property. > > Setting up a fixed alias seems to be precisely what aliases are about > (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with > ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label > would be more appropriate here, but perhaps I'm missing some context? Maybe I don't get your point, but your email said "sysfs", so why do you refer to /dev? Best regards, Krzysztof
Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:42:46AM +0100: > >> Sorry, that's why you have labels and compatibles. > > > Setting up a fixed alias seems to be precisely what aliases are about > > (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with > > ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label > > would be more appropriate here, but perhaps I'm missing some context? > > Maybe I don't get your point, but your email said "sysfs", so why do you > refer to /dev? I wrote /dev/rtc0, but it also sets the name in /sys, right? For example /sys/class/rtc/rtc0 As far as I'm aware iio also creates character devices in /dev with the same name (/dev/iio/iio:deviceX), but our application doesn't use these at all and has to? look in /sys directly, so normal udev SYMLINK+= unfortunately isn't applicable or I wouldn't be bothering with all this.. > > I'm not sure I understand this comment -- would you rather this doesn't > > use aliases but instead add a new label (e.g. `iio,index = <10>` or > > whatever) to the iio node itself? > > No, the devices already have label property. Thank you for pointing me at the 'label' property, looking at other subsystems e.g. leds I see paths in sysfs that use labels as I'd like it to work for iio (/sys/class/leds/<label> and /sys/devices/platform/<parent>/leds/<label>) Unfortunately for iio it looks like labels isn't ignored, but instead create a file in the sysfs directory of the device, e.g. I now have /sys/bus/iio/devices/iio:device1/label which contains the label string, so I'm not sure that can be changed easily as that'd be a change of API for existing users for labels in iio devices? (I checked briefly and didn't find any, but there seems to be an awful lot of code in the iio drivers tree about labels so I'm not really comfortable changing that without some more background on iio first... Jonathan perhaps has an opinion on this?) Thanks,
On Wed, 28 Feb 2024 17:11:59 +0900 Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:42:46AM +0100: > > >> Sorry, that's why you have labels and compatibles. > > > > > Setting up a fixed alias seems to be precisely what aliases are about > > > (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with > > > ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label > > > would be more appropriate here, but perhaps I'm missing some context? > > > > Maybe I don't get your point, but your email said "sysfs", so why do you > > refer to /dev? > > I wrote /dev/rtc0, but it also sets the name in /sys, right? > For example /sys/class/rtc/rtc0 > > As far as I'm aware iio also creates character devices in /dev with the > same name (/dev/iio/iio:deviceX), but our application doesn't use these > at all and has to? look in /sys directly, so normal udev SYMLINK+= > unfortunately isn't applicable or I wouldn't be bothering with all > this.. A given IIO device driver may create multiple sysfs directories (registers device + one or more triggers), so I'm not sure how this would work. > > > > I'm not sure I understand this comment -- would you rather this doesn't > > > use aliases but instead add a new label (e.g. `iio,index = <10>` or > > > whatever) to the iio node itself? > > > > No, the devices already have label property. > > Thank you for pointing me at the 'label' property, looking at other > subsystems e.g. leds I see paths in sysfs that use labels as I'd like it > to work for iio (/sys/class/leds/<label> and > /sys/devices/platform/<parent>/leds/<label>) > > Unfortunately for iio it looks like labels isn't ignored, but instead > create a file in the sysfs directory of the device, e.g. I now have > /sys/bus/iio/devices/iio:device1/label which contains the label string, > so I'm not sure that can be changed easily as that'd be a change of API > for existing users for labels in iio devices? Yes, don't touch that ABI. IIO software assumes naming of iio\:deviceX etc. > > (I checked briefly and didn't find any, but there seems to be an awful > lot of code in the iio drivers tree about labels so I'm not really > comfortable changing that without some more background on iio first... > Jonathan perhaps has an opinion on this?) There are labels for channels as well as devices, but the short description you have above is it. I don't see why that isn't sufficient for your use case though? What does a directory name matter when you can write a few lines of code to retrieve the IIO device that you want. If this was day 0 maybe we could support renaming devices like this but we have a long history of those names not being stable and label + parentage of the IIO devices being used to establish stable identification. Anything beyond a trivial single use script is going to need to deal with not having stable names (old kernel, dt without alias etc) so this doesn't help in general. Jonathan > > > Thanks,
Thank you for the quick reply, Jonathan Cameron wrote on Wed, Feb 28, 2024 at 02:24:41PM +0000: > > > Maybe I don't get your point, but your email said "sysfs", so why do you > > > refer to /dev? > > > > I wrote /dev/rtc0, but it also sets the name in /sys, right? > > For example /sys/class/rtc/rtc0 > > > > As far as I'm aware iio also creates character devices in /dev with the > > same name (/dev/iio/iio:deviceX), but our application doesn't use these > > at all and has to? look in /sys directly, so normal udev SYMLINK+= > > unfortunately isn't applicable or I wouldn't be bothering with all > > this.. > > A given IIO device driver may create multiple sysfs directories (registers > device + one or more triggers), so I'm not sure how this would work. Thanks for pointing this out, the driver I'm using doesn't seem to create extra triggers (iio_trigger_alloc doesn't seem to be called), but the current patch would only affect iio_device_register, so presumably would have no impact for these extra directories. (There's also no impact without dt changes, it's only adding an extra way of fixing the X of iio:deviceX everywhere) > > Unfortunately for iio it looks like labels isn't ignored, but instead > > create a file in the sysfs directory of the device, e.g. I now have > > /sys/bus/iio/devices/iio:device1/label which contains the label string, > > so I'm not sure that can be changed easily as that'd be a change of API > > for existing users for labels in iio devices? > > Yes, don't touch that ABI. IIO software assumes naming of > iio\:deviceX etc. > > > (I checked briefly and didn't find any, but there seems to be an awful > > lot of code in the iio drivers tree about labels so I'm not really > > comfortable changing that without some more background on iio first... > > Jonathan perhaps has an opinion on this?) > > There are labels for channels as well as devices, but the short description > you have above is it. > > I don't see why that isn't sufficient for your use case though? I'll have a lot of trouble arguing that one out as I agree it's "not that hard" to check the names to get the correct IIO device, but it's still definitely more work than being able to use fixed names. For more background, we're selling a device+platform where our users can write code themselves to read the sensors, with a variable number of sensors (extension cards can be plugged in offline, reboot and you get one more). Adding an extra device currently comes in first and renames all pre-existing ones because that's apparently the order linux gets them from the dtb after adding overlays, and it'd "look better" if devices get added in fixed order so our users don't need to deal with the checking names/labels logic. toradex apparently has the same need because they provide a script that crates ugly links from /dev/xxx-adcY to /sys/.../in_voltageY_raw, so it's not like we're the first ones to want something like this; I think however that udev isn't appropriate to create links to /sys, so having some way of fixing names in dts sounds better to me. > What does a directory name matter when you can write a few lines of > code to retrieve the IIO device that you want. > > If this was day 0 maybe we could support renaming devices like this > but we have a long history of those names not being stable and label > + parentage of the IIO devices being used to establish stable identification. I'm sure we can make something work out while preserving compatibility, the patch I sent might not be great but it wouldn't bother anyone not using said aliases. aliases are apparently not appropriate for this (still not sure why?), but if for example labels are better we could keep the current iio:deviceX path (/sys/bus/iio/devices/iio:device0) with a label file in it as current software expect, but add a brand new directory with a link named as per the label itself (for example /sys/class/iio/<label>; my understanding is that /sys/class is meant for "easier" links and we don't have anything iio-related there yet) That wouldn't break users checking through the existing paths, and provide a new fixed path for anyone looking for it. > Anything beyond a trivial single use script is going to need to deal with > not having stable names (old kernel, dt without alias etc) so this doesn't > help in general. I'm sure any major software will have to deal with old kernels, but I disagree that it doesn't help: in our case described above our users are basically writing trivial scripts and won't ever be downgrading, we want to guarantee they can rely on fixed names for our hardware because I know for sure most won't be bothering to check. Even for major softwares, any feature written now will hopefully be considered generally available 20 years from now, we need to start somewhere. Thanks,
Hi Jonathan, Dominique Martinet wrote on Thu, Feb 29, 2024 at 11:59:19AM +0900: > Jonathan Cameron wrote on Wed, Feb 28, 2024 at 02:24:41PM +0000: > > A given IIO device driver may create multiple sysfs directories (registers > > device + one or more triggers), so I'm not sure how this would work. > > Thanks for pointing this out, the driver I'm using doesn't seem to > create extra triggers (iio_trigger_alloc doesn't seem to be called), but > the current patch would only affect iio_device_register, so presumably > would have no impact for these extra directories. So my device doesn't have any "built-in" trigger if that's a thing (let alone multiple), but I've played with iio-trig-sysfs and also had a look at what's in the tree's dts, and as far as I can see the 'name' (/sys/bus/iio/devices/trigger*/name, also used when registering a trigger for a device) seems to be fixed by the driver with parameters of the dts (e.g. 'reg'), so if there are multiple triggers and one wants something in the triggerX directory they're supposed to check all the names? So as far as I can see, I keep thinking it's orthogonal: - devices get a link as /sys/bus/iio/devices/iio:deviceX ; which contains: * 'name', set by driver (some have an index but many are constant), and does not have to be unique, * 'label' contains whatever was set as label if set * 'of_node', a symlink to the device tree node which is what we currently use to differentiate devices in our code - triggers get /sys/bus/iio/devices/triggerX, which has a 'name' file that probably must be unique (as it's can be written in device's trigger/current_trigger to select it) > I'm sure we can make something work out while preserving compatibility, > the patch I sent might not be great but it wouldn't bother anyone not > using said aliases. > > aliases are apparently not appropriate for this (still not sure why?), > but if for example labels are better we could keep the current > iio:deviceX path (/sys/bus/iio/devices/iio:device0) with a label file in > it as current software expect, but add a brand new directory with a link > named as per the label itself (for example /sys/class/iio/<label>; > my understanding is that /sys/class is meant for "easier" links and we > don't have anything iio-related there yet) I've looked at this /sys/class/iio idea (could use the label or fallback to iio:deviceX for devices, and name for triggers), but /sys/class seems to be entierly managed by the linux core driver framework so that doesn't leave much room for compromise... The links there use the device name (so iio:deviceX for devices), and if creating such a link fails it'll also fail the whole device creation (cdev_device_add() -> device_add() -> device_add_class_symlinks()), so my evil plan is foiled. (/sys/bus/iio/devices links are also automatically created by device_add() -> bus_add_device() from the device name) I guess we could manage another new directory somewhere or haphazardly create extra redundant links in the current bus directory, but that's not exactly something I'd consider workable given there is no possible deprecation path down the road, so ultimately I still think the aliases patch I sent is amongst the least bad options we have here: - there's currently no alias for iio so it won't break anything; - even if one adds some on a device with multiple iio devices all that can happen is some indices will be shuffled, but paths will still be compatible with all current applications. Did you have time to think about this or another possible way forward? Thanks,
On Fri, Mar 15, 2024 at 12:58 AM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Hi Jonathan, > > Dominique Martinet wrote on Thu, Feb 29, 2024 at 11:59:19AM +0900: > > Jonathan Cameron wrote on Wed, Feb 28, 2024 at 02:24:41PM +0000: > > > A given IIO device driver may create multiple sysfs directories (registers > > > device + one or more triggers), so I'm not sure how this would work. > > > > Thanks for pointing this out, the driver I'm using doesn't seem to > > create extra triggers (iio_trigger_alloc doesn't seem to be called), but > > the current patch would only affect iio_device_register, so presumably > > would have no impact for these extra directories. > > So my device doesn't have any "built-in" trigger if that's a thing (let > alone multiple), but I've played with iio-trig-sysfs and also had a look > at what's in the tree's dts, and as far as I can see the 'name' > (/sys/bus/iio/devices/trigger*/name, also used when registering a > trigger for a device) seems to be fixed by the driver with parameters of > the dts (e.g. 'reg'), so if there are multiple triggers and one wants > something in the triggerX directory they're supposed to check all the > names? > > > So as far as I can see, I keep thinking it's orthogonal: > - devices get a link as /sys/bus/iio/devices/iio:deviceX ; which contains: > * 'name', set by driver (some have an index but many are constant), and > does not have to be unique, > * 'label' contains whatever was set as label if set > * 'of_node', a symlink to the device tree node which is what we > currently use to differentiate devices in our code > - triggers get /sys/bus/iio/devices/triggerX, which has a 'name' file > that probably must be unique (as it's can be written in device's > trigger/current_trigger to select it) > > > I'm sure we can make something work out while preserving compatibility, > > the patch I sent might not be great but it wouldn't bother anyone not > > using said aliases. > > > > aliases are apparently not appropriate for this (still not sure why?), > > but if for example labels are better we could keep the current > > iio:deviceX path (/sys/bus/iio/devices/iio:device0) with a label file in > > it as current software expect, but add a brand new directory with a link > > named as per the label itself (for example /sys/class/iio/<label>; > > my understanding is that /sys/class is meant for "easier" links and we > > don't have anything iio-related there yet) > > I've looked at this /sys/class/iio idea (could use the label or fallback > to iio:deviceX for devices, and name for triggers), but /sys/class seems > to be entierly managed by the linux core driver framework so that > doesn't leave much room for compromise... > The links there use the device name (so iio:deviceX for devices), and if > creating such a link fails it'll also fail the whole device creation > (cdev_device_add() -> device_add() -> device_add_class_symlinks()), so > my evil plan is foiled. (/sys/bus/iio/devices links are also > automatically created by device_add() -> bus_add_device() from the > device name) > > > I guess we could manage another new directory somewhere or haphazardly > create extra redundant links in the current bus directory, but that's > not exactly something I'd consider workable given there is no possible > deprecation path down the road, so ultimately I still think the aliases > patch I sent is amongst the least bad options we have here: > - there's currently no alias for iio so it won't break anything; > - even if one adds some on a device with multiple iio devices all that > can happen is some indices will be shuffled, but paths will still be > compatible with all current applications. > > > Did you have time to think about this or another possible way forward? > How about using udev rules to create symlinks for each device based on the label attribute? No changes to the kernel are needed.
Wed, Feb 28, 2024 at 02:12:54PM +0900, Dominique Martinet kirjoitti: > From: Syunya Ohshio <syunya.ohshio@atmark-techno.com> > > When using dtb overlays it can be difficult to predict which iio device DTB IIO > will get assigned what index, and there is no easy way to create > symlinks for /sys nodes through udev so to simplify userspace code make > it possible to request fixed indices for iio devices in device tree. IIO > > For platforms without device trees of_alias_get_id will just fail and We refer to functions as func(): of_alias_get_id() > ida_alloc_range will behave as ida_alloc currently does. Ditto. > For platforms with device trees, they can not set an alias, for example > this would try to get 10 from the ida for the device corresponding to > adc2: > aliases { > iio10 = &adc2 > }; > To: Jonathan Cameron <jic23@kernel.org> > Cc: Guido Günther <agx@sigxcpu.org> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > Cc: Conor Dooley <conor+dt@kernel.org> > Cc: linux-iio@vger.kernel.org > Cc: devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org Please, make sure you don't pollute commit message with these. Either use specific --to and --cc when formating patch or move them into comment block (after '---' line below). > Signed-off-by: Syunya Ohshio <syunya.ohshio@atmark-techno.com> > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> ... > +#include <linux/of.h> What about ACPI? Please try avoid hard to use OF-specific code for the new features.
Fri, Mar 15, 2024 at 10:53:36AM -0500, David Lechner kirjoitti: > On Fri, Mar 15, 2024 at 12:58 AM Dominique Martinet > <dominique.martinet@atmark-techno.com> wrote: ... > How about using udev rules to create symlinks for each device based on > the label attribute? No changes to the kernel are needed. +1 for the suggestion. We have a lot of information in sysfs to distinguish devices by a physical connection, even if we have the same names/labels they may not have the same physical connections (if there is a possibility of the collision, then it means it has to be fixed elsewhere).
David Lechner wrote on Fri, Mar 15, 2024 at 10:53:36AM -0500: > How about using udev rules to create symlinks for each device based on > the label attribute? No changes to the kernel are needed. Right, it's definitely possible to make symlinks for each "device" -- my patch comment links to such an udev script "solution": https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/toradex-adc.sh?h=kirkstone-6.x.y (the script is launched by udev here: https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/99-toradex.rules ) My conceptual problem with this is that this makes symlinks in /dev to files in /sys and it feels like we're crossing boundaries. As far as I can tell there is no way for userspace to create arbitrary symlinks in /sys, so I think we could have an interface more user-friendly by allowing paths to be static for users with multiple devices. (I guess that's a weak argument given e.g. disks etc will also have an unreliable name in /sys in the general case, but simple programs don't interact with them in /sys and can use stable links in /dev so my expectations here aren't quite the same) Ultimately, the problem might run deeper in that we're having userspace interact with the device through /sys and not the /dev char dev... As far as I could see /dev/iio:deviceX only allows reading buffered values and doesn't have any ioctl or other way of reading immediate values as is possible in /sys though, so that'd require quite a bit of work to duplicate the interface there... Perhaps I'm just thinking too much and symlinks from /dev to /sys are a thing in the IIO world? I've not seen it done anywhere except in that toradex tree when I was looking earlier. Andy Shevchenko wrote on Sat, Mar 16, 2024 at 10:14:35PM +0200: > [...] Thank you for the review! >> +#include <linux/of.h> > > What about ACPI? > Please try avoid hard to use OF-specific code for the new features. Given my suggestion here relied on users giving manual hints in the DTB I'm not sure how that could be interfaced with ACPI, but if you have a suggestion to make paths static that'd work with either interfaces I'd be more than happy to give it a try. I'd also like to add that in my particular case it's a problem created by the OF interface in the first place: devices are currently created in the order they're parsed from OF, and it just so happens that this order doesn't work well for us; I'm not aware of how IIO interacts with ACPI but perhaps the way the list of devices processed from ACPI is "stable enough" in practice? Thank you,
On Mon, 18 Mar 2024 11:15:36 +0900 Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > David Lechner wrote on Fri, Mar 15, 2024 at 10:53:36AM -0500: > > How about using udev rules to create symlinks for each device based on > > the label attribute? No changes to the kernel are needed. > > Right, it's definitely possible to make symlinks for each "device" -- my > patch comment links to such an udev script "solution": > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/toradex-adc.sh?h=kirkstone-6.x.y > (the script is launched by udev here: > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/99-toradex.rules > ) > > My conceptual problem with this is that this makes symlinks in /dev to > files in /sys and it feels like we're crossing boundaries. > As far as I can tell there is no way for userspace to create arbitrary > symlinks in /sys, so I think we could have an interface more > user-friendly by allowing paths to be static for users with multiple > devices. > (I guess that's a weak argument given e.g. disks etc will also have an > unreliable name in /sys in the general case, but simple programs don't > interact with them in /sys and can use stable links in /dev so my > expectations here aren't quite the same) > > > Ultimately, the problem might run deeper in that we're having userspace > interact with the device through /sys and not the /dev char dev... As > far as I could see /dev/iio:deviceX only allows reading buffered values > and doesn't have any ioctl or other way of reading immediate values as > is possible in /sys though, so that'd require quite a bit of work to > duplicate the interface there... Don't. The sysfs interface as only control is entirely intentional and we do not want IOCTL based duplication. Just addressing this bit as I'm still a bit snowed under to think about this more generally.
On Sun, Mar 17, 2024 at 9:15 PM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > David Lechner wrote on Fri, Mar 15, 2024 at 10:53:36AM -0500: > > How about using udev rules to create symlinks for each device based on > > the label attribute? No changes to the kernel are needed. > > Right, it's definitely possible to make symlinks for each "device" -- my > patch comment links to such an udev script "solution": > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/toradex-adc.sh?h=kirkstone-6.x.y > (the script is launched by udev here: > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/99-toradex.rules > ) > > My conceptual problem with this is that this makes symlinks in /dev to > files in /sys and it feels like we're crossing boundaries. > As far as I can tell there is no way for userspace to create arbitrary > symlinks in /sys, so I think we could have an interface more > user-friendly by allowing paths to be static for users with multiple > devices. How about this: Use udev to create one symlink from /dev/frendly-name to /dev/iio:deviceX. Then in your application, look up the device by /dev/friendly-name. Then resolve the symlink to translate friendly-name to iio:deviceX. Now your app has the correct iio:deviceX and can use /sys/bus/iio/devices/iio:deviceX/ directly instead of making any symlinks to sysfs.
On Mon, 18 Mar 2024 12:29:53 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 18 Mar 2024 11:15:36 +0900 > Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > > David Lechner wrote on Fri, Mar 15, 2024 at 10:53:36AM -0500: > > > How about using udev rules to create symlinks for each device based on > > > the label attribute? No changes to the kernel are needed. > > > > Right, it's definitely possible to make symlinks for each "device" -- my > > patch comment links to such an udev script "solution": > > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/toradex-adc.sh?h=kirkstone-6.x.y > > (the script is launched by udev here: > > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/99-toradex.rules > > ) > > > > My conceptual problem with this is that this makes symlinks in /dev to > > files in /sys and it feels like we're crossing boundaries. > > As far as I can tell there is no way for userspace to create arbitrary > > symlinks in /sys, so I think we could have an interface more > > user-friendly by allowing paths to be static for users with multiple > > devices. > > (I guess that's a weak argument given e.g. disks etc will also have an > > unreliable name in /sys in the general case, but simple programs don't > > interact with them in /sys and can use stable links in /dev so my > > expectations here aren't quite the same) > > > > > > Ultimately, the problem might run deeper in that we're having userspace > > interact with the device through /sys and not the /dev char dev... As > > far as I could see /dev/iio:deviceX only allows reading buffered values > > and doesn't have any ioctl or other way of reading immediate values as > > is possible in /sys though, so that'd require quite a bit of work to > > duplicate the interface there... > > Don't. The sysfs interface as only control is entirely intentional and > we do not want IOCTL based duplication. > > Just addressing this bit as I'm still a bit snowed under to think about this > more generally. > Hi, got back to this finally... So the problems compared to other 'alias' users is that IIO is a bit more complex than for example LEDs. A single DT node/compatible (or equivalent) can result in a 1+ IIO devices and 1+ triggers. Triggers can also be instantiated via configfs (technically devices can as well but we can ignore that). Any alias scheme needs to work for all these options. To my mind that makes it a userspace problem, not something the kernel can deal with in generic enough way. I think that all IIO devices have ways to stability identify them (label, or parent devices) There is an approximate equivalent of DT alias entries in SMBIOS but I suspect not all ACPI platforms will provide that (it's typically used for stable disk / network device naming on complex servers). So I've messed around a bit and can think of various possible options to make this simpler. 1) Use a tmpfs mount and link from that. Now we 'could' put an alias directory somewhere under /sys/bus/iio/ that is a mount point created via sysfs_create_mount_point() - I abused the /sys/kernel/debug directory to test this (unmounted debugfs and mounted a tmpfs). That would provide somewhere in sysfs that allows suitable links. However, this is unusual so likely to be controversial. 2) Alternatively the relevant platform could create one of these somewhere outside of sysfs and use udev rules to create the links. 3) Stick to the oddity of doing it under /dev/ 4) Access the things in the first place via more stable paths? /sys/bus/i2c/devices/i2c-0/0-0008/iio\:device?/ etc Relying on the alias support for i2c bus numbering to make that stable should work and if you are sure there will only be one entry (most devices) that matches the wild card, should be easy enough to use in scripts. My personal preference is this last option. Basically if you want stable paths don't use /sys/bus/iio/devices/ to get them. Jonathan
Jonathan Cameron wrote on Sun, Mar 31, 2024 at 03:20:42PM +0100: > Hi, got back to this finally... Thank you for taking the time to express your thoughts! > So the problems compared to other 'alias' users is that IIO is a bit more > complex than for example LEDs. A single DT node/compatible (or equivalent) can > result in a 1+ IIO devices and 1+ triggers. Right. I'm no longer really arguing for it at this point, but for comparison in the patch I sent, the alias sets the start of the idr for the device index, so if you have a driver that creates two IIO devices you could just "reserve" two for this DT node and assuming the order within this node is constant you'd still get constant numbering, so I think it still somewhat holds up here. For triggers though the numbers are separate and it wouldn't make sense to use the same alias, if we wanted a coherent design with this we'd need to add a second alias (such as iio_trigger = ..), but that makes much less sense to me given they're also likely to be dynamically instancied via configfs from what I've seen; I wouldn't want to do this kind of mapping, so I agree with you. > So I've messed around a bit and can think of various possible options to make > this simpler. > 1) Use a tmpfs mount and link from that. > Now we 'could' put an alias directory somewhere under /sys/bus/iio/ that > is a mount point created via sysfs_create_mount_point() - I abused the > /sys/kernel/debug directory to test this (unmounted debugfs and mounted > a tmpfs). That would provide somewhere in sysfs that allows suitable > links. However, this is unusual so likely to be controversial. Agreed that's probably not something we want to put our hands into. > 2) Alternatively the relevant platform could create one of these somewhere > outside of sysfs and use udev rules to create the links. I'm not sure I understood this one, something akin to the udev rules I've showed that made links to the /sys iio device in /dev? "relevant platform" here would be vendors? > 3) Stick to the oddity of doing it under /dev/ > 4) Access the things in the first place via more stable paths? > /sys/bus/i2c/devices/i2c-0/0-0008/iio\:device?/ etc > Relying on the alias support for i2c bus numbering to make that stable should work > and if you are sure there will only be one entry (most devices) that matches > the wild card, should be easy enough to use in scripts. > > My personal preference is this last option. Basically if you want stable paths > don't use /sys/bus/iio/devices/ to get them. Hmm, I wouldn't call that path stable given the '?' portion can change, but at least that certainly is a single glob away so it's definitely simpler than checking every labels all the time. My second nitpick with this is that while these paths are stable for a given kernel version, but we've had some paths changes over many years (not sure if it was 3.14 or 4.9 but one of these perhaps didn't have /sys/devices/platform yet? and things got moved there at some point with some subtle name changes, breaking a couple of scripts). OTOH /sys/bus/iio/devices feels less likely to change, but I guess this is something that'd come up on tests when preparing a new kernel anyway, so this is probably acceptable; I'm just thinking out loud. With that said I can't think of anything that'd work everywhere either, so I guess we can stick to the status-quo and I'll rediscuss what we want to do with coworkers. Thanks,
On Mon, 1 Apr 2024 17:18:31 +0900 Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > Jonathan Cameron wrote on Sun, Mar 31, 2024 at 03:20:42PM +0100: > > Hi, got back to this finally... > > Thank you for taking the time to express your thoughts! > > > So the problems compared to other 'alias' users is that IIO is a bit more > > complex than for example LEDs. A single DT node/compatible (or equivalent) can > > result in a 1+ IIO devices and 1+ triggers. > > Right. I'm no longer really arguing for it at this point, but for > comparison in the patch I sent, the alias sets the start of the idr for > the device index, so if you have a driver that creates two IIO devices > you could just "reserve" two for this DT node and assuming the order > within this node is constant you'd still get constant numbering, so I > think it still somewhat holds up here. > > For triggers though the numbers are separate and it wouldn't make sense > to use the same alias, if we wanted a coherent design with this we'd > need to add a second alias (such as iio_trigger = ..), but that makes > much less sense to me given they're also likely to be dynamically > instancied via configfs from what I've seen; I wouldn't want to do this > kind of mapping, so I agree with you. > > > So I've messed around a bit and can think of various possible options to make > > this simpler. > > 1) Use a tmpfs mount and link from that. > > Now we 'could' put an alias directory somewhere under /sys/bus/iio/ that > > is a mount point created via sysfs_create_mount_point() - I abused the > > /sys/kernel/debug directory to test this (unmounted debugfs and mounted > > a tmpfs). That would provide somewhere in sysfs that allows suitable > > links. However, this is unusual so likely to be controversial. > > Agreed that's probably not something we want to put our hands into. > > > 2) Alternatively the relevant platform could create one of these somewhere > > outside of sysfs and use udev rules to create the links. > > I'm not sure I understood this one, something akin to the udev rules > I've showed that made links to the /sys iio device in /dev? > "relevant platform" here would be vendors? Yes. Just somewhere other than /dev because I agree that feels wrong. > > > 3) Stick to the oddity of doing it under /dev/ > > 4) Access the things in the first place via more stable paths? > > /sys/bus/i2c/devices/i2c-0/0-0008/iio\:device?/ etc > > Relying on the alias support for i2c bus numbering to make that stable should work > > and if you are sure there will only be one entry (most devices) that matches > > the wild card, should be easy enough to use in scripts. > > > > My personal preference is this last option. Basically if you want stable paths > > don't use /sys/bus/iio/devices/ to get them. > > Hmm, I wouldn't call that path stable given the '?' portion can change, > but at least that certainly is a single glob away so it's definitely > simpler than checking every labels all the time. Agreed - this does rely on that wildcard. > > My second nitpick with this is that while these paths are stable for a > given kernel version, but we've had some paths changes over many years > (not sure if it was 3.14 or 4.9 but one of these perhaps didn't have > /sys/devices/platform yet? and things got moved there at some point with > some subtle name changes, breaking a couple of scripts). > OTOH /sys/bus/iio/devices feels less likely to change, but I guess this > is something that'd come up on tests when preparing a new kernel anyway, > so this is probably acceptable; I'm just thinking out loud. Agreed they have changed in the past. Mostly a case of fixing up devices that didn't have their parents set (there are lots of those left - that reminds me that I was cleaning up perf drivers that do this wrong last year and only got part way through it :( We will keep the /sys/bus/iio/devices/ path the same and continue to support all paths below there (there are already compatibility links in place for buffers for example from when we added multiple buffer support). However, sysfs docs are pretty blunt on not relying on reliable paths for classes (and the IIO bus is effectively a class from this point of view). > > > With that said I can't think of anything that'd work everywhere either, > so I guess we can stick to the status-quo and I'll rediscuss what we > want to do with coworkers. Good luck. If you have time it might be good to hear what you end up with! Thanks, Jonathan > > > Thanks,
Jonathan Cameron wrote on Mon, Apr 01, 2024 at 05:47:56PM +0100: > Good luck. If you have time it might be good to hear what you end up > with! Just a quick follow-up since you asked -- given we manage our own kernel that already has its share of patches and it's not something user-visible we'll stick with the aliases approach for this kernel to make identifiers static. (and I'm adding labels for meticulous users, but not expecting it to be used in practice, it'll mostly be used in automated testing to make sure the number doesn't change on our end) The rationale was as per my previous mails that paths in /sys/devices/platform have changed in the past so we'd rather not rely on these being set in stone, and while a new symlink would have been workable it's a user-noticeable change so we've prefered just pinning the device numbers. I'm always reluctant to take in more "in house" patches in our tree but in this case it's "simple enough" (death by thousands paper cut?), and we'll rediscuss this if/when another upstream solution shows up. Thanks a lot for your time thinking it through and discussing it though, that was appreciated! (Jonathan and everyone else involved)
diff --git a/Documentation/devicetree/bindings/iio/common.yaml b/Documentation/devicetree/bindings/iio/common.yaml index b3a10af86d76..23d4c3012aeb 100644 --- a/Documentation/devicetree/bindings/iio/common.yaml +++ b/Documentation/devicetree/bindings/iio/common.yaml @@ -12,13 +12,18 @@ maintainers: description: | This document defines device tree properties common to several iio - sensors. It doesn't constitute a device tree binding specification by itself but - is meant to be referenced by device tree bindings. + sensors. It doesn't constitute a device tree binding specification by itself + but is meant to be referenced by device tree bindings. When referenced from sensor tree bindings the properties defined in this document are defined as follows. The sensor tree bindings are responsible for defining whether each property is required or optional. + Note: it is also possible to request an index for the iio device through the + "aliases" device tree node. It is however only used as a hint so care should + be taken to either set all devices, or set indices in a range that will not + be used by devices without aliases. + properties: proximity-near-level: $ref: /schemas/types.yaml#/definitions/uint32 diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 173dc00762a1..0f088be3a48c 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -20,6 +20,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of.h> #include <linux/poll.h> #include <linux/property.h> #include <linux/sched.h> @@ -1644,6 +1645,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) struct iio_dev_opaque *iio_dev_opaque; struct iio_dev *indio_dev; size_t alloc_size; + int iio_dev_id; alloc_size = sizeof(struct iio_dev_opaque); if (sizeof_priv) { @@ -1667,7 +1669,10 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) mutex_init(&iio_dev_opaque->info_exist_lock); INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list); - iio_dev_opaque->id = ida_alloc(&iio_ida, GFP_KERNEL); + iio_dev_id = of_alias_get_id(parent->of_node, "iio"); + iio_dev_opaque->id = ida_alloc_range(&iio_ida, + iio_dev_id < 0 ? 0 : iio_dev_id, + ~0, GFP_KERNEL); if (iio_dev_opaque->id < 0) { /* cannot use a dev_err as the name isn't available */ pr_err("failed to get device id\n"); @@ -1681,6 +1686,16 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) return NULL; } + /* log about iio_dev_id after dev_set_name() for dev_* helpers */ + if (iio_dev_id < 0) { + dev_dbg(&indio_dev->dev, + "No aliases in fw node for device: %d\n", iio_dev_id); + } else if (iio_dev_opaque->id != iio_dev_id) { + dev_warn(&indio_dev->dev, + "Device requested %d in fw node but could not get it\n", + iio_dev_id); + } + INIT_LIST_HEAD(&iio_dev_opaque->buffer_list); INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);