Re: [PATCH] i2c: mux: pca954x: use relaxed locking of the top i2c adapter during mux-locked muxing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Peter,

On 04/27/2018 11:25 AM, Peter Rosin wrote:
On 2018-04-27 10:47, Bastian Stender wrote:
With an i2c device behind a PCA9540 mux having CONFIG_I2C_DEBUG_BUS enabled
connection timeouts caused by a busy i2c bus can be observed:

   i2c i2c-3: master_xfer[0] W, addr=0x57, len=2
   i2c i2c-3: master_xfer[1] R, addr=0x57, len=128
   i2c i2c-2: <i2c_imx_xfer>
   i2c i2c-2: <i2c_imx_start>
   i2c i2c-2: <i2c_imx_bus_busy>
   i2c i2c-2: <i2c_imx_bus_busy> I2C bus is busy
   i2c i2c-2: <i2c_imx_xfer> exit with: error: -110

This happens due to the locking problem described in 6ef91fcca8a8
("i2c: mux: relax locking of the top i2c adapter during mux-locked muxing"):

The cause of the problem is that the mux is a i2c client on the same i2c bus
that it muxes. Transfers to the mux clients will lock the whole i2c bus prior
to attempting to switch the mux to the correct i2c segment.

The mentioned commit introduced a new locking concept as "mux-locked"
muxes so that these muxes lock only the muxes on the parent adapter
instead of the whole i2c bus when there is a transfer to the slave side of
the mux.

This can't be the whole story, since the pca954x driver carefully uses
the unlocked __i2c_transfer. So, what device is it that sits behind the
mux that runs into this problem? And what do your i2c topology look
like?

There are multiple EEPROMs (AT24C32D) and gpio expanders (PCF8575)
behind the mux:

                             EEPROM  EEPROM  EEPROM   EEPROM
                               |        |       |       |
         RTC     TMP          -+--------+-------+-------+--
          |       |          /
I2C  --+--+---+---+----+-- MUX  GPIO_EXPANDER     EEPROM
       |      |        |     \         |             |
      PMIC  EEPROM    ADC     --+------+--+-------+--+---
                                |         |       |
                              EEPROM    EEPROM  GPIO_EXPANDER

I suspect your deadlocking device has some kind of interaction with some
other device on the some root i2c bus as the mux. I.e. you should not see
a deadlock because of the i2c interaction to update the mux itself, but
because of some other i2c interaction that isn't unlocked.

I am not aware of any interaction between the devices. Without the patch
I see the EEPROMs getting probed correctly. Within the probe function
i2c reads are performed and work. After that I can interact with the
devices on the bus, but not with the devices behind the mux.

Sometimes I see:

$ hexdump /sys/bus/i2c/devices/3-0057/eeprom
hexdump: /sys/bus/i2c/devices/3-0057/eeprom: Connection timed out

Sometimes the whole system just hangs, I do not see any cause/log messages.

The patch prevents all that. Any idea what could cause this?

Make use of this new locking concept: use the introduced flag I2C_MUX_LOCKED
along with lock-aware i2c_transfer() instead of __i2c_transfer().


Signed-off-by: Bastian Stender <bst@xxxxxxxxxxxxxx>
---
  drivers/i2c/muxes/i2c-mux-pca954x.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 09bafd3e68fa..0ea970eaa324 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -230,7 +230,7 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
  		msg.len = 1;
  		buf[0] = val;
  		msg.buf = buf;
-		ret = __i2c_transfer(adap, &msg, 1);
+		ret = i2c_transfer(adap, &msg, 1);

This is not complete, since you do not fix the unlocked SMBus access below,
which you must do if you switch to mux-locked.

Oh, right. I am always getting into the if path, so the missing else
path did not strike me.

However, *IF* this driver can be changed to be mux-locked (which it probably
can't, there are implications...) the whole of pca954x_reg_write should be
removed and call sites updated with regular i2c_smbus_write_byte calls (or
whatever is appropriate, I didn't check all that carefully).

I would love it if this was possible, and it is for the simple cases which
is somewhat annoying. But if you consider some of the more complex examples
in Documentation/i2c/i2c-topology you will see that it probably can't be
done without causing problems elsewhere.

Okay, I see. I will do my best to make it work without mux-locked then.

Regards,
Bastian

--
Pengutronix e.K.
Industrial Linux Solutions
http://www.pengutronix.de/
Peiner Str. 6-8, 31137 Hildesheim, Germany
Amtsgericht Hildesheim, HRA 2686



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux