| Message ID | 20200211135121.15752-1-uwe@kleine-koenig.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: siox: use raw spinlock for irq related locking | expand |
Hello, AFAICT this is all good. Unfortunately i don't have any idea on how to test out the difference this patch makes on a real SIOX. Any hints? Is it necessary at all? Thank you. Kind regards Thorsten Acked-by: Thorsten Scherer <t.scherer@eckelmann.de> On Tue, Feb 11, 2020 at 02:51:21PM +0100, Uwe Kleine-König wrote: > All the irq related callbacks are called with the (raw) spinlock > desc->lock being held. So the lock here must be raw as well. Also irqs > were already disabled by the caller for the irq chip callbacks, so the > non-irq variants of spin_lock must be used there. > > Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox") > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > > > > 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. > > Ah, I read "irqchip callbacks" as "spinlock calls". Thanks to restate > this for me. > > Thanks > Uwe > > drivers/gpio/gpio-siox.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c > index 311f66757b92..26e1fe092304 100644 > --- a/drivers/gpio/gpio-siox.c > +++ b/drivers/gpio/gpio-siox.c > @@ -15,7 +15,7 @@ struct gpio_siox_ddata { > u8 setdata[1]; > u8 getdata[3]; > > - spinlock_t irqlock; > + raw_spinlock_t irqlock; > u32 irq_enable; > u32 irq_status; > u32 irq_type[20]; > @@ -44,7 +44,7 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) > > mutex_lock(&ddata->lock); > > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock_irq(&ddata->irqlock); > > for (offset = 0; offset < 12; ++offset) { > unsigned int bitpos = 11 - offset; > @@ -66,7 +66,7 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) > > trigger = ddata->irq_status & ddata->irq_enable; > > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock_irq(&ddata->irqlock); > > ddata->getdata[0] = buf[0]; > ddata->getdata[1] = buf[1]; > @@ -84,9 +84,9 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) > * handler of the irq chip. But it doesn't, so we have > * to clean the irq_status here. > */ > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock_irq(&ddata->irqlock); > ddata->irq_status &= ~(1 << offset); > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock_irq(&ddata->irqlock); > > handle_nested_irq(irq); > } > @@ -101,9 +101,9 @@ static void gpio_siox_irq_ack(struct irq_data *d) > struct gpio_siox_ddata *ddata = > container_of(ic, struct gpio_siox_ddata, ichip); > > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock(&ddata->irqlock); > ddata->irq_status &= ~(1 << d->hwirq); > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock(&ddata->irqlock); > } > > static void gpio_siox_irq_mask(struct irq_data *d) > @@ -112,9 +112,9 @@ static void gpio_siox_irq_mask(struct irq_data *d) > struct gpio_siox_ddata *ddata = > container_of(ic, struct gpio_siox_ddata, ichip); > > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock(&ddata->irqlock); > ddata->irq_enable &= ~(1 << d->hwirq); > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock(&ddata->irqlock); > } > > static void gpio_siox_irq_unmask(struct irq_data *d) > @@ -123,9 +123,9 @@ static void gpio_siox_irq_unmask(struct irq_data *d) > struct gpio_siox_ddata *ddata = > container_of(ic, struct gpio_siox_ddata, ichip); > > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock(&ddata->irqlock); > ddata->irq_enable |= 1 << d->hwirq; > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock(&ddata->irqlock); > } > > static int gpio_siox_irq_set_type(struct irq_data *d, u32 type) > @@ -134,9 +134,9 @@ static int gpio_siox_irq_set_type(struct irq_data *d, u32 type) > struct gpio_siox_ddata *ddata = > container_of(ic, struct gpio_siox_ddata, ichip); > > - spin_lock_irq(&ddata->irqlock); > + raw_spin_lock(&ddata->irqlock); > ddata->irq_type[d->hwirq] = type; > - spin_unlock_irq(&ddata->irqlock); > + raw_spin_unlock(&ddata->irqlock); > > return 0; > } > @@ -222,7 +222,7 @@ static int gpio_siox_probe(struct siox_device *sdevice) > dev_set_drvdata(dev, ddata); > > mutex_init(&ddata->lock); > - spin_lock_init(&ddata->irqlock); > + raw_spin_lock_init(&ddata->irqlock); > > ddata->gchip.base = -1; > ddata->gchip.can_sleep = 1; > -- > 2.24.0 > -- Thorsten Scherer | Eckelmann AG | www.eckelmann.de |
Hello Thorsten, On Fri, Feb 14, 2020 at 12:02:38PM +0100, Thorsten Scherer wrote: > AFAICT this is all good. > > Unfortunately i don't have any idea on how to test out the difference > this patch makes on a real SIOX. When I started looking into the problem I expected that the lock debugging would catch these problems. But either I did something wrong or there is no mechanism that catches - a spinlock is taken when there is already a raw spinlock taken - spin_lock_irq is used with irqs already off And I wonder if there are reasons I don't see that make these two tests a bad idea. > Any hints? Is it necessary at all? Apart from "normal" testing that SIOX still works I have no good suggestion. Having said that I would be surprised if my patch breaked something. (But it wouldn't be the first time such a surprise happens :-) Best regards Uwe
On Tue, Feb 11, 2020 at 2:59 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote: > All the irq related callbacks are called with the (raw) spinlock > desc->lock being held. So the lock here must be raw as well. Also irqs > were already disabled by the caller for the irq chip callbacks, so the > non-irq variants of spin_lock must be used there. > > Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox") > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> Patch applied. Is this a regression so I should put it in fixes? I put it for v5.7 for now but I can easily change that. Yours, Linus Walleij
On 2/21/20 2:52 PM, Linus Walleij wrote: > On Tue, Feb 11, 2020 at 2:59 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote: > >> All the irq related callbacks are called with the (raw) spinlock >> desc->lock being held. So the lock here must be raw as well. Also irqs >> were already disabled by the caller for the irq chip callbacks, so the >> non-irq variants of spin_lock must be used there. >> >> Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox") >> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > Patch applied. Is this a regression so I should put it in fixes? > I put it for v5.7 for now but I can easily change that. I don't care much. AFAIK siox is only used at Eckelmann and we will have to deploy the fix there anyhow. Conceptually the conversion to raw is only relevant for RT (please correct me if I'm wrong), but too early enablement of irqs probably can yield bad races. Best regards Uwe
diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c index 311f66757b92..26e1fe092304 100644 --- a/drivers/gpio/gpio-siox.c +++ b/drivers/gpio/gpio-siox.c @@ -15,7 +15,7 @@ struct gpio_siox_ddata { u8 setdata[1]; u8 getdata[3]; - spinlock_t irqlock; + raw_spinlock_t irqlock; u32 irq_enable; u32 irq_status; u32 irq_type[20]; @@ -44,7 +44,7 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) mutex_lock(&ddata->lock); - spin_lock_irq(&ddata->irqlock); + raw_spin_lock_irq(&ddata->irqlock); for (offset = 0; offset < 12; ++offset) { unsigned int bitpos = 11 - offset; @@ -66,7 +66,7 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) trigger = ddata->irq_status & ddata->irq_enable; - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock_irq(&ddata->irqlock); ddata->getdata[0] = buf[0]; ddata->getdata[1] = buf[1]; @@ -84,9 +84,9 @@ static int gpio_siox_get_data(struct siox_device *sdevice, const u8 buf[]) * handler of the irq chip. But it doesn't, so we have * to clean the irq_status here. */ - spin_lock_irq(&ddata->irqlock); + raw_spin_lock_irq(&ddata->irqlock); ddata->irq_status &= ~(1 << offset); - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock_irq(&ddata->irqlock); handle_nested_irq(irq); } @@ -101,9 +101,9 @@ static void gpio_siox_irq_ack(struct irq_data *d) struct gpio_siox_ddata *ddata = container_of(ic, struct gpio_siox_ddata, ichip); - spin_lock_irq(&ddata->irqlock); + raw_spin_lock(&ddata->irqlock); ddata->irq_status &= ~(1 << d->hwirq); - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock(&ddata->irqlock); } static void gpio_siox_irq_mask(struct irq_data *d) @@ -112,9 +112,9 @@ static void gpio_siox_irq_mask(struct irq_data *d) struct gpio_siox_ddata *ddata = container_of(ic, struct gpio_siox_ddata, ichip); - spin_lock_irq(&ddata->irqlock); + raw_spin_lock(&ddata->irqlock); ddata->irq_enable &= ~(1 << d->hwirq); - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock(&ddata->irqlock); } static void gpio_siox_irq_unmask(struct irq_data *d) @@ -123,9 +123,9 @@ static void gpio_siox_irq_unmask(struct irq_data *d) struct gpio_siox_ddata *ddata = container_of(ic, struct gpio_siox_ddata, ichip); - spin_lock_irq(&ddata->irqlock); + raw_spin_lock(&ddata->irqlock); ddata->irq_enable |= 1 << d->hwirq; - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock(&ddata->irqlock); } static int gpio_siox_irq_set_type(struct irq_data *d, u32 type) @@ -134,9 +134,9 @@ static int gpio_siox_irq_set_type(struct irq_data *d, u32 type) struct gpio_siox_ddata *ddata = container_of(ic, struct gpio_siox_ddata, ichip); - spin_lock_irq(&ddata->irqlock); + raw_spin_lock(&ddata->irqlock); ddata->irq_type[d->hwirq] = type; - spin_unlock_irq(&ddata->irqlock); + raw_spin_unlock(&ddata->irqlock); return 0; } @@ -222,7 +222,7 @@ static int gpio_siox_probe(struct siox_device *sdevice) dev_set_drvdata(dev, ddata); mutex_init(&ddata->lock); - spin_lock_init(&ddata->irqlock); + raw_spin_lock_init(&ddata->irqlock); ddata->gchip.base = -1; ddata->gchip.can_sleep = 1;
All the irq related callbacks are called with the (raw) spinlock desc->lock being held. So the lock here must be raw as well. Also irqs were already disabled by the caller for the irq chip callbacks, so the non-irq variants of spin_lock must be used there. Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox") Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Hello, On Tue, Feb 11, 2020 at 02:18:23PM +0100, Thomas Gleixner wrote: > 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. Ah, I read "irqchip callbacks" as "spinlock calls". Thanks to restate this for me. Thanks Uwe drivers/gpio/gpio-siox.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)