| Message ID | 20241207043045.1255409-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: at91: call input_free_device() on allocated iio_dev | expand |
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; > } >
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.
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.
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; }
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(-)