[PATCH] iio: trigger: Fix condition for own trigger

[PATCH] iio: trigger: Fix condition for own trigger

[João Paulo Gonçalves]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

The condition for checking if triggers belong to the same IIO device to
set attached_own_device is currently inverted, causing
iio_trigger_using_own() to return an incorrect value. Fix it by testing
for the correct return value of iio_validate_own_trigger().

Cc: stable@vger.kernel.org
Fixes: 517985ebc531 ("iio: trigger: Add simple trigger_validation helper")
Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
---
 drivers/iio/industrialio-trigger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 16de57846bd9..2e84776f4fbd 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -315,7 +315,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	 * this is the case if the IIO device and the trigger device share the
 	 * same parent device.
 	 */
-	if (iio_validate_own_trigger(pf->indio_dev, trig))
+	if (!iio_validate_own_trigger(pf->indio_dev, trig))
 		trig->attached_own_device = true;

 	return ret;
--
2.34.1

Re: [PATCH] iio: trigger: Fix condition for own trigger

[Jonathan Cameron]

On Fri, 14 Jun 2024 11:36:58 -0300
João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com> wrote:

> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> The condition for checking if triggers belong to the same IIO device to
> set attached_own_device is currently inverted, causing
> iio_trigger_using_own() to return an incorrect value. Fix it by testing
> for the correct return value of iio_validate_own_trigger().
> 
> Cc: stable@vger.kernel.org
> Fixes: 517985ebc531 ("iio: trigger: Add simple trigger_validation helper")
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>

Ouch.  Can you give an example of resulting user visible result? That
will help people decide whether to pick this up for their distro kernels
etc.  In some cases, looks like we'll get garbage timestamps and in others
may get stale data (or garbage).

Odd no one has noticed this in the past whilst testing those dependent
features in particular drivers and I worry a little that we may have bugs
in the users as a result of iio_trigger_using_own() reporting the inverse
of the intended. I've take a quick look at the users and 'think' they are
ok, but would definitely like a few others to confirm.

Also on a practical basis I just sent a fixes pull request so this one
probably won't go anywhere for a week or so anyway so we have time.

> ---
>  drivers/iio/industrialio-trigger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 16de57846bd9..2e84776f4fbd 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -315,7 +315,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>  	 * this is the case if the IIO device and the trigger device share the
>  	 * same parent device.
>  	 */
> -	if (iio_validate_own_trigger(pf->indio_dev, trig))
> +	if (!iio_validate_own_trigger(pf->indio_dev, trig))
>  		trig->attached_own_device = true;
> 
>  	return ret;
> --
> 2.34.1

Re: [PATCH] iio: trigger: Fix condition for own trigger

[Francesco Dolcini]

On Sat, Jun 15, 2024 at 11:50:18AM +0100, Jonathan Cameron wrote:
> On Fri, 14 Jun 2024 11:36:58 -0300
> João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com> wrote:
> 
> > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > 
> > The condition for checking if triggers belong to the same IIO device to
> > set attached_own_device is currently inverted, causing
> > iio_trigger_using_own() to return an incorrect value. Fix it by testing
> > for the correct return value of iio_validate_own_trigger().
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 517985ebc531 ("iio: trigger: Add simple trigger_validation helper")
> > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>

Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>

> 
> Ouch.  Can you give an example of resulting user visible result? That
> will help people decide whether to pick this up for their distro kernels
> etc.  In some cases, looks like we'll get garbage timestamps and in others
> may get stale data (or garbage).

This was noticed while me and Joao were working on the ads1119 driver you
have been recently reviewing. We wanted to use iio_trigger_using_own()
and it was not behaving the right way. We looked into it and found the bug.

Given that I do not know the exact impact on the drivers that are using this
function.

> Odd no one has noticed this in the past whilst testing those dependent
> features in particular drivers and I worry a little that we may have bugs
> in the users as a result of iio_trigger_using_own() reporting the inverse
> of the intended. I've take a quick look at the users and 'think' they are
> ok, but would definitely like a few others to confirm.

All the users of iio_trigger_using_own() are older than the commit that
introduced the bug, it is safe to assume that they need the fix and
are expecting the function to behave the same way is documented and it was
before the bug was introduced.

The broken commit is not that old and less than 10 IIO drivers are using this
function. Given that I think that is not that odd that it took 1 year to find
the bug.

Francesco

Re: [PATCH] iio: trigger: Fix condition for own trigger

[Matti Vaittinen]

On 6/14/24 17:36, João Paulo Gonçalves wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> The condition for checking if triggers belong to the same IIO device to
> set attached_own_device is currently inverted, causing
> iio_trigger_using_own() to return an incorrect value. Fix it by testing
> for the correct return value of iio_validate_own_trigger().
> 
> Cc: stable@vger.kernel.org
> Fixes: 517985ebc531 ("iio: trigger: Add simple trigger_validation helper")
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Thanks for fixing this!

> ---
>   drivers/iio/industrialio-trigger.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 16de57846bd9..2e84776f4fbd 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -315,7 +315,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>   	 * this is the case if the IIO device and the trigger device share the
>   	 * same parent device.
>   	 */
> -	if (iio_validate_own_trigger(pf->indio_dev, trig))
> +	if (!iio_validate_own_trigger(pf->indio_dev, trig))
>   		trig->attached_own_device = true;
> 
>   	return ret;
> --
> 2.34.1

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH] iio: trigger: Fix condition for own trigger

[Jonathan Cameron]

On Sun, 16 Jun 2024 11:38:54 +0200
Francesco Dolcini <francesco@dolcini.it> wrote:

> On Sat, Jun 15, 2024 at 11:50:18AM +0100, Jonathan Cameron wrote:
> > On Fri, 14 Jun 2024 11:36:58 -0300
> > João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com> wrote:
> >   
> > > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > 
> > > The condition for checking if triggers belong to the same IIO device to
> > > set attached_own_device is currently inverted, causing
> > > iio_trigger_using_own() to return an incorrect value. Fix it by testing
> > > for the correct return value of iio_validate_own_trigger().
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 517985ebc531 ("iio: trigger: Add simple trigger_validation helper")
> > > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>  
> 
> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> > 
> > Ouch.  Can you give an example of resulting user visible result? That
> > will help people decide whether to pick this up for their distro kernels
> > etc.  In some cases, looks like we'll get garbage timestamps and in others
> > may get stale data (or garbage).  
> 
> This was noticed while me and Joao were working on the ads1119 driver you
> have been recently reviewing. We wanted to use iio_trigger_using_own()
> and it was not behaving the right way. We looked into it and found the bug.
> 
> Given that I do not know the exact impact on the drivers that are using this
> function.
> 
> > Odd no one has noticed this in the past whilst testing those dependent
> > features in particular drivers and I worry a little that we may have bugs
> > in the users as a result of iio_trigger_using_own() reporting the inverse
> > of the intended. I've take a quick look at the users and 'think' they are
> > ok, but would definitely like a few others to confirm.  
> 
> All the users of iio_trigger_using_own() are older than the commit that
> introduced the bug, it is safe to assume that they need the fix and
> are expecting the function to behave the same way is documented and it was
> before the bug was introduced.
> 
> The broken commit is not that old and less than 10 IIO drivers are using this
> function. Given that I think that is not that odd that it took 1 year to find
> the bug.

Yes. Long tail of IIO devices that are used on the sort of board that only
gets a kernel update once in a while and well behind mainline.  So indeed
not that surprising :( 

Applied to the fixes-togreg branch of iio.git

Thanks,

Jonathan

> 
> Francesco
>