From: Syunya Ohshio <[email protected]>
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.
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
};
To: Jonathan Cameron <[email protected]>
Cc: Guido Günther <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Conor Dooley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Syunya Ohshio <[email protected]>
Signed-off-by: Dominique Martinet <[email protected]>
---
Hello! We are facing an issue on one of our device where iio devices
aren't numbered as we'd like in some situations, and I feel like we
could do better than the only alternative I found of making symlinks
directly to /sys in /dev as e.g.
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
Ultimately we'd just like to able to designate a stable path for our
users to use in their application and tell them it won't change even if
we fiddle with the overlays a bit, which is a problem we had as current
init is done in whatever order device tree nodes are processed, and that
in turn depends on how the overlays are applied.
If you can think of a better way of doing it then we'll be happy to
consider something else.
Otherwise aliases seem like it could do a good job, and isn't too
surprising for users - the main downside I can see would be that it
doesn't help platforms without device trees but I honestly don't see
what would work well in a more generic way -- looking at
/sys/bus/iio/devices/iio:deviceX/name to decide what we're looking at
is a bit of a hassle.
Thanks!
.../devicetree/bindings/iio/common.yaml | 9 +++++++--
drivers/iio/industrialio-core.c | 17 ++++++++++++++++-
2 files changed, 23 insertions(+), 3 deletions(-)
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);
--
2.39.2
On 28/02/2024 06:12, Dominique Martinet wrote:
> From: Syunya Ohshio <[email protected]>
>
> 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 <[email protected]>
> >
> > 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,
--
Dominique
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 <[email protected]>
>>>
>>> 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,
--
Dominique
On Wed, 28 Feb 2024 17:11:59 +0900
Dominique Martinet <[email protected]> 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,
--
Dominique
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,
--
Dominique
On Fri, Mar 15, 2024 at 12:58 AM Dominique Martinet
<[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Guido G?nther <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Krzysztof Kozlowski <[email protected]>
> Cc: Conor Dooley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
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 <[email protected]>
> Signed-off-by: Dominique Martinet <[email protected]>
..
> +#include <linux/of.h>
What about ACPI?
Please try avoid hard to use OF-specific code for the new features.
--
With Best Regards,
Andy Shevchenko
Fri, Mar 15, 2024 at 10:53:36AM -0500, David Lechner kirjoitti:
> On Fri, Mar 15, 2024 at 12:58 AM Dominique Martinet
> <[email protected]> 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).
--
With Best Regards,
Andy Shevchenko
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,
--
Dominique
On Mon, 18 Mar 2024 11:15:36 +0900
Dominique Martinet <[email protected]> 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
<[email protected]> 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 <[email protected]> wrote:
> On Mon, 18 Mar 2024 11:15:36 +0900
> Dominique Martinet <[email protected]> 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,
--
Dominique
On Mon, 1 Apr 2024 17:18:31 +0900
Dominique Martinet <[email protected]> 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)
--
Dominique