kern/47179: major issues with i2c locking (iic_acquire_bus/iic_release_bus)

[darkstar%city-net.com@localhost]

>Number:         47179
>Category:       kern
>Synopsis:       major issues with i2c locking (iic_acquire_bus/iic_release_bus)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Nov 10 22:45:00 +0000 2012
>Originator:     Matthew Orgass
>Release:        6.99.10
>Organization:
>Environment:
examination of source code
>Description:
Between the controller drivers and device drivers many types of errors in 
iic_acquire_bus and iic_release_bus usage are represented and few combinations 
are correct. I2C_F_POLL might always be handled incorrectly.

The return value of iic_acquire_bus is not documented in iic(9).  

The actual return values are inconsistent and sometimes contradictory between 
controllers.  

Some controller drivers seem to do something related to preparing the bus to be 
accessed in iic_acquire_bus but do not actually lock anything.  

Many controller drivers always use mutex_lock in iic_acquire_bus without 
checking for I2C_F_POLL.

Some controller drivers always return what they think means failure if 
I2C_F_POLL is specified to iic_acquire_bus.

Many device drivers do not check the return value of iic_acquire_bus even if 
they specified I2C_F_POLL.  Sometimes there is a layer of indirection that 
returns the return value, which is then usually ignored by callers.

Some drivers have indirected read/write functions which lock the bus, read or 
write, and then release the bus.  These functions are usually called many times 
in series, rather than locking the bus once and then reading and writing as 
needed and releasing when done.

The return value of iic_release_bus is documented as int but it looks like 
every controller driver returns void because that makes sense.  


>How-To-Repeat:
examine source code
>Fix:
I'll eventually submit a patch, but it is taking a while so I thought I would 
send-pr the problem so it isn't lost.  If someone wants to get to it faster, go 
for it.

Basically, all usage of iic_acquire_bus and iic_release_bus in both controller 
and device drivers needs to be checked and fixed and the documentation updated. 
 It seems like the return value of iic_acquire_bus should maybe be the return 
value of mutex_tryenter (although this is contrary to what seems to be the most 
common assumption now) and that it should be documented to always succeed if 
I2C_F_POLL is not specified.  An iic generic macro or function should probably 
be provided to DTRT in controller iic_acquire_bus functions.  Possibly, the 
mutex could just be put in struct i2c_controller and handled in a completely 
controller independent way, although that would be the only regularly modified 
data in that structure and a few controllers seem to want hooks at those points 
anyway.