| Message ID | 20200211073511.r24n3bygyjxrsuez@kili.mountain |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: siox: potentially enabling IRQs too early | expand |
[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
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
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
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
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; }
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(-)