Re: [PATCH v9 0/9] i2c mux cleanup and locking update

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

On Wed, May 04, 2016 at 10:15:26PM +0200, Peter Rosin wrote:
> Hi!
> 
> I have a pair of boards with this i2c topology:
> 
>                        GPIO ---|  ------ BAT1
>                         |      v /
>    I2C  -----+------B---+---- MUX
>              |                   \
>            EEPROM                 ------ BAT2
> 
> 	(B denotes the boundary between the boards)
> 
> The problem with this is that the GPIO controller sits on the same i2c bus
> that it MUXes. For pca954x devices this is worked around by using unlocked
> transfers when updating the MUX. I have no such luck as the GPIO is a general
> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
> of the fact that it is muxing an i2c bus. Extending unlocked transfers
> into the GPIO subsystem is too ugly to even think about. But the general hw
> approach is sane in my opinion, with the number of connections between the
> two boards minimized. To put it plainly, I need support for it.
> 
> So, I observe that while it is needed to have the i2c bus locked during the
> actual MUX update in order to avoid random garbage on the slave side, it
> is not strictly a must to have it locked over the whole sequence of a full
> select-transfer-deselect operation. The MUX itself needs to be locked, so
> transfers to clients behind the mux are serialized, and the MUX needs to be
> stable during all i2c traffic (otherwise individual mux slave segments
> might see garbage).
> 
> This series accomplishes this by adding code to i2c-mux-gpio and
> i2c-mux-pinctrl that determines if all involved devices used to update the
> mux are controlled by the same root i2c adapter that is muxed. When this
> is the case, the select-transfer-deselect operations should be locked
> individually to avoid the deadlock. The i2c bus *is* still locked
> during muxing, since the muxing happens as part of i2c transfers. This
> is true even if the MUX is updated with several transfers to the GPIO (at
> least as long as *all* MUX changes are using the i2c master bus). A lock
> is added to i2c adapters that muxes on that adapter grab, so that transfers
> through the muxes are serialized.
> 
> Concerns:
> - The locking is perhaps too complex?
> - I worry about the priority inheritance aspect of the adapter lock. When
>   the transfers behind the mux are divided into select-transfer-deselect all
>   locked individually, low priority transfers get more chances to interfere
>   with high priority transfers.
> - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(),
>   there is a higher possibility that the mux is not returned to its idle
>   state after a failed (-EAGAIN) transfer due to trylock.
> - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the
>   usage of the new i2c_root_adapter() function in 18/24)?
> 
> The first half (patches 01-15 in v7) of what was originally part of this
> series have already been scheduled for 4.6. So, this is the second half
> (patches 16-24 in v7, patches 1-9 in v9).
> 
> To summarize the series, there is some preparatory locking changes in
> in 1/9 and the real meat is in 3/9. There is some documentation added in
> 4/9 while 5/9 and after are cleanups to existing drivers utilizing
> the new stuff.
> 
> PS. needs a bunch of testing, I do not have access to all the involved hw.
> 
> This second half of the series is planned to be merged with 4.7 and can
> also be pulled from github, if that is preferred:
> 

Applied all to for-next, thanks for keeping at it!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]