On 18. 02. 20 14:58, Laine Jaakko EXT wrote: > I2C master operating in multimaster mode can get stuck > indefinitely if I2C start is detected on bus, but no master > has a transaction going. > > This is a weakness in I2C standard, which defines no way > to recover, since all masters are indefinitely disallowed > from interrupting the currently operating master. A start > condition can be created for example by an electromagnetic > discharge applied near physical I2C lines. Or a already > operating master could get reset immediately after sending > a start. > > If it is known during device tree creation that only a single > I2C master will be present on the bus, this deadlock of the > I2C bus could be avoided in the driver by ignoring the > bus_is_busy register of the xiic, since bus can never be > reserved by any other master. > > This patch adds this support for detecting multi-master flag > in device tree and when not provided, improves I2C reliability > by ignoring the therefore unnecessary xiic bus_is_busy register. > > Error can be reproduced by pulling I2C SDA -line temporarily low > by shorting it to ground, while linux I2C master is operating on > it using the xiic driver. The application using the bus will > start receiving linux error code 16: "Device or resource busy" > indefinitely: > > kernel: pca953x 0-0020: failed writing register > app: Error writing file, error: 16 > > With multi-master disabled device will instead receive error > code 5: "I/O error" while SDA is grounded, but recover normal > operation once short is removed. > > kernel: pca953x 0-0020: failed reading register > app: Error reading file, error: 5 > > Signed-off-by: Jaakko Laine <ext-jaakko.laine@xxxxxxxxxxx> > --- > > Applies against Linux 5.6-rc1 from master in > https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git > > I would like to point out that that since this patch disables > multimaster mode based on the standard I2C multimaster property > in device tree (as it propably should) and since the driver has > previously supported multimaster even when this property doesn't > exist in device tree, there is a possible backwards > compatibility issue: > > If there are devices relying on the multimaster mode to work > without defining the property in device tree, their I2C bus > might not work without issues anymore after this patch, since > the driver will asume it is the only master on bus and could > therefore interrupt the communication of some other master on > same bus. > > Please suggest some alternative fix if this is not acceptable > as is. On the other hand supporting multimaster even on a bus > with only a single master does currently cause some > reliability issues since the bus can get indefinitely stuck. > I don't think there exists a I2C protocol compatible way to > resolve the deadlock on multimaster bus. Wolfram: I don't think this feature is used on this driver a lot but clearly this breaks compatibility. Not sure how to handle this properly and I am fine with this solution. Shubhrajyoti: Any comment? > > drivers/i2c/busses/i2c-xiic.c | 52 +++++++++++++++++++++-------------- > 1 file changed, 32 insertions(+), 20 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c > index 90c1c362394d..37f8d6ee0577 100644 > --- a/drivers/i2c/busses/i2c-xiic.c > +++ b/drivers/i2c/busses/i2c-xiic.c > @@ -46,19 +46,20 @@ enum xiic_endian { > > /** > * struct xiic_i2c - Internal representation of the XIIC I2C bus > - * @dev: Pointer to device structure > - * @base: Memory base of the HW registers > - * @wait: Wait queue for callers > - * @adap: Kernel adapter representation > - * @tx_msg: Messages from above to be sent > - * @lock: Mutual exclusion > - * @tx_pos: Current pos in TX message > - * @nmsgs: Number of messages in tx_msg > - * @state: See STATE_ > - * @rx_msg: Current RX message > - * @rx_pos: Position within current RX message > - * @endianness: big/little-endian byte order > - * @clk: Pointer to AXI4-lite input clock > + * @dev: Pointer to device structure > + * @base: Memory base of the HW registers > + * @wait: Wait queue for callers > + * @adap: Kernel adapter representation > + * @tx_msg: Messages from above to be sent > + * @lock: Mutual exclusion > + * @tx_pos: Current pos in TX message > + * @nmsgs: Number of messages in tx_msg > + * @state: See STATE_ > + * @rx_msg: Current RX message > + * @rx_pos: Position within current RX message > + * @endianness: big/little-endian byte order > + * @multimaster: Indicates bus has multiple masters > + * @clk: Pointer to AXI4-lite input clock nit: I can't see reason for these changes above. I would do it in separate patch if you want to align. Thanks, Michal
