Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 31/05/16 15:03, Lars-Peter Clausen wrote:
> On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
>> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
 The trigger name is documented as unique but drivers are currently
 allowed to register triggers with duplicate names. This should be
 considered a bug since it makes the 'current_trigger' interface
 unusable.

 Signed-off-by: Crestez Dan Leonard 
>>> This feels like the right approach to my mind (and should have been there
>>> all along - oops).
>>>
>>> However, we do need to avoid breaking userspace. It's ugly but for those 3 
>>> drivers
>>> can we assume that using more than one on a board was impossible before 
>>> this series
>>> and as such play a slight game in which we don't change the trigger name 
>>> they
>>> are exporting, unless that name is already in use?
>>>
>>> It's ugly but it gets us the nicest solution for all drivers for a bit of 
>>> ugly in
>>> 3 of them...
>>
>> How would that look like? I guess I could handle -EEXIST from
>> iio_trigger_register and try again with another name? Unfortunately the
>> name is initialized at alloc time while uniqueness can only be checked
>> at register time. This would require some refactoring in drivers for
>> devices I don't have.
>>
>> An alternative would be to just submit patches 4/5 and only give a
>> warning when non-unique trigger names are used. After all, iio device
>> names are not unique, the easy way would be to give up on this guarantee
>> for trigger names as well.
>>
> 
> I'd say apply this patch keep things as they are in the drivers and if
> somebody creates a board with more than one of those devices let them come
> up with a fix.

Fair call. Lets do that.

Applied this patch to the togreg branch of iio.git (usual testing blah blah)
Let's see who (hopefully no one) screams.

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 31/05/16 15:03, Lars-Peter Clausen wrote:
> On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
>> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
 The trigger name is documented as unique but drivers are currently
 allowed to register triggers with duplicate names. This should be
 considered a bug since it makes the 'current_trigger' interface
 unusable.

 Signed-off-by: Crestez Dan Leonard 
>>> This feels like the right approach to my mind (and should have been there
>>> all along - oops).
>>>
>>> However, we do need to avoid breaking userspace. It's ugly but for those 3 
>>> drivers
>>> can we assume that using more than one on a board was impossible before 
>>> this series
>>> and as such play a slight game in which we don't change the trigger name 
>>> they
>>> are exporting, unless that name is already in use?
>>>
>>> It's ugly but it gets us the nicest solution for all drivers for a bit of 
>>> ugly in
>>> 3 of them...
>>
>> How would that look like? I guess I could handle -EEXIST from
>> iio_trigger_register and try again with another name? Unfortunately the
>> name is initialized at alloc time while uniqueness can only be checked
>> at register time. This would require some refactoring in drivers for
>> devices I don't have.
>>
>> An alternative would be to just submit patches 4/5 and only give a
>> warning when non-unique trigger names are used. After all, iio device
>> names are not unique, the easy way would be to give up on this guarantee
>> for trigger names as well.
>>
> 
> I'd say apply this patch keep things as they are in the drivers and if
> somebody creates a board with more than one of those devices let them come
> up with a fix.

Fair call. Lets do that.

Applied this patch to the togreg branch of iio.git (usual testing blah blah)
Let's see who (hopefully no one) screams.

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Lars-Peter Clausen]

On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>>> The trigger name is documented as unique but drivers are currently
>>> allowed to register triggers with duplicate names. This should be
>>> considered a bug since it makes the 'current_trigger' interface
>>> unusable.
>>>
>>> Signed-off-by: Crestez Dan Leonard 
>> This feels like the right approach to my mind (and should have been there
>> all along - oops).
>>
>> However, we do need to avoid breaking userspace. It's ugly but for those 3 
>> drivers
>> can we assume that using more than one on a board was impossible before this 
>> series
>> and as such play a slight game in which we don't change the trigger name they
>> are exporting, unless that name is already in use?
>>
>> It's ugly but it gets us the nicest solution for all drivers for a bit of 
>> ugly in
>> 3 of them...
> 
> How would that look like? I guess I could handle -EEXIST from
> iio_trigger_register and try again with another name? Unfortunately the
> name is initialized at alloc time while uniqueness can only be checked
> at register time. This would require some refactoring in drivers for
> devices I don't have.
> 
> An alternative would be to just submit patches 4/5 and only give a
> warning when non-unique trigger names are used. After all, iio device
> names are not unique, the easy way would be to give up on this guarantee
> for trigger names as well.
> 

