gpio: siox: use raw spinlock for irq related locking
diff mbox series

Message ID 20200211135121.15752-1-uwe@kleine-koenig.org
State New
Headers show
Series
  • gpio: siox: use raw spinlock for irq related locking
Related show

Commit Message

Uwe Kleine-König Feb. 11, 2020, 1:51 p.m. UTC
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(-)

Comments

Thorsten Scherer Feb. 14, 2020, 11:02 a.m. UTC | #1
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 |
Uwe Kleine-König Feb. 14, 2020, 12:20 p.m. UTC | #2
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
Linus Walleij Feb. 21, 2020, 1:52 p.m. UTC | #3
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
Uwe Kleine-König Feb. 21, 2020, 2:54 p.m. UTC | #4
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

Patch
diff mbox series

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;