diff mbox series

[5.10,073/593] iio: light: tcs3472: do not free unallocated IRQ

Message ID 20210712060851.173417192@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg KH July 12, 2021, 6:03 a.m. UTC
From: frank zago <frank@zago.net>

commit 7cd04c863f9e1655d607705455e7714f24451984 upstream.

Allocating an IRQ is conditional to the IRQ existence, but freeing it
was not. If no IRQ was allocate, the driver would still try to free
IRQ 0. Add the missing checks.

This fixes the following trace when the driver is removed:

[  100.667788] Trying to free already-free IRQ 0
[  100.667793] WARNING: CPU: 0 PID: 2315 at kernel/irq/manage.c:1826 free_irq+0x1fd/0x370
...
[  100.667914] Call Trace:
[  100.667920]  tcs3472_remove+0x3a/0x90 [tcs3472]
[  100.667927]  i2c_device_remove+0x2b/0xa0

Signed-off-by: frank zago <frank@zago.net>
Link: https://lore.kernel.org/r/20210427022017.19314-2-frank@zago.net
Fixes: 9d2f715d592e ("iio: light: tcs3472: support out-of-threshold events")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 drivers/iio/light/tcs3472.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Pavel Machek July 13, 2021, 9:31 p.m. UTC | #1
Hi!

> commit 7cd04c863f9e1655d607705455e7714f24451984 upstream.
> 
> Allocating an IRQ is conditional to the IRQ existence, but freeing it
> was not. If no IRQ was allocate, the driver would still try to free
> IRQ 0. Add the missing checks.
> 
> This fixes the following trace when the driver is removed:
> 
> [  100.667788] Trying to free already-free IRQ 0

AFAICT this will need more fixing, because IRQ == 0 is a valid
IRQ. NO_IRQ (aka -1) should be safe to use for "no irq assigned".

Best regards,
								Pavel

> +++ b/drivers/iio/light/tcs3472.c
> @@ -531,7 +531,8 @@ static int tcs3472_probe(struct i2c_clie
>  	return 0;
>  
>  free_irq:
> -	free_irq(client->irq, indio_dev);
> +	if (client->irq)
> +		free_irq(client->irq, indio_dev);
>  buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	return ret;
Jonathan Cameron July 14, 2021, 9:04 a.m. UTC | #2
On Tue, 13 Jul 2021 23:31:28 +0200
Pavel Machek <pavel@denx.de> wrote:

> Hi!

> 

> > commit 7cd04c863f9e1655d607705455e7714f24451984 upstream.

> > 

> > Allocating an IRQ is conditional to the IRQ existence, but freeing it

> > was not. If no IRQ was allocate, the driver would still try to free

> > IRQ 0. Add the missing checks.

> > 

> > This fixes the following trace when the driver is removed:

> > 

> > [  100.667788] Trying to free already-free IRQ 0  

> 

> AFAICT this will need more fixing, because IRQ == 0 is a valid

> IRQ. NO_IRQ (aka -1) should be safe to use for "no irq assigned".

> 


I thought we had long put this to bed.  IRQ == 0 is not a valid irq number.
If there is an error in parsing DT (e.g. no irq specified) it returns 0
not NO_IRQ.  Naturally irq chips can have a 0, but that's pre translation to
virtual IRQ space.

Many years ago, ARM did have 0 as as valid IRQ, but that got cleaned up
to make it easier to do generic code like this.

Jonathan


> Best regards,

> 								Pavel

> 

> > +++ b/drivers/iio/light/tcs3472.c

> > @@ -531,7 +531,8 @@ static int tcs3472_probe(struct i2c_clie

> >  	return 0;

> >  

> >  free_irq:

> > -	free_irq(client->irq, indio_dev);

> > +	if (client->irq)

> > +		free_irq(client->irq, indio_dev);

> >  buffer_cleanup:

> >  	iio_triggered_buffer_cleanup(indio_dev);

> >  	return ret;  

>
diff mbox series

Patch

--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -531,7 +531,8 @@  static int tcs3472_probe(struct i2c_clie
 	return 0;
 
 free_irq:
-	free_irq(client->irq, indio_dev);
+	if (client->irq)
+		free_irq(client->irq, indio_dev);
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
 	return ret;
@@ -559,7 +560,8 @@  static int tcs3472_remove(struct i2c_cli
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 
 	iio_device_unregister(indio_dev);
-	free_irq(client->irq, indio_dev);
+	if (client->irq)
+		free_irq(client->irq, indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 	tcs3472_powerdown(iio_priv(indio_dev));