Locking in the (now generic) GPIO infrastructure?

Locking in the (now generic) GPIO infrastructure?

[Leon Woestenberg]

Hello,

compare void gpio_line_set() in

arch/arm/plat-iop/gpio.c:
void gpio_line_set(int line, int value)
{
	unsigned long flags;

	local_irq_save(flags);
	if (value == GPIO_LOW) {
		*IOP3XX_GPOD &= ~(1 << line);
	} else if (value == GPIO_HIGH) {
		*IOP3XX_GPOD |= 1 << line;
	}
	local_irq_restore(flags);
}

with

include/asm-arm/arch-ixp4xx/platform.h:
static inline void gpio_line_set(u8 line, int value)
{
	if (value == IXP4XX_GPIO_HIGH)
	    *IXP4XX_GPIO_GPOUTR |= (1 << line);
	else if (value == IXP4XX_GPIO_LOW)
	    *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
}

Under a Linux kernel where multiple drivers are accessing GPIO, the
latter does not seem safe against preemption (assuming the memory
read-modify-write is not atomic).

Shouldn't GPIO access be protected against concurrent access here?

Documentation/gpio.txt does not really mention the locking mechanism
assumed to modify GPIO lines.

And I think I am running into an issue with this under -rt kernels,
but that needs more analysis from my side.

Regards,
-- 
Leon

Re: Locking in the (now generic) GPIO infrastructure?

[Russell King - ARM Linux]

On Wed, Jun 04, 2008 at 01:00:19PM +0200, Leon Woestenberg wrote:
> include/asm-arm/arch-ixp4xx/platform.h:
> static inline void gpio_line_set(u8 line, int value)
> {
> 	if (value == IXP4XX_GPIO_HIGH)
> 	    *IXP4XX_GPIO_GPOUTR |= (1 << line);
> 	else if (value == IXP4XX_GPIO_LOW)
> 	    *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
> }
> 
> Under a Linux kernel where multiple drivers are accessing GPIO, the
> latter does not seem safe against preemption (assuming the memory
> read-modify-write is not atomic).

or even interrupts.

> Shouldn't GPIO access be protected against concurrent access here?

Definitely.  It's certainly buggy as currently stands.

Re: Locking in the (now generic) GPIO infrastructure?

[Ben Dooks]

On Wed, Jun 04, 2008 at 01:00:19PM +0200, Leon Woestenberg wrote:
> Hello,
> 
> compare void gpio_line_set() in
> 
> arch/arm/plat-iop/gpio.c:
> void gpio_line_set(int line, int value)
> {
> 	unsigned long flags;
> 
> 	local_irq_save(flags);
> 	if (value == GPIO_LOW) {
> 		*IOP3XX_GPOD &= ~(1 << line);
> 	} else if (value == GPIO_HIGH) {
> 		*IOP3XX_GPOD |= 1 << line;
> 	}
> 	local_irq_restore(flags);
> }
> 
> with
> 
> include/asm-arm/arch-ixp4xx/platform.h:
> static inline void gpio_line_set(u8 line, int value)
> {
> 	if (value == IXP4XX_GPIO_HIGH)
> 	    *IXP4XX_GPIO_GPOUTR |= (1 << line);
> 	else if (value == IXP4XX_GPIO_LOW)
> 	    *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
> }

Yes, that looks rather buggy to me, and also sub-optimal to boot. The
u8 line should be changed to just 'unsigned' having the compiler truncate
to 8bit isn't useful when then used with a shift.

static inline void gpio_line_set(unsigned line, int value)
{
	unsigned long flags;
	unsigned regval;

	local_irq_save(flags);

	regval = *IXP4XX_GPIO_GPOUTR;

	if (value == IXP4XX_GPIO_HIGH)
		regval |= (1 << line);
	else if (value == IXP4XX_GPIO_LOW)
	   	regval &= ~(1 << line);

	*IXP4XX_GPIO_GPOUTR = regval;
	local_irq_restore(flags);
}
 
> Under a Linux kernel where multiple drivers are accessing GPIO, the
> latter does not seem safe against preemption (assuming the memory
> read-modify-write is not atomic).
> 
> Shouldn't GPIO access be protected against concurrent access here?
> 
> Documentation/gpio.txt does not really mention the locking mechanism
> assumed to modify GPIO lines.

I think it depends on whether gpiolib is being used or not, there may
be some locking in there.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: Locking in the (now generic) GPIO infrastructure?

[David Brownell]

On Wednesday 04 June 2008, Leon Woestenberg wrote:
> include/asm-arm/arch-ixp4xx/platform.h:
> static inline void gpio_line_set(u8 line, int value)
> {
>         if (value == IXP4XX_GPIO_HIGH)
>             *IXP4XX_GPIO_GPOUTR |= (1 << line);
>         else if (value == IXP4XX_GPIO_LOW)
>             *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
> }
> 
> Under a Linux kernel where multiple drivers are accessing GPIO, the
> latter does not seem safe against preemption (assuming the memory
> read-modify-write is not atomic).
> 
> Shouldn't GPIO access be protected against concurrent access here?

