[PATCH] IIO: Fix adding more than one iio device eventset

[PATCH] IIO: Fix adding more than one iio device eventset

[yauhen.kharuzhy]

From: Yauhen Kharuzhy <yauhen.kharuzhy@promwad.com>

iio_get_new_idr_val() returns new id, but this value was checked as
usual error code.

Also fix typo in sysfs attribute name generation, seems that this name
should be unique.

Signed-off-by: Yauhen Kharuzhy <yauhen.kharuzhy@promwad.com>
---
 drivers/staging/iio/industrialio-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index b456dfc..8d33584 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -659,7 +659,7 @@ static int iio_device_register_eventset(struct iio_dev *dev_info)
 	for (i = 0; i < dev_info->num_interrupt_lines; i++) {
 		dev_info->event_interfaces[i].owner = dev_info->driver_module;
 		ret = iio_get_new_idr_val(&iio_event_idr);
-		if (ret)
+		if (ret < 0)
 			goto error_free_setup_ev_ints;
 		else
 			dev_info->event_interfaces[i].id = ret;
@@ -685,7 +685,7 @@ static int iio_device_register_eventset(struct iio_dev *dev_info)
 
 	for (i = 0; i < dev_info->num_interrupt_lines; i++) {
 		snprintf(dev_info->event_interfaces[i]._attrname, 20,
-			"event_line%d_sources", i);
+			"event_line%d_sources", dev_info->event_interfaces[i].id);
 		dev_info->event_attrs[i].name
 			= (const char *)
 			(dev_info->event_interfaces[i]._attrname);
-- 
1.6.6.1

Re: [PATCH] IIO: Fix adding more than one iio device eventset

[Jonathan Cameron]

Hi,

NACK

Thanks for the patch.  The first bug was fixed by a patch from Sonic Zhang.
Andrew Morton picked it up so it is in the mm tree.  Thanks for the report
of this particularly horrible bug though as had been there quite a while before
Sonic pointed it out!

http://userweb.kernel.org/~akpm/mmotm/broken-out/iio-iio_get_new_idr_val-return-negative-value-on-failure-fix.patch

I don't think the second is actually a bug.  I agree it would make more sense if the numbering
of event_line_sources matched that of the character device through which the events
are read, but technically it doesn't have to do so.  So your fix for this one is
reasonable but not vital.  It is actually a typo either, more or an evolutionary
disconnect in naming!   The relevant code is removed entirely by the abi change
patch set.  On that iirc the equivalent sysfs files are in

/sys/bus/iio/devices/device[n]:event[m]
which can also be acessed via
/sys/bus/iio/devices/device[n]/device[n]:event[m]

For reference, the relevant patches are:

http://marc.info/?l=linux-iio&m=126980876128689&w=2
and
http://marc.info/?l=linux-iio&m=126980876328704&w=2

These will probably have a v2 before I send these to lkml - if nothing else some
of the early patches in the series have pending changes and as it turns out
a few minor reworks of the abi spec are still needed to avoid ambiguous event
parameter naming.

Thanks again,

Jonathan

> iio_get_new_idr_val() returns new id, but this value was checked as
> usual error code.
> 
> Also fix typo in sysfs attribute name generation, seems that this name
> should be unique.
> 
> Signed-off-by: Yauhen Kharuzhy <yauhen.kharuzhy@promwad.com>
> ---
>  drivers/staging/iio/industrialio-core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
> index b456dfc..8d33584 100644
> --- a/drivers/staging/iio/industrialio-core.c
> +++ b/drivers/staging/iio/industrialio-core.c
> @@ -659,7 +659,7 @@ static int iio_device_register_eventset(struct iio_dev *dev_info)
>  	for (i = 0; i < dev_info->num_interrupt_lines; i++) {
>  		dev_info->event_interfaces[i].owner = dev_info->driver_module;
>  		ret = iio_get_new_idr_val(&iio_event_idr);
> -		if (ret)
> +		if (ret < 0)
>  			goto error_free_setup_ev_ints;
>  		else
>  			dev_info->event_interfaces[i].id = ret;
> @@ -685,7 +685,7 @@ static int iio_device_register_eventset(struct iio_dev *dev_info)
>  
>  	for (i = 0; i < dev_info->num_interrupt_lines; i++) {
>  		snprintf(dev_info->event_interfaces[i]._attrname, 20,
> -			"event_line%d_sources", i);
> +			"event_line%d_sources", dev_info->event_interfaces[i].id);
>  		dev_info->event_attrs[i].name
>  			= (const char *)
>  			(dev_info->event_interfaces[i]._attrname);

Re: [PATCH] IIO: Fix adding more than one iio device eventset

[Andrew Morton]

On Wed, 07 Apr 2010 19:15:56 +0100
Jonathan Cameron <jic23@cam.ac.uk> wrote:

> Thanks for the patch.  The first bug was fixed by a patch from Sonic Zhang.
> Andrew Morton picked it up so it is in the mm tree.  Thanks for the report
> of this particularly horrible bug though as had been there quite a while before
> Sonic pointed it out!

hm.  Is it a "horrible bug"?  I had assumed that it was a minor bug and
had staged the patch for 2.6.35.  Was that a mistake?

Re: [PATCH] IIO: Fix adding more than one iio device eventset

[Jonathan Cameron]

On 04/08/10 20:48, Andrew Morton wrote:
> On Wed, 07 Apr 2010 19:15:56 +0100
> Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
>> Thanks for the patch.  The first bug was fixed by a patch from Sonic Zhang.
>> Andrew Morton picked it up so it is in the mm tree.  Thanks for the report
>> of this particularly horrible bug though as had been there quite a while before
>> Sonic pointed it out!
> 
> hm.  Is it a "horrible bug"?  I had assumed that it was a minor bug and
> had staged the patch for 2.6.35.  Was that a mistake?
> 
It is a bug you will only see if you attempt to bring up two iio devices
with event interfaces at the same time.  I've no idea if anyone has a, non-dev
board where this is happening. If you trigger the bug the second devices probe
will fail. So moderately horrible but the problem is fairly self contained.

By the look of it two people have run into the bug (or spotted the problem during
code review?) so probably worth pushing the fix in sooner rather than later.

Thanks,

Jonathan