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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx>
> ---
> >
> > 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 |