I'd say apply this patch keep things as they are in the drivers and if
somebody creates a board with more than one of those devices let them come
up with a fix.

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Lars-Peter Clausen]

On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>>> The trigger name is documented as unique but drivers are currently
>>> allowed to register triggers with duplicate names. This should be
>>> considered a bug since it makes the 'current_trigger' interface
>>> unusable.
>>>
>>> Signed-off-by: Crestez Dan Leonard 
>> This feels like the right approach to my mind (and should have been there
>> all along - oops).
>>
>> However, we do need to avoid breaking userspace. It's ugly but for those 3 
>> drivers
>> can we assume that using more than one on a board was impossible before this 
>> series
>> and as such play a slight game in which we don't change the trigger name they
>> are exporting, unless that name is already in use?
>>
>> It's ugly but it gets us the nicest solution for all drivers for a bit of 
>> ugly in
>> 3 of them...
> 
> How would that look like? I guess I could handle -EEXIST from
> iio_trigger_register and try again with another name? Unfortunately the
> name is initialized at alloc time while uniqueness can only be checked
> at register time. This would require some refactoring in drivers for
> devices I don't have.
> 
> An alternative would be to just submit patches 4/5 and only give a
> warning when non-unique trigger names are used. After all, iio device
> names are not unique, the easy way would be to give up on this guarantee
> for trigger names as well.
> 

I'd say apply this patch keep things as they are in the drivers and if
somebody creates a board with more than one of those devices let them come
up with a fix.

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Crestez Dan Leonard]

On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>> The trigger name is documented as unique but drivers are currently
>> allowed to register triggers with duplicate names. This should be
>> considered a bug since it makes the 'current_trigger' interface
>> unusable.
>>
>> Signed-off-by: Crestez Dan Leonard 
> This feels like the right approach to my mind (and should have been there
> all along - oops).
> 
> However, we do need to avoid breaking userspace. It's ugly but for those 3 
> drivers
> can we assume that using more than one on a board was impossible before this 
> series
> and as such play a slight game in which we don't change the trigger name they
> are exporting, unless that name is already in use?
> 
> It's ugly but it gets us the nicest solution for all drivers for a bit of 
> ugly in
> 3 of them...

How would that look like? I guess I could handle -EEXIST from
iio_trigger_register and try again with another name? Unfortunately the
name is initialized at alloc time while uniqueness can only be checked
at register time. This would require some refactoring in drivers for
devices I don't have.

An alternative would be to just submit patches 4/5 and only give a
warning when non-unique trigger names are used. After all, iio device
names are not unique, the easy way would be to give up on this guarantee
for trigger names as well.

-- 
Regards,
Leonard

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Crestez Dan Leonard]

On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>> The trigger name is documented as unique but drivers are currently
>> allowed to register triggers with duplicate names. This should be
>> considered a bug since it makes the 'current_trigger' interface
>> unusable.
>>
>> Signed-off-by: Crestez Dan Leonard 
> This feels like the right approach to my mind (and should have been there
> all along - oops).
> 
> However, we do need to avoid breaking userspace. It's ugly but for those 3 
> drivers
> can we assume that using more than one on a board was impossible before this 
> series
> and as such play a slight game in which we don't change the trigger name they
> are exporting, unless that name is already in use?
> 
> It's ugly but it gets us the nicest solution for all drivers for a bit of 
> ugly in
> 3 of them...

How would that look like? I guess I could handle -EEXIST from
iio_trigger_register and try again with another name? Unfortunately the
name is initialized at alloc time while uniqueness can only be checked
at register time. This would require some refactoring in drivers for
devices I don't have.

