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
--

[ message continues ]

Definitely.  It's certainly buggy as currently stands.
--

[ message continues ]

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);

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.

--

[ message continues ]

Well, against an IRQ in the middle of those read/modify/write
sequences hidden by the "|=" and "&=" syntax.  Last I knew,

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.

--

[ message continues ]

Hello David, all,

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
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

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
--

[ message continues ]

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

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

--

[ message continues ]

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.
--

[ message continues ]