IIO trigger names are not actually unique

IIO trigger names are not actually unique

[Crestez Dan Leonard]

Hello,

I've been looking at trigger code and it seems to me that trigger->name
is not guaranteed to be unique. This is despite the fact that it's
documented as "@name: [DRIVER] unique name" on top of struct
iio_trigger. Just like indio_dev->name it seems to be a mostly cosmetic
label.

You can easily create a software trigger with a duplicate name if you
enable CONFIG_IIO_HRTIMER_TRIGGER:

mkdir /sys/kernel/config/iio/triggers/hrtimer/`cat
/sys/bus/iio/devices/trigger0/name`

It seems that trigger names are set at allocation time, mostly through
some variant of:

    iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);

indio_dev->name is not guaranteed to be unique but indio_dev->id is. The
are some exceptions, including st_sensors which does:

    iio_trigger_alloc("%s-trigger", indio_dev->name);

Since indio_dev->name is just the model name it seems that attaching two
identical st_sensors devices will create two triggers with the same name.

The "current_trigger" for an iio_device is identified by name. If you
have two triggers with the same name then the "first one" will be picked
up, probably in registration order.

One solution would be to refuse to register triggers with duplicate
names but that might cause probe errors in some cases.

Another option would be to just warn on duplicate names and provide a
separate current_trigger_id which identifies the current trigger by
numeric ID. In general it seems to me that it's better to identify
entities by numbers rather than strings.

-- 
Regards,
Leonard

Re: IIO trigger names are not actually unique

[Lars-Peter Clausen]

On 05/05/2016 01:52 PM, Crestez Dan Leonard wrote:
> Hello,
> 
> I've been looking at trigger code and it seems to me that trigger->name
> is not guaranteed to be unique. This is despite the fact that it's
> documented as "@name: [DRIVER] unique name" on top of struct
> iio_trigger. Just like indio_dev->name it seems to be a mostly cosmetic
> label.
> 
> You can easily create a software trigger with a duplicate name if you
> enable CONFIG_IIO_HRTIMER_TRIGGER:
> 
> mkdir /sys/kernel/config/iio/triggers/hrtimer/`cat
> /sys/bus/iio/devices/trigger0/name`
> 
> It seems that trigger names are set at allocation time, mostly through
> some variant of:
> 
>     iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> 
> indio_dev->name is not guaranteed to be unique but indio_dev->id is. The
> are some exceptions, including st_sensors which does:
> 
>     iio_trigger_alloc("%s-trigger", indio_dev->name);
> 
> Since indio_dev->name is just the model name it seems that attaching two
> identical st_sensors devices will create two triggers with the same name.
> 

Yeah, this is certainly broken.

> The "current_trigger" for an iio_device is identified by name. If you
> have two triggers with the same name then the "first one" will be picked
> up, probably in registration order.
> 
> One solution would be to refuse to register triggers with duplicate
> names but that might cause probe errors in some cases.
> 

Well, it is broken anyway and wont work as expected, which means nobody
probably ever ran such a configuration, so we might return an error to
prevent unexpected behavior.

> Another option would be to just warn on duplicate names and provide a
> separate current_trigger_id which identifies the current trigger by
> numeric ID. In general it seems to me that it's better to identify
> entities by numbers rather than strings.

We could support writing triggerX to current_name as an alternative to the
name to get a unique identifier.

Re: IIO trigger names are not actually unique

[Jonathan Cameron]

On 09/05/16 10:04, Lars-Peter Clausen wrote:
> On 05/05/2016 01:52 PM, Crestez Dan Leonard wrote:
>> Hello,
>>
>> I've been looking at trigger code and it seems to me that trigger->name
>> is not guaranteed to be unique. This is despite the fact that it's
>> documented as "@name: [DRIVER] unique name" on top of struct
>> iio_trigger. Just like indio_dev->name it seems to be a mostly cosmetic
>> label.
>>
>> You can easily create a software trigger with a duplicate name if you
>> enable CONFIG_IIO_HRTIMER_TRIGGER:
>>
>> mkdir /sys/kernel/config/iio/triggers/hrtimer/`cat
>> /sys/bus/iio/devices/trigger0/name`
>>
>> It seems that trigger names are set at allocation time, mostly through
>> some variant of:
>>
>>     iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
>>
>> indio_dev->name is not guaranteed to be unique but indio_dev->id is. The
>> are some exceptions, including st_sensors which does:
>>
>>     iio_trigger_alloc("%s-trigger", indio_dev->name);
>>
>> Since indio_dev->name is just the model name it seems that attaching two
>> identical st_sensors devices will create two triggers with the same name.
>>
> 
> Yeah, this is certainly broken.
> 
>> The "current_trigger" for an iio_device is identified by name. If you
>> have two triggers with the same name then the "first one" will be picked
>> up, probably in registration order.
>>
>> One solution would be to refuse to register triggers with duplicate
>> names but that might cause probe errors in some cases.
>>
> 
> Well, it is broken anyway and wont work as expected, which means nobody
> probably ever ran such a configuration, so we might return an error to
> prevent unexpected behavior.
Agreed - I think that would be a safe ABI fix to make.
> 
>> Another option would be to just warn on duplicate names and provide a
>> separate current_trigger_id which identifies the current trigger by
>> numeric ID. In general it seems to me that it's better to identify
>> entities by numbers rather than strings.
> 
> We could support writing triggerX to current_name as an alternative to the
> name to get a unique identifier.
That would also work fine as far as I'm concerned.  Mostly a case of documenting
that case clearly in the ABI docs.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>