An alternative would be to just submit patches 4/5 and only give a
warning when non-unique trigger names are used. After all, iio device
names are not unique, the easy way would be to give up on this guarantee
for trigger names as well.

-- 
Regards,
Leonard

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 23/05/16 19:40, Crestez Dan Leonard wrote:
> The trigger name is documented as unique but drivers are currently
> allowed to register triggers with duplicate names. This should be
> considered a bug since it makes the 'current_trigger' interface
> unusable.
> 
> Signed-off-by: Crestez Dan Leonard 
This feels like the right approach to my mind (and should have been there
all along - oops).

However, we do need to avoid breaking userspace. It's ugly but for those 3 
drivers
can we assume that using more than one on a board was impossible before this 
series
and as such play a slight game in which we don't change the trigger name they
are exporting, unless that name is already in use?

It's ugly but it gets us the nicest solution for all drivers for a bit of ugly 
in
3 of them...

Jonathan
> ---
>  drivers/iio/industrialio-trigger.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c 
> b/drivers/iio/industrialio-trigger.c
> index e79c64c..e77503c 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(iio_trig_dev);
>  
> +static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
> +
>  int iio_trigger_register(struct iio_trigger *trig_info)
>  {
>   int ret;
> @@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)
>  
>   /* Add to list of available triggers held by the IIO core */
>   mutex_lock(_trigger_list_lock);
> + if (__iio_trigger_find_by_name(trig_info->name)) {
> + pr_err("Duplicate trigger name '%s'\n", trig_info->name);
> + ret = -EEXIST;
> + goto error_device_del;
> + }
>   list_add_tail(_info->list, _trigger_list);
>   mutex_unlock(_trigger_list_lock);
>  
>   return 0;
>  
> +error_device_del:
> + device_del(_info->dev);
>  error_unregister_id:
>   ida_simple_remove(_trigger_ida, trig_info->id);
>   return ret;
> @@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger 
> *trig_info)
>  }
>  EXPORT_SYMBOL(iio_trigger_unregister);
>  
> +/* Search for trigger by name, assuming iio_trigger_list_lock held */
> +static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
> +{
> + struct iio_trigger *iter;
> +
> + list_for_each_entry(iter, _trigger_list, list)
> + if (!strcmp(iter->name, name))
> + return iter;
> +
> + return NULL;
> +}
> +
>  static struct iio_trigger *iio_trigger_find_by_name(const char *name,
>   size_t len)
>  {
> 

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 23/05/16 19:40, Crestez Dan Leonard wrote:
> The trigger name is documented as unique but drivers are currently
> allowed to register triggers with duplicate names. This should be
> considered a bug since it makes the 'current_trigger' interface
> unusable.
> 
> Signed-off-by: Crestez Dan Leonard 
This feels like the right approach to my mind (and should have been there
all along - oops).

However, we do need to avoid breaking userspace. It's ugly but for those 3 
drivers
can we assume that using more than one on a board was impossible before this 
series
and as such play a slight game in which we don't change the trigger name they
are exporting, unless that name is already in use?

It's ugly but it gets us the nicest solution for all drivers for a bit of ugly 
in
3 of them...

Jonathan
> ---
>  drivers/iio/industrialio-trigger.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c 
> b/drivers/iio/industrialio-trigger.c
> index e79c64c..e77503c 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(iio_trig_dev);
>  
> +static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
> +
>  int iio_trigger_register(struct iio_trigger *trig_info)
>  {
>   int ret;
> @@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)
>  
>   /* Add to list of available triggers held by the IIO core */
>   mutex_lock(_trigger_list_lock);
> + if (__iio_trigger_find_by_name(trig_info->name)) {
> + pr_err("Duplicate trigger name '%s'\n", trig_info->name);
> + ret = -EEXIST;
> + goto error_device_del;
> + }
>   list_add_tail(_info->list, _trigger_list);
>   mutex_unlock(_trigger_list_lock);
>  
>   return 0;
>  
> +error_device_del:
> + device_del(_info->dev);
>  error_unregister_id:
>   ida_simple_remove(_trigger_ida, trig_info->id);
>   return ret;
> @@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger 
> *trig_info)
>  }
>  EXPORT_SYMBOL(iio_trigger_unregister);
>  
> +/* Search for trigger by name, assuming iio_trigger_list_lock held */
> +static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
> +{
> + struct iio_trigger *iter;
> +
> + list_for_each_entry(iter, _trigger_list, list)
> + if (!strcmp(iter->name, name))
> + return iter;
> +
> + return NULL;
> +}
> +
>  static struct iio_trigger *iio_trigger_find_by_name(const char *name,
>   size_t len)
>  {
> 

[RFC 6/7] iio: Refuse to register triggers with duplicate names

[Crestez Dan Leonard]

The trigger name is documented as unique but drivers are currently
allowed to register triggers with duplicate names. This should be
considered a bug since it makes the 'current_trigger' interface
unusable.

Signed-off-by: Crestez Dan Leonard 
---
 drivers/iio/industrialio-trigger.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c 
b/drivers/iio/industrialio-trigger.c
index e79c64c..e77503c 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(iio_trig_dev);
 
+static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
+
 int iio_trigger_register(struct iio_trigger *trig_info)
 {
int ret;
@@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)
 
/* Add to list of available triggers held by the IIO core */
mutex_lock(_trigger_list_lock);
+   if (__iio_trigger_find_by_name(trig_info->name)) {
+   pr_err("Duplicate trigger name '%s'\n", trig_info->name);
+   ret = -EEXIST;
+   goto error_device_del;
+   }
list_add_tail(_info->list, _trigger_list);
mutex_unlock(_trigger_list_lock);
 
return 0;
 
+error_device_del:
+   device_del(_info->dev);
 error_unregister_id:
ida_simple_remove(_trigger_ida, trig_info->id);
return ret;
@@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
 }
 EXPORT_SYMBOL(iio_trigger_unregister);
 
