Updated switch/glue patch?

[Adrian Chadd]

On 18 December 2011 02:17, Stefan Bethke <stb at lassitu.de> wrote:
>> Erm, surely that's a bit ridiculous.. surely the locking doesn't need
>> to be that fine grained _and_ multi-levelled. There has to be a better
>> way to do this. :)
>
> Exactly.
>
> I've reimplemented iicbb.c to be slightly more protocol compliant, and to be able to tune transfer speeds to the actual hardware capabilities.  I've timed a single GETSCL (with WITNESS) to 8.7 microseconds, clearly that won't do.
>
> I think I'll look at gpio next, as you have, and see if the overhead can be reduced.

I'm just hacking up gpio and gpioiic at the moment. gpiobus already
_has_ locks for access to the bus, so as long as gpioiic actually
grabs the gpio bus for the duration of the transfer, the individual
GPIO operations shouldn't require locks.

Same deal with gpioiic and the gpioiic locks being used for each
sda/scl operation. The bus should already be acquired.

With those locks removed (and I haven't committed that to my git tree,
I have no idea what the implications are), things certainly look like
they're behaving better. There are still the occasional spike in CPU
time: this is a vmstat 0.1:

# vmstat 0.1
 procs      memory      page                    disks     faults         cpu
 r b w     avm    fre   flt  re  pi  po    fr  sr fl0 md0   in   sy
cs us sy id
 0 0 0  62832k  7536k   212   1   2   0   137   0   0   0    0  182
265  2 21 77
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
640  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
430  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
350  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
410  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
440  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
350  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
410  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
450  0 58 42
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
460  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
400  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
370  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
370  0  8 92
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
400  0  0 100
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
470  8  0 92
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
460  0  8 92
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
460  0  8 92
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
510  0 14 86
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
450  0 61 39
 0 0 0  62832k  7536k     0   0   0   0     0   0   0   0    0 1070
500  0  8 92

.. but notice that it uses a lot of CPU in a short burst, rather than
actually tying up the CPU for almost a second. I still don't like it
(it's only doing a couple hundred register accesses a second over the
gpio bus, surely it shouldn't take that much CPU..) but at least now
it's behaving correctly.

So now - why are the gpioiic and ar71xx_gpio locks even required? If
all consumers of the gpio bus code actually use the gpiobus
lock/unlock device methods correctly, surely that should be enough?

I may actually add a couple of debug device methods to allow a driver
to effectively call BLAH_LOCK_ASSERT() and BLAH_UNLOCK_ASSERT(). That
way the GPIO driver code can ensure that the gpio bus is actually
locked whenever an operation is done. Similarly, the i2c bus code can
ensure that the gpio bus it's hanging off is also locked. That should
capture any stupid crap from occuring.

G'night, and thanks very much for looking into this. :)


Adrian