[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCH] i2c: mux: pca954x: use relaxed locking of the top i2c adapter during mux-locked muxing
From:       Peter Rosin <peda () axentia ! se>
Date:       2018-04-27 11:43:23
Message-ID: a90dde83-3299-3046-0e49-0a398b1b2404 () axentia ! se
[Download RAW message or body]

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?

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?

*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?

These are the kinds of cross-device interactions on the i2c bus that
you should be looking for...

>>> 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@pengutronix.de>
>>> ---
>>>   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.

Cheers,
Peter
[prev in list] [next in list] [prev in thread] [next in thread]