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

2016-05-04 Thread Wolfram Sang
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!



signature.asc
Description: PGP signature


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

2016-05-04 Thread Peter Rosin
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:

-
The following changes since commit b3b85922534861445a24f48f1d6125a99c6f3aa2:

  Merge branch 'i2c/for-4.7' into i2c/for-next (2016-04-27 19:11:32 +0200)

are available in the git repository at:

  https://github.com/peda-r/i2c-mux.git mux-core-and-locking-9

for you to fetch changes up to ce3cf4098f0bb41fa9fc9a87bb5d4d64ec90074f:

  [media] rtl2832: regmap is aware of lockdep, drop local locking hack 
(2016-05-04 21:05:53 +0200)
-

v9 compared to v8:
- Drop patches 01-15, since they are in the pipe anyway.
- Change the name of the I2C_LOCK_ADAPTER flag to I2C_LOCK_ROOT_ADAPTER.
- Do not change the locking in i2c_slave_register and i2c_slave_unregister.
  That cannot possibly work for muxes anyway, so the changes were somewhere
  between pointless and silly.
- The the BIT macro to specify bits for flags.
- Add kerneldoc for i2c_lock_bus and i2c_unlock_bus.
- Add some comments in i2c_mux_trylock_bus and i2c_parent_trylock_bus.
- Use true instead of 1 for a bool variable.
- Whitespace issues fixed as indicated by --strict checkpatching.
- Updated commit message for the first patch.

v8 compared to v7:
- Do not change i2c_get_clientdata() into dev_get_drvdata() in the mpu6050
  driver.

v7 compared to v6:
- Removed i2c_mux_reserve_adapters, and all realloc attempts in
  i2c_mux_add_adapter. Supply a maximum number of adapters in i2c_mux_alloc
  instead.
- Removed i2c_mux_one_adapter since it is was hard to use correctly, which
  was evident from the crash in the mpu6050 driver (on a mpu9150 chip) reported
  by Crestez Dan Leonard. Also, it didn't make things all that much simpler
  anyway (even if used correctly).
- Rename