Re: [PATCH v2 8/8] i2c-mux: relax locking of the top i2c adapter during i2c controlled muxing

2016-01-07 Thread Peter Rosin 
Hi Rob,

On 2016-01-06 15:49, Rob Herring wrote:
> On Tue, Jan 05, 2016 at 04:57:18PM +0100, Peter Rosin wrote:
>> From: Peter Rosin 
>>
>> With a i2c topology like the following
>>
>>GPIO ---|  -- BAT1
>> |  v /
>>I2C  -+--+ MUX
>>  |   \
>>EEPROM -- BAT2
> 
> Yuck. One would think you would just use an I2C controlled mux in this 
> case...
>  
>> there is a locking problem with the GPIO controller since it is a client
>> on the same i2c bus that it muxes. Transfers to the mux clients (e.g. BAT1)
>> will lock the whole i2c bus prior to attempting to switch the mux to the
>> correct i2c segment. In the above case, the GPIO device is an I/O expander
>> with an i2c interface, and since the GPIO subsystem knows nothing (and
>> rightfully so) about the lockless needs of the i2c mux code, this results
>> in a deadlock when the GPIO driver issues i2c transfers to modify the
>> mux.
>>
>> So, observing 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 mux client 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, or worse).
>>
>> Add devive tree properties (bool named i2c-controlled) to i2c-mux-gpio and
>> i2c-mux-pinctrl that asserts that the the gpio/pinctrl is controlled via
>> the same i2c bus that it muxes.
> 
> Can't you determine this condition by checking the mux parent and gpio 
> parent are the same i2c controller?

Good suggestion, I wrote code that implements this, and will include it in
v3. Do not expect v3 to hit the dt crowd though, since no dt changes will
be needed then, but I'm sure that is not a problem...

> Alternatively, can't you just always do the locking like i2c-controlled 
> is set when a mux is involved? What is the harm in doing that if the 
> GPIO is controlled somewhere else?

No, that is not possible. If you change a non-i2c-controlled gpio in the
middle of some i2c-access, the slave side of the mux might see partial
i2c transfers, and that is a recipe for disaster.

> I would prefer to see a solution not requiring DT updates to fix and 
> this change seems like it is working around kernel issues.

Right, I'll make it so.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/8] i2c-mux: relax locking of the top i2c adapter during i2c controlled muxing

2016-01-06 Thread Rob Herring 
On Tue, Jan 05, 2016 at 04:57:18PM +0100, Peter Rosin wrote:
> From: Peter Rosin 
> 
> With a i2c topology like the following
> 
>GPIO ---|  -- BAT1
> |  v /
>I2C  -+--+ MUX
>  |   \
>EEPROM -- BAT2

Yuck. One would think you would just use an I2C controlled mux in this 
case...
 
> there is a locking problem with the GPIO controller since it is a client
> on the same i2c bus that it muxes. Transfers to the mux clients (e.g. BAT1)
> will lock the whole i2c bus prior to attempting to switch the mux to the
> correct i2c segment. In the above case, the GPIO device is an I/O expander
> with an i2c interface, and since the GPIO subsystem knows nothing (and
> rightfully so) about the lockless needs of the i2c mux code, this results
> in a deadlock when the GPIO driver issues i2c transfers to modify the
> mux.
> 
> So, observing 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 mux client 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, or worse).
> 
> Add devive tree properties (bool named i2c-controlled) to i2c-mux-gpio and
> i2c-mux-pinctrl that asserts that the the gpio/pinctrl is controlled via
> the same i2c bus that it muxes.

Can't you determine this condition by checking the mux parent and gpio 
parent are the same i2c controller?

Alternatively, can't you just always do the locking like i2c-controlled 
is set when a mux is involved? What is the harm in doing that if the 
GPIO is controlled somewhere else?

I would prefer to see a solution not requiring DT updates to fix and 
this change seems like it is working around kernel issues.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/8] i2c-mux: relax locking of the top i2c adapter during i2c controlled muxing

2016-01-05 Thread Peter Rosin 
From: Peter Rosin 

With a i2c topology like the following

   GPIO ---|  -- BAT1
|  v /
   I2C  -+--+ MUX
 |   \
   EEPROM -- BAT2

there is a locking problem with the GPIO controller since it is a client
on the same i2c bus that it muxes. Transfers to the mux clients (e.g. BAT1)
will lock the whole i2c bus prior to attempting to switch the mux to the
correct i2c segment. In the above case, the GPIO device is an I/O expander
with an i2c interface, and since the GPIO subsystem knows nothing (and
rightfully so) about the lockless needs of the i2c mux code, this results
in a deadlock when the GPIO driver issues i2c transfers to modify the
mux.

So, observing 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 mux client 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, or worse).

Add devive tree properties (bool named i2c-controlled) to i2c-mux-gpio and
i2c-mux-pinctrl that asserts that the the gpio/pinctrl is controlled via
the same i2c bus that it muxes.

Modify the i2c mux locking so that muxes that are "i2c-controlled" locks
the mux instead of the whole i2c bus when there is a transfer to the slave
side of the mux. This lock serializes transfers to the slave side of the
mux.

Modify the select-transfer-deselect code for "i2c-controlled" muxes so
that each of the select-transfer-deselect ops locks the mux parent
adapter individually.

Signed-off-by: Peter Rosin 
---
 .../devicetree/bindings/i2c/i2c-mux-gpio.txt   |   2 +
 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt|   4 +
 drivers/i2c/i2c-mux.c  | 109 +++--
 drivers/i2c/muxes/i2c-mux-gpio.c   |   3 +
 drivers/i2c/muxes/i2c-mux-pinctrl.c|   3 +
 include/linux/i2c-mux-gpio.h   |   2 +
 include/linux/i2c-mux-pinctrl.h|   2 +
 include/linux/i2c-mux.h|   2 +
 8 files changed, 120 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt 
b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
index 66709a825541..17670b997d81 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
@@ -28,6 +28,8 @@ Required properties:
 Optional properties:
 - idle-state: value to set the muxer to when idle. When no value is
   given, it defaults to the last value used.
+- i2c-controlled: The muxed I2C bus is also used to control all the gpios
+  used for muxing.
 
 For each i2c child node, an I2C child bus will be created. They will
 be numbered based on their order in the device tree.
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt 
b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
index ae8af1694e95..8374a1f7a709 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
@@ -23,6 +23,10 @@ Required properties:
 - i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
   port is connected to.
 
+Optional properties:
+- i2c-controlled: The muxed I2C bus is also used to control all the pinctrl
+  pins used for muxing.
+
 Also required are:
 
 * Standard pinctrl properties that specify the pin mux state for each child
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index dd8a790cb4cc..c4d4b14a5399 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -54,6 +54,25 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
return ret;
 }
 
+static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
+struct i2c_msg msgs[], int num)
+{
+   struct i2c_mux_priv *priv = adap->algo_data;
+   struct i2c_mux_core *muxc = priv->muxc;
+   struct i2c_adapter *parent = muxc->parent;
+   int ret;
+
+   /* Switch to the right mux port and perform the transfer. */
+
+   ret = muxc->select(muxc, priv->chan_id);
+   if (ret >= 0)
+   ret = i2c_transfer(parent, msgs, num);
+   if (muxc->deselect)
+   muxc->deselect(muxc, priv->chan_id);
+
+   return ret;
+}
+
 static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
  u16 addr, unsigned short flags,
  char read_write, u8 command,
@@ -76,6 +95,28 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
return ret;
 }
 
+static int __i2c_mux_smbus_xfer(struct i2c_adapter