Hi Peter,
On 04/27/2018 01:43 PM, Peter Rosin wrote:
On 2018-04-27 12:38, Bastian Stender wrote:
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?
What are the GPIO expanders used for? Is the PMIC perhaps involved
with some device on the other side of the mux and that's what's
triggering the deadlock?
I was wrong, only one GPIO expander is on the bus. It is used for some
GPIO buttons, no interaction with other devices here. The PMIC is not
involved either.
Have you tried running with lockdep? I don't think it will tell you
all that much since the i2c locks are rt_mutexes and are not tracked
IIRC, but maybe?
Yes, I tried running with lockdep but did not see anything.
*time passes*
Looking at the driver for at24c32 I see that the WP pin may be
controlled by a gpio driver. Is the eeprom WP-pins perhaps connected
to the gpio-expanders? Could that lead to a dead-lock somewhere?
No, the WP pin is hardwired. And I did not even try to write to the
EEPROM, only read operations.
These are the kinds of cross-device interactions on the i2c bus that
you should be looking for...
After reviewing the schematics again I could not find anything
unfortunately.
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.
If you can't, mux-locked could perhaps be made optional with a DT
property or something. See the binding for i2c-mux-gpmux for
precedent.
Sounds good. I will change the patch to handle opt-in 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