Well, against an IRQ in the middle of those read/modify/write
sequences hidden by the "|=" and "&=" syntax.  Last I knew,
no XScale CPUs support on-chip SMP.


> Documentation/gpio.txt does not really mention the locking mechanism
> assumed to modify GPIO lines.

That function isn't part of the GPIO interface, despite
its gpio_* prefix, so that's not relevant.

It resembles gpio_set_value() though.  That can use at
most spinlocks to establish its atomicity guarantee; it's
described as "spinlock-safe", and in distinction to the
gpio_set_value_cansleep() call which could use a mutex or
other sleeping synch primitive.

Re: Locking in the (now generic) GPIO infrastructure?

[Leon Woestenberg]

Hello David, all,

On Fri, Jun 6, 2008 at 2:53 PM, David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 04 June 2008, Leon Woestenberg wrote:
>> include/asm-arm/arch-ixp4xx/platform.h:
>> static inline void gpio_line_set(u8 line, int value)
>> {
>>         if (value == IXP4XX_GPIO_HIGH)
>>             *IXP4XX_GPIO_GPOUTR |= (1 << line);
>>         else if (value == IXP4XX_GPIO_LOW)
>>             *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
>> }
>>
>> Under a Linux kernel where multiple drivers are accessing GPIO, the
>> latter does not seem safe against preemption (assuming the memory
>> read-modify-write is not atomic).
>>
>> Shouldn't GPIO access be protected against concurrent access here?
>
> Well, against an IRQ in the middle of those read/modify/write
> sequences hidden by the "|=" and "&=" syntax.  Last I knew,
> no XScale CPUs support on-chip SMP.
>
Indeed, however, I used a kernel with -rt patch (and using PREEMPT RT)
as mentioned in my original e-mail. For completeness I should have
stated this:

The interrupt handlers become kernel threads.
As such they become preemptable (to reduce latencies for any higher
priority threads, such as those from other interrupts or even RT user
tasks).

>> Documentation/gpio.txt does not really mention the locking mechanism
>> assumed to modify GPIO lines.
>
> That function isn't part of the GPIO interface, despite
> its gpio_* prefix, so that's not relevant.
>
> It resembles gpio_set_value() though.  That can use at
>
In fact, on the IXP4xx, gpio_set_value() is just gpio_line_set(), so I
think it is valid to understand where the locking should occur (lowest
level, higher level?)

> most spinlocks to establish its atomicity guarantee; it's
> described as "spinlock-safe", and in distinction to the
> gpio_set_value_cansleep() call which could use a mutex or
> other sleeping synch primitive.
>

So, the solution (for the upstream work on -rt) would be to add
spinlock protection to gpio_line_set(), mutex protection for
_cansleep() variants?

Regards,
-- 
Leon

Re: Locking in the (now generic) GPIO infrastructure?

[David Brownell]

On Friday 06 June 2008, Leon Woestenberg wrote:
> In fact, on the IXP4xx, gpio_set_value() is just gpio_line_set(),

OK, so the generic GPIO calls are inheriting a locking bug
from the older code.  Calls to the older stuff should probably
start getting phased out.


> so I 
> think it is valid to understand where the locking should occur (lowest
> level, higher level?)
> 
> > most spinlocks to establish its atomicity guarantee; it's
> > described as "spinlock-safe", and in distinction to the
> > gpio_set_value_cansleep() call which could use a mutex or
> > other sleeping synch primitive.
> >
> 
> So, the solution (for the upstream work on -rt) would be to add
> spinlock protection to gpio_line_set(), mutex protection for
> _cansleep() variants?

Given this is on -RT, yes it seems like a spinlock is needed
to protect against preemption (but not against concurrency,
which those XScale chips don't provide).

Though with that addition, the size of that function exceeds
what I'd call appropriate to inline.

- Dave

Re: Locking in the (now generic) GPIO infrastructure?

[Mikael Pettersson]

David Brownell writes:
 > On Wednesday 04 June 2008, Leon Woestenberg wrote:
 > > include/asm-arm/arch-ixp4xx/platform.h:
 > > static inline void gpio_line_set(u8 line, int value)
 > > {
 > >         if (value == IXP4XX_GPIO_HIGH)
 > >             *IXP4XX_GPIO_GPOUTR |= (1 << line);
 > >         else if (value == IXP4XX_GPIO_LOW)
 > >             *IXP4XX_GPIO_GPOUTR &= ~(1 << line);
 > > }
 > > 
 > > Under a Linux kernel where multiple drivers are accessing GPIO, the
 > > latter does not seem safe against preemption (assuming the memory
 > > read-modify-write is not atomic).
 > > 
 > > Shouldn't GPIO access be protected against concurrent access here?
 > 
 > Well, against an IRQ in the middle of those read/modify/write
 > sequences hidden by the "|=" and "&=" syntax.  Last I knew,
 > no XScale CPUs support on-chip SMP.

The IOP342 has two XScale cores on-chip. However, these cores are
still only ARMv5TE and Linux wants ARMv6 or newer for SMP, so I
don't know to what extent the IOP342 is supported by Linux.