Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell <david-b@pacbell.net>]

On Monday 12 November 2007, Andrew Morton wrote:
> On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <david-b@pacbell.net> wrote:
> 
> > Provide new implementation infrastructure that platforms may choose to use
> > when implementing the GPIO programming interface.  ...
> > 
> > ...
> >
> > +/* gpio_lock protects the table of chips and to gpio_chip->requested.
> > + * While any gpio is requested, its gpio_chip is not removable.  It's
> > + * a raw spinlock to ensure safe access from hardirq contexts, and to
> > + * shrink bitbang overhead:  per-bit preemption would be very wrong.
> > + */
> > +static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;
> 
> Well that's weird.
> 
> For starters, this initialisation will confound lockdep: it should use
> DEFINE_SPINLOCK.

Then it wouldn't be "raw"!  It seems info on raw spinlocks is out
of sync with current code.  Allegedly it should be possible to just
pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends
and have GCC sort out the right stuff ... but that didn't work.

I speculate that either the design has changed (without fanfare),
or else that stuff is in RT kernels and has not yet gone upstream.


> And the rationale seems dubious.  All you're saving here is a couple of
> accesses to task_struct at spin_unlock()-time.  If the current task has a
> preemption pending then yes, we'll schedule away but that's a very rare
> thing and that's just what we're supposed to do.

Unfortunately, that's not what I observed/measured.

  http://marc.info/?l=linux-kernel&m=119429680220361&w=2

Plus, note the comment about hardirq context and RT environments.
When that spinlock is really a mutex, and the non-hardirq contexts
are tasks, non-raw spinlocks would wrongly prevent access to GPIOs
from true hardirq contexts.


> So please tell us more about this.  Perhaps there are performance problems
> with the current core preemption machinery.

The above email summarizes significant slowdown I observed using
just i2c-gpio, which happened to be easy to measure down to the
microsecond level.


> > +	local_irq_save(flags);
> > +	__raw_spin_lock(&gpio_lock);
> >
> > ...
> > +	__raw_spin_unlock(&gpio_lock);
> > +	local_irq_restore(flags);
> > +	return status;
> > +}
> 
> And of course if this code is converted to conventional locking, the above
> becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

See above.  Allegedly we ought to be able to use those calls even
with raw spinlocks ... but that didn't work when I tried it. I'd
certainly prefer if it did work.


> > +/* There's no value in inlining GPIO calls that may sleep.
> 
> There's no value in inlining anything, hardly ;)

Remember that GPIOs do get used for bitbanging, and there's lots
of kernel code to make bit ops stay fast!

The canonical reason to inline GPIO operations is so that when
you bitbang some serial protocol, you're closer to one instruction
per bit toggle than fifty ... at one point I measured a factor of
nearly three speedup for byte-level bitbanging for SPI (ca. 70 Kbit
with quick function calls, vs about 2 Mbits inlined, ISTR) ... with
most of the overhead being to pass the next byte down to the bitbang
loop.  Using a bigger wordsize saw even more improvement.


> > +postcore_initcall(gpiolib_debugfs_init);
> 
> postcore_initcall() is unusual, hence a comment describing why it was
> employed would be a good thing to have.

Actually that one could easily be later, fs_initcall or whatnot.
I think that's debris from other versions.

- Dave