diff mbox series

[2/2] i2c: designware: abort the transfer if receiving byte count of 0

Message ID 20230811124624.12792-2-yann@sionneau.net
State New
Headers show
Series [1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low | expand

Commit Message

Yann Sionneau Aug. 11, 2023, 12:46 p.m. UTC 
From: Yann Sionneau <ysionneau@kalray.eu>

Context:
It's not clear whether Linux SMBus stack supports receiving 0
as byte count for SMBus read data block.

Linux supports SMBus v2.0 spec, which says "The byte count may not be 0."
Which does not seem very clear to me, as a non-native speaker.
(Note that v3.0 of the spec says "The byte count may be 0.")

Some drivers explicitly return -EPROTO in case of byte count 0.

The issue:
Regardless of whether Linux supports byte count of 0, if this happens
the i2c-designware driver goes into an unrecoverable state holding
SCL low if the IP is synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
parameter.

The fix proposed by this patch:
Abort the transfer by sending a STOP condition on the bus.

Another approach would be to ignore the issue and let the driver
timeout and disable the IP. The IP disabling is fixed by the previous
patch in this patch series.
The current patch just makes the recovery faster since Abort is sent
directly without waiting for the timeout to happen. With this patch,
disabling the IP is not necessary anymore.

Co-developed-by: Jonathan Borne <jborne@kalray.eu>
Signed-off-by: Jonathan Borne <jborne@kalray.eu>
Tested-by: Yann Sionneau <ysionneau@kalray.eu>
Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
---
 drivers/i2c/busses/i2c-designware-master.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Aug. 11, 2023, 2:01 p.m. UTC | #1 
On Fri, Aug 11, 2023 at 02:46:24PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> Context:
> It's not clear whether Linux SMBus stack supports receiving 0
> as byte count for SMBus read data block.
> 
> Linux supports SMBus v2.0 spec, which says "The byte count may not be 0."
> Which does not seem very clear to me, as a non-native speaker.
> (Note that v3.0 of the spec says "The byte count may be 0.")
> 
> Some drivers explicitly return -EPROTO in case of byte count 0.
> 
> The issue:
> Regardless of whether Linux supports byte count of 0, if this happens
> the i2c-designware driver goes into an unrecoverable state holding
> SCL low if the IP is synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
> parameter.
> 
> The fix proposed by this patch:
> Abort the transfer by sending a STOP condition on the bus.
> 
> Another approach would be to ignore the issue and let the driver
> timeout and disable the IP. The IP disabling is fixed by the previous
> patch in this patch series.
> The current patch just makes the recovery faster since Abort is sent
> directly without waiting for the timeout to happen. With this patch,
> disabling the IP is not necessary anymore.

...

> +				if ((tmp <= I2C_SMBUS_BLOCK_MAX) && (tmp != 0))

				if (tmp && tmp <= I2C_SMBUS_BLOCK_MAX)

> +					len = i2c_dw_recv_len(dev, tmp);
> +				else
> +					i2c_dw_abort(dev);
Yann Sionneau Aug. 17, 2023, 2:42 p.m. UTC | #2 
Hi,

Le 11/08/2023 à 16:01, Andy Shevchenko a écrit :
> On Fri, Aug 11, 2023 at 02:46:24PM +0200, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@kalray.eu>
>>
>> Context:
>> It's not clear whether Linux SMBus stack supports receiving 0
>> as byte count for SMBus read data block.
>>
>> Linux supports SMBus v2.0 spec, which says "The byte count may not be 0."
>> Which does not seem very clear to me, as a non-native speaker.
>> (Note that v3.0 of the spec says "The byte count may be 0.")
>>
>> Some drivers explicitly return -EPROTO in case of byte count 0.
>>
>> The issue:
>> Regardless of whether Linux supports byte count of 0, if this happens
>> the i2c-designware driver goes into an unrecoverable state holding
>> SCL low if the IP is synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
>> parameter.
>>
>> The fix proposed by this patch:
>> Abort the transfer by sending a STOP condition on the bus.
>>
>> Another approach would be to ignore the issue and let the driver
>> timeout and disable the IP. The IP disabling is fixed by the previous
>> patch in this patch series.
>> The current patch just makes the recovery faster since Abort is sent
>> directly without waiting for the timeout to happen. With this patch,
>> disabling the IP is not necessary anymore.
> ...
>
>> +				if ((tmp <= I2C_SMBUS_BLOCK_MAX) && (tmp != 0))
> 				if (tmp && tmp <= I2C_SMBUS_BLOCK_MAX)

I find the tmp != 0 "more obvious", I am more used to "just tmp" when 
it's a pointer or a boolean, but maybe it's just me!

I'll fix that in the V2 :)

>
>> +					len = i2c_dw_recv_len(dev, tmp);
>> +				else
>> +					i2c_dw_abort(dev);
Thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index b55c19b2515a..fa5301566bb1 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -499,6 +499,15 @@  i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
 	return len;
 }
 
+static void
+i2c_dw_abort(struct dw_i2c_dev *dev)
+{
+	u32 enable;
+
+	regmap_read(dev->map, DW_IC_ENABLE, &enable);
+	regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
+}
+
 static void
 i2c_dw_read(struct dw_i2c_dev *dev)
 {
@@ -526,11 +535,16 @@  i2c_dw_read(struct dw_i2c_dev *dev)
 			u32 flags = msgs[dev->msg_read_idx].flags;
 
 			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
+			tmp &= DW_IC_DATA_CMD_DAT;
+
 			/* Ensure length byte is a valid value */
-			if (flags & I2C_M_RECV_LEN &&
-			    (tmp & DW_IC_DATA_CMD_DAT) <= I2C_SMBUS_BLOCK_MAX && (tmp & DW_IC_DATA_CMD_DAT) > 0) {
-				len = i2c_dw_recv_len(dev, tmp);
+			if (flags & I2C_M_RECV_LEN) {
+				if ((tmp <= I2C_SMBUS_BLOCK_MAX) && (tmp != 0))
+					len = i2c_dw_recv_len(dev, tmp);
+				else
+					i2c_dw_abort(dev);
 			}
+
 			*buf++ = tmp;
 			dev->rx_outstanding--;
 		}