gpio: siox: potentially enabling IRQs too early
diff mbox series

Message ID 20200211073511.r24n3bygyjxrsuez@kili.mountain
State New
Headers show
Series
  • gpio: siox: potentially enabling IRQs too early
Related show

Commit Message

Dan Carpenter Feb. 11, 2020, 7:35 a.m. UTC
Smatch thinks that gpio_siox_irq_set_type() can be called from
probe_irq_on().  In that case the call to spin_unlock_irq() would
renable IRQs too early.

Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/gpio/gpio-siox.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Uwe Kleine-König Feb. 11, 2020, 8:01 a.m. UTC | #1
[Dropped Gavin Schenk from recipients as he left Eckelmann. Added tglx.]

Hello,

On Tue, Feb 11, 2020 at 10:35:44AM +0300, Dan Carpenter wrote:
> Smatch thinks that gpio_siox_irq_set_type() can be called from
> probe_irq_on().  In that case the call to spin_unlock_irq() would
> renable IRQs too early.
> 
> Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/gpio/gpio-siox.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
> index 311f66757b92..578b71760939 100644
> --- a/drivers/gpio/gpio-siox.c
> +++ b/drivers/gpio/gpio-siox.c
> @@ -133,10 +133,11 @@ static int gpio_siox_irq_set_type(struct irq_data *d, u32 type)
>  	struct irq_chip *ic = irq_data_get_irq_chip(d);
>  	struct gpio_siox_ddata *ddata =
>  		container_of(ic, struct gpio_siox_ddata, ichip);
> +	unsigned long flags;
>  
> -	spin_lock_irq(&ddata->irqlock);
> +	spin_lock_irqsave(&ddata->irqlock, flags);
>  	ddata->irq_type[d->hwirq] = type;
> -	spin_unlock_irq(&ddata->irqlock);
> +	spin_unlock_irqrestore(&ddata->irqlock, flags);

"Normally" the .irq_set_type() callback is called from irq_set_irq_type.
There desc->lock is taken with raw_spin_lock_irqsave (as part of
irq_get_desc_buslock()), so I assume irqs are always disabled when the
type changes? tglx?

If so, the better fix would be to change to spin_lock/spin_unlock
instead. Also given that a raw spinlock is taken, the irqlock here
should be changed to a raw one, too?

Best regards
Uwe
Thomas Gleixner Feb. 11, 2020, 8:59 a.m. UTC | #2
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> On Tue, Feb 11, 2020 at 10:35:44AM +0300, Dan Carpenter wrote:
>> Smatch thinks that gpio_siox_irq_set_type() can be called from
>> probe_irq_on().  In that case the call to spin_unlock_irq() would
>> renable IRQs too early.
>> 
>> Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>>  drivers/gpio/gpio-siox.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
>> index 311f66757b92..578b71760939 100644
>> --- a/drivers/gpio/gpio-siox.c
>> +++ b/drivers/gpio/gpio-siox.c
>> @@ -133,10 +133,11 @@ static int gpio_siox_irq_set_type(struct irq_data *d, u32 type)
>>  	struct irq_chip *ic = irq_data_get_irq_chip(d);
>>  	struct gpio_siox_ddata *ddata =
>>  		container_of(ic, struct gpio_siox_ddata, ichip);
>> +	unsigned long flags;
>>  
>> -	spin_lock_irq(&ddata->irqlock);
>> +	spin_lock_irqsave(&ddata->irqlock, flags);
>>  	ddata->irq_type[d->hwirq] = type;
>> -	spin_unlock_irq(&ddata->irqlock);
>> +	spin_unlock_irqrestore(&ddata->irqlock, flags);
>
> "Normally" the .irq_set_type() callback is called from irq_set_irq_type.
> There desc->lock is taken with raw_spin_lock_irqsave (as part of
> irq_get_desc_buslock()), so I assume irqs are always disabled when the
> type changes? tglx?
>
> If so, the better fix would be to change to spin_lock/spin_unlock
> instead. Also given that a raw spinlock is taken, the irqlock here
> should be changed to a raw one, too?

Indeed. Looking at the driver, all of the spin_lock_irq() usage in the
irqchip callbacks is broken.