+/* Search for trigger by name, assuming iio_trigger_list_lock held */
+static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
+{
+   struct iio_trigger *iter;
+
+   list_for_each_entry(iter, _trigger_list, list)
+   if (!strcmp(iter->name, name))
+   return iter;
+
+   return NULL;
+}
+
 static struct iio_trigger *iio_trigger_find_by_name(const char *name,
size_t len)
 {
-- 
2.5.5

[RFC 6/7] iio: Refuse to register triggers with duplicate names

[Crestez Dan Leonard]

The trigger name is documented as unique but drivers are currently
allowed to register triggers with duplicate names. This should be
considered a bug since it makes the 'current_trigger' interface
unusable.

Signed-off-by: Crestez Dan Leonard 
---
 drivers/iio/industrialio-trigger.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c 
b/drivers/iio/industrialio-trigger.c
index e79c64c..e77503c 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(iio_trig_dev);
 
+static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
+
 int iio_trigger_register(struct iio_trigger *trig_info)
 {
int ret;
@@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)
 
/* Add to list of available triggers held by the IIO core */
mutex_lock(_trigger_list_lock);
+   if (__iio_trigger_find_by_name(trig_info->name)) {
+   pr_err("Duplicate trigger name '%s'\n", trig_info->name);
+   ret = -EEXIST;
+   goto error_device_del;
+   }
list_add_tail(_info->list, _trigger_list);
mutex_unlock(_trigger_list_lock);
 
return 0;
 
+error_device_del:
+   device_del(_info->dev);
 error_unregister_id:
ida_simple_remove(_trigger_ida, trig_info->id);
return ret;
@@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
 }
 EXPORT_SYMBOL(iio_trigger_unregister);
 
+/* Search for trigger by name, assuming iio_trigger_list_lock held */
+static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
+{
+   struct iio_trigger *iter;
+
+   list_for_each_entry(iter, _trigger_list, list)
+   if (!strcmp(iter->name, name))
+   return iter;
+
+   return NULL;
+}
+
 static struct iio_trigger *iio_trigger_find_by_name(const char *name,
size_t len)
 {
-- 
2.5.5