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

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

On Wednesday 14 November 2007, Thomas Gleixner wrote:
> On Mon, 12 Nov 2007, David Brownell wrote:
> > > I'm still trying to understand what you've observed here.  Is it the case
> > > that a single gpio operation went from 6.4 up to 11.2 usecs?
> > 
> > That was a single bitbanged I2C bit transfer, with embedded udelay()s.
> > I believe that was four gpio operations, as summarized at the end of
> > that email above.  Enabling preempt + debug increased the cost of
> > each GPIO call from whatever it was (reasonable) by 1.2 usecs.
> 
> This raw lock change is just pampering over the design problem of the
> gpio lib:
> 
> There is no need to check for every single access to a GPIO pin,
> whether the pin has a valid number and the chip, which provides access
> to the pin, is still registered.

As Haavard had noted.  The "requested" flag is actually serving
as a longterm bit-level lock, which -- assuming well-behaved
callers, and no debug instrumentation -- obviates any need to
grab a spinlock in hot paths.


> Each driver, which wants to access a pin, needs to make sure that
> 
> - the pin is available
> - the pin is associated to this driver
> - the chip reference count is incremented
> 
> _before_ it starts to do anything with the pin. Once this is done the
> access to the pin is completely lock free except for the protection of
> the chip hardware itself.

That's what the gpio_request() call does, although it's using
something isomorphic to a refcount, not an actual refcount.

The key observation here is that we already *have* a bit which
is serving as a per-gpio lock.  It's just never been viewed as
a lock before.  :)


> The protection of the chip list can be converted to a mutex and
> does not need to be a spinlock at all.

No, we still need to use a spinlock to protect table changes.
The reason for that is briefly:

  - gpio_request()/gpio_free() have so far been optional.  Most
    platforms implement them as NOPs, not all drivers use them.
    (Having gpiolib in place should help change that ...)

  - gpio_direction_input()/gpio_direction_output() implicitly
    request the pins, if they weren't already requested.

  - Those input/output direction-setting calls may be called
    in IRQ contexts, which means (on non-RT kernels) no mutex.


So we're actually in good shape; just take out a bit of code
(or turn it into debugging instrumentation) and I don't think
anyone will complain about the locking any more.

- Dave