[RFC PATCH] arm/imx/gpio: add spinlock protection

[Sascha Hauer]

On Sun, Jul 04, 2010 at 10:15:13AM +0300, Baruch Siach wrote:
> The GPIO and IRQ/GPIO registers need protection from concurrent access for
> operations that are not atomic.

I don't think we need locking here. mxc_gpio_irq_handler is called with
desc->lock held (from the parent interrupt, not the chained interrupts).
Other functions like enable_irq/disable_irq which result in mask/unmask
operations run with interrupts disabled.

Apart from this other architectures do not use locking here aswell.

Sascha

> 
> Cc: Juergen Beisert <j.beisert at pengutronix.de>
> Cc: Daniel Mack <daniel at caiaq.de>
> Reported-by: rpkamiak at rockwellcollins.com
> Signed-off-by: Baruch Siach <baruch at tkos.co.il>
> ---
>  arch/arm/plat-mxc/gpio.c              |   28 +++++++++++++++++++++++++---
>  arch/arm/plat-mxc/include/mach/gpio.h |    3 +++
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
> index 71437c6..a8a33cd 100644
> --- a/arch/arm/plat-mxc/gpio.c
> +++ b/arch/arm/plat-mxc/gpio.c
> @@ -56,10 +56,13 @@ static void _set_gpio_irqenable(struct mxc_gpio_port *port, u32 index,
>  				int enable)
>  {
>  	u32 l;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&port->irq_lock, flags);
>  	l = __raw_readl(port->base + GPIO_IMR);
>  	l = (l & (~(1 << index))) | (!!enable << index);
>  	__raw_writel(l, port->base + GPIO_IMR);
> +	spin_unlock_irqrestore(&port->irq_lock, flags);
>  }
>  
>  static void gpio_ack_irq(u32 irq)
> @@ -87,8 +90,11 @@ static int gpio_set_irq_type(u32 irq, u32 type)
>  	u32 gpio = irq_to_gpio(irq);
>  	struct mxc_gpio_port *port = &mxc_gpio_ports[gpio / 32];
>  	u32 bit, val;
> -	int edge;
> +	int edge, rc = 0;
>  	void __iomem *reg = port->base;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->irq_lock, flags);
>  
>  	port->both_edges &= ~(1 << (gpio & 31));
>  	switch (type) {
> @@ -116,7 +122,8 @@ static int gpio_set_irq_type(u32 irq, u32 type)
>  		edge = GPIO_INT_HIGH_LEV;
>  		break;
>  	default:
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto out;
>  	}
>  
>  	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> @@ -125,9 +132,12 @@ static int gpio_set_irq_type(u32 irq, u32 type)
>  	__raw_writel(val | (edge << (bit << 1)), reg);
>  	_clear_gpio_irqstatus(port, gpio & 0x1f);
>  
> -	return 0;
> +out:
> +	spin_unlock_irqrestore(&port->irq_lock, flags);
> +	return rc;
>  }
>  
> +/* caller must hold port->irq_lock */
>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  {
>  	void __iomem *reg = port->base;
> @@ -157,12 +167,15 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  static void mxc_gpio_irq_handler(struct mxc_gpio_port *port, u32 irq_stat)
>  {
>  	u32 gpio_irq_no_base = port->virtual_irq_start;
> +	unsigned long flags;
>  
>  	while (irq_stat != 0) {
>  		int irqoffset = fls(irq_stat) - 1;
>  
> +		spin_lock_irqsave(&port->irq_lock, flags);
>  		if (port->both_edges & (1 << irqoffset))
>  			mxc_flip_edge(port, irqoffset);
> +		spin_unlock_irqrestore(&port->irq_lock, flags);
>  
>  		generic_handle_irq(gpio_irq_no_base + irqoffset);
>  
> @@ -214,13 +227,16 @@ static void _set_gpio_direction(struct gpio_chip *chip, unsigned offset,
>  	struct mxc_gpio_port *port =
>  		container_of(chip, struct mxc_gpio_port, chip);
>  	u32 l;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&port->lock, flags);
>  	l = __raw_readl(port->base + GPIO_GDIR);
>  	if (dir)
>  		l |= 1 << offset;
>  	else
>  		l &= ~(1 << offset);
>  	__raw_writel(l, port->base + GPIO_GDIR);
> +	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> @@ -229,9 +245,12 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  		container_of(chip, struct mxc_gpio_port, chip);
>  	void __iomem *reg = port->base + GPIO_DR;
>  	u32 l;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&port->lock, flags);
>  	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
>  	__raw_writel(l, reg);
> +	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static int mxc_gpio_get(struct gpio_chip *chip, unsigned offset)
> @@ -285,6 +304,9 @@ int __init mxc_gpio_init(struct mxc_gpio_port *port, int cnt)
>  		port[i].chip.base = i * 32;
>  		port[i].chip.ngpio = 32;
>  
> +		spin_lock_init(&port[i].lock);
> +		spin_lock_init(&port[i].irq_lock);
> +
>  		/* its a serious configuration bug when it fails */
>  		BUG_ON( gpiochip_add(&port[i].chip) < 0 );
>  
> diff --git a/arch/arm/plat-mxc/include/mach/gpio.h b/arch/arm/plat-mxc/include/mach/gpio.h
> index 894d2f8..a37724a 100644
> --- a/arch/arm/plat-mxc/include/mach/gpio.h
> +++ b/arch/arm/plat-mxc/include/mach/gpio.h
> @@ -36,6 +36,9 @@ struct mxc_gpio_port {
>  	int virtual_irq_start;
>  	struct gpio_chip chip;
>  	u32 both_edges;
> +
> +	spinlock_t	lock;		/* GPIO registers */
> +	spinlock_t	irq_lock;	/* IRQ registers */
>  };
>  
>  int mxc_gpio_init(struct mxc_gpio_port*, int);
> -- 
> 1.7.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |