[PATCH] iio: adc: at91: call input_free_device() on allocated iio_dev

[PATCH] iio: adc: at91: call input_free_device() on allocated iio_dev

[Joe Hattori]

Current implementation of at91_ts_register() calls input_free_deivce()
on st->ts_input, however, the err label can be reached before the
allocated iio_dev is stored to st->ts_input. Thus call
input_free_device() on input instead of st->ts_input.

Fixes: 84882b060301 ("iio: adc: at91_adc: Add support for touchscreens without TSMR")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 drivers/iio/adc/at91_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index a3f0a2321666..5927756b749a 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -979,7 +979,7 @@ static int at91_ts_register(struct iio_dev *idev,
 	return ret;
 
 err:
-	input_free_device(st->ts_input);
+	input_free_device(input);
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH] iio: adc: at91: call input_free_device() on allocated iio_dev

[Jonathan Cameron]

On Sat,  7 Dec 2024 13:30:45 +0900
Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> wrote:

> Current implementation of at91_ts_register() calls input_free_deivce()
> on st->ts_input, however, the err label can be reached before the
> allocated iio_dev is stored to st->ts_input. Thus call
> input_free_device() on input instead of st->ts_input.
> 
> Fixes: 84882b060301 ("iio: adc: at91_adc: Add support for touchscreens without TSMR")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
Hi Joe.

Good catch.  Longer term I'd like this driver to be fully converted to devm
managed cleanup though which would have made this bug go away.

However, having looked at it, that conversion is a rather substantial, if simple
patch, so I'm fine taking this fix and maybe someone will revisit to do that
cleanup later.

Applied and marked for stable.

thanks,

Jonathan

> ---
>  drivers/iio/adc/at91_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index a3f0a2321666..5927756b749a 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -979,7 +979,7 @@ static int at91_ts_register(struct iio_dev *idev,
>  	return ret;
>  
>  err:
> -	input_free_device(st->ts_input);
> +	input_free_device(input);
>  	return ret;
>  }
>  

Re: [PATCH] iio: adc: at91: call input_free_device() on allocated iio_dev

[Andy Shevchenko]

On Sat, Dec 07, 2024 at 05:30:46PM +0000, Jonathan Cameron wrote:
> On Sat,  7 Dec 2024 13:30:45 +0900
> Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> wrote:
> 
> > Current implementation of at91_ts_register() calls input_free_deivce()
> > on st->ts_input, however, the err label can be reached before the
> > allocated iio_dev is stored to st->ts_input. Thus call
> > input_free_device() on input instead of st->ts_input.
> > 
> > Fixes: 84882b060301 ("iio: adc: at91_adc: Add support for touchscreens without TSMR")
> > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> Hi Joe.
> 
> Good catch.  Longer term I'd like this driver to be fully converted to devm
> managed cleanup though which would have made this bug go away.

I dunno it's good.

First of all, the message doesn't contain any pointers to real issue, because
there is none. And this is can be checked in two clicks on Elixir. Hence, the
all dance with Fixes tag is wrong.

Second, the submissions is not following the researcher guidelines.

Please, consider dropping this.

+Cc: Greg to flag this email for not following the rules.
Joe, I highly recommend to answer to all your patches that you sent so far that
they should not be applied (at least in their current forms).

See also for the details: 20241204122152.1312051-1-joe@pf.is.s.u-tokyo.ac.jp.mbx.

> However, having looked at it, that conversion is a rather substantial, if simple
> patch, so I'm fine taking this fix and maybe someone will revisit to do that
> cleanup later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: at91: call input_free_device() on allocated iio_dev

[Andy Shevchenko]

On Sat, Dec 07, 2024 at 08:58:32PM +0200, Andy Shevchenko wrote:
> On Sat, Dec 07, 2024 at 05:30:46PM +0000, Jonathan Cameron wrote:
> > On Sat,  7 Dec 2024 13:30:45 +0900
> > Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> wrote:
> > 
> > > Current implementation of at91_ts_register() calls input_free_deivce()
> > > on st->ts_input, however, the err label can be reached before the
> > > allocated iio_dev is stored to st->ts_input. Thus call
> > > input_free_device() on input instead of st->ts_input.
> > > 
> > > Fixes: 84882b060301 ("iio: adc: at91_adc: Add support for touchscreens without TSMR")
> > > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> > Hi Joe.
> > 
> > Good catch.  Longer term I'd like this driver to be fully converted to devm
> > managed cleanup though which would have made this bug go away.
> 
> I dunno it's good.
> 
> First of all, the message doesn't contain any pointers to real issue, because
> there is none. And this is can be checked in two clicks on Elixir. Hence, the
> all dance with Fixes tag is wrong.

Ah, sorry, I was looking to the wrong path!

> Second, the submissions is not following the researcher guidelines.

While this is true, the patch seems correct as we need to free the allocated
input.

> Please, consider dropping this.

So, patch can go, but we really need to have explanation that it's done by a
brand new static analyzer tool.

> +Cc: Greg to flag this email for not following the rules.
> Joe, I highly recommend to answer to all your patches that you sent so far that
> they should not be applied (at least in their current forms).
> 
> See also for the details: 20241204122152.1312051-1-joe@pf.is.s.u-tokyo.ac.jp.mbx.
> 
> > However, having looked at it, that conversion is a rather substantial, if simple
> > patch, so I'm fine taking this fix and maybe someone will revisit to do that
> > cleanup later.

-- 
With Best Regards,
Andy Shevchenko