So this needs two changes:

   1) Convert to raw spin lock, as this won't work on RT otherwise

   2) s/lock_irq/lock/ for all irqchip callbacks.

Thanks,

        tglx
Uwe Kleine-König Feb. 11, 2020, 12:17 p.m. UTC | #3
On Tue, Feb 11, 2020 at 09:59:30AM +0100, Thomas Gleixner wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > On Tue, Feb 11, 2020 at 10:35:44AM +0300, Dan Carpenter wrote:
> >> Smatch thinks that gpio_siox_irq_set_type() can be called from
> >> probe_irq_on().  In that case the call to spin_unlock_irq() would
> >> renable IRQs too early.
> >> 
> >> Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> ---
> >>  drivers/gpio/gpio-siox.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
> >> index 311f66757b92..578b71760939 100644
> >> --- a/drivers/gpio/gpio-siox.c
> >> +++ b/drivers/gpio/gpio-siox.c
> >> @@ -133,10 +133,11 @@ static int gpio_siox_irq_set_type(struct irq_data *d, u32 type)
> >>  	struct irq_chip *ic = irq_data_get_irq_chip(d);
> >>  	struct gpio_siox_ddata *ddata =
> >>  		container_of(ic, struct gpio_siox_ddata, ichip);
> >> +	unsigned long flags;
> >>  
> >> -	spin_lock_irq(&ddata->irqlock);
> >> +	spin_lock_irqsave(&ddata->irqlock, flags);
> >>  	ddata->irq_type[d->hwirq] = type;
> >> -	spin_unlock_irq(&ddata->irqlock);
> >> +	spin_unlock_irqrestore(&ddata->irqlock, flags);
> >
> > "Normally" the .irq_set_type() callback is called from irq_set_irq_type.
> > There desc->lock is taken with raw_spin_lock_irqsave (as part of
> > irq_get_desc_buslock()), so I assume irqs are always disabled when the
> > type changes? tglx?
> >
> > If so, the better fix would be to change to spin_lock/spin_unlock
> > instead. Also given that a raw spinlock is taken, the irqlock here
> > should be changed to a raw one, too?
> 
> Indeed. Looking at the driver, all of the spin_lock_irq() usage in the
> irqchip callbacks is broken.
> 
> So this needs two changes:
> 
>    1) Convert to raw spin lock, as this won't work on RT otherwise
> 
>    2) s/lock_irq/lock/ for all irqchip callbacks.

Are you sure about the calls in gpio_siox_get_data()? This is only
called by siox_poll() which doesn't disable irqs.
 
Best regards
Uwe
Thomas Gleixner Feb. 11, 2020, 1:18 p.m. UTC | #4
Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> On Tue, Feb 11, 2020 at 09:59:30AM +0100, Thomas Gleixner wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> Indeed. Looking at the driver, all of the spin_lock_irq() usage in the
>> irqchip callbacks is broken.
>> 
>> So this needs two changes:
>> 
>>    1) Convert to raw spin lock, as this won't work on RT otherwise
>> 
>>    2) s/lock_irq/lock/ for all irqchip callbacks.
>
> Are you sure about the calls in gpio_siox_get_data()? This is only
> called by siox_poll() which doesn't disable irqs.

I explicitely said: "for all irqchip callbacks".

gpio_siox_get_data() is not a irq chip callback, right? So obviously it
has to stay there.

Thanks,

        tglx

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index 311f66757b92..578b71760939 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -133,10 +133,11 @@  static int gpio_siox_irq_set_type(struct irq_data *d, u32 type)
 	struct irq_chip *ic = irq_data_get_irq_chip(d);
 	struct gpio_siox_ddata *ddata =
 		container_of(ic, struct gpio_siox_ddata, ichip);
+	unsigned long flags;
 
-	spin_lock_irq(&ddata->irqlock);
+	spin_lock_irqsave(&ddata->irqlock, flags);
 	ddata->irq_type[d->hwirq] = type;
-	spin_unlock_irq(&ddata->irqlock);
+	spin_unlock_irqrestore(&ddata->irqlock, flags);
 
 	return 0;
 }