[PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[Yann Sionneau]

From: Yann Sionneau <ysionneau@kalray.eu>

The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
parameter.
In which case, if the TX FIFO gets empty and the last command didn't have
the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until
a new command is pushed into the TX FIFO or the transfer is aborted.

When the dw_apb_i2c is holding SCL low, it cannot be disabled.
The transfer must first be aborted.
Also, the bus recover won't work because SCL is held low by the master.

This patch checks if the master is holding SCL low in __i2c_dw_disable
before trying to disable the IP.
If SCL is held low, an abort is initiated.
When the abort is done, the disabling can then proceed.

This whole situation can happen for instance during SMBUS read data block
if the slave just responds with "byte count == 0".
This puts the master in an unrecoverable state, holding SCL low and the
current __i2c_dw_disable procedure is not working. In this situation
only a Linux reboot can fix the i2c bus.

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-common.c | 17 +++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h   |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 9f8574320eb2..744927b0c5af 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -440,8 +440,25 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	int timeout = 100;
 	u32 status;
+	u32 raw_intr_stats;
+	u32 enable;
+	bool abort_needed;
+	bool abort_done = false;
+
+	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
+	regmap_read(dev->map, DW_IC_ENABLE, &enable);
+
+	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
+	if (abort_needed)
+		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
 
 	do {
+		if (abort_needed && !abort_done) {
+			regmap_read(dev->map, DW_IC_ENABLE, &enable);
+			abort_done = !(enable & DW_IC_ENABLE_ABORT);
+			continue;
+		}
+
 		__i2c_dw_disable_nowait(dev);
 		/*
 		 * The enable status register may be unimplemented, but
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 19ae23575945..dcd9bd9ee00f 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -98,6 +98,7 @@
 #define DW_IC_INTR_START_DET	BIT(10)
 #define DW_IC_INTR_GEN_CALL	BIT(11)
 #define DW_IC_INTR_RESTART_DET	BIT(12)
+#define DW_IC_INTR_MST_ON_HOLD	BIT(13)
 
 #define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
 					 DW_IC_INTR_TX_ABRT | \
@@ -109,6 +110,8 @@
 					 DW_IC_INTR_RX_UNDER | \
 					 DW_IC_INTR_RD_REQ)
 
+#define DW_IC_ENABLE_ABORT		BIT(1)
+
 #define DW_IC_STATUS_ACTIVITY		BIT(0)
 #define DW_IC_STATUS_TFE		BIT(2)
 #define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
-- 
2.17.1

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

[Yann Sionneau]

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(-)

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--;
 		}
-- 
2.17.1

Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN

DesignWare

> parameter.
> In which case, if the TX FIFO gets empty and the last command didn't have

"In this case when the..."

> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until

"the controller will..."

> a new command is pushed into the TX FIFO or the transfer is aborted.
> 
> When the dw_apb_i2c is holding SCL low, it cannot be disabled.

"When the controller..."

> The transfer must first be aborted.
> Also, the bus recover won't work because SCL is held low by the master.
> 
> This patch checks if the master is holding SCL low in __i2c_dw_disable

Grep for "This patch" in the Submitting Patches document and fix this
accordingly.

__i2c_dw_disable()

> before trying to disable the IP.
> If SCL is held low, an abort is initiated.
> When the abort is done, the disabling can then proceed.
> 
> This whole situation can happen for instance during SMBUS read data block
> if the slave just responds with "byte count == 0".
> This puts the master in an unrecoverable state, holding SCL low and the
> current __i2c_dw_disable procedure is not working. In this situation

__i2c_dw_disable()

> only a Linux reboot can fix the i2c bus.

If reboot helps, what magic does it do that Linux OS can't repeat in software?
Please, elaborate more.

...

>  	int timeout = 100;
>  	u32 status;
> +	u32 raw_intr_stats;
> +	u32 enable;
> +	bool abort_needed;
> +	bool abort_done = false;

Perhaps reversed xmas tree order?

	bool abort_done = false;
	bool abort_needed;
	u32 raw_intr_stats;
	int timeout = 100;
	u32 status;
	u32 enable;

...

> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
> +	if (abort_needed)
> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>  
>  	do {
> +		if (abort_needed && !abort_done) {
> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
> +			continue;

This will exhaust the timeout and below can be run at most once,
is it a problem?

Also it's a tight busyloop, are you sure it's what you need?

> +		}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[Yann Sionneau]

Hi,

Le 11/08/2023 à 15:59, Andy Shevchenko a écrit :
> On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@kalray.eu>
>>
>> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
> DesignWare
Ack!
>
>> parameter.
>> In which case, if the TX FIFO gets empty and the last command didn't have
> "In this case when the..."
Ack!
>
>> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until
> "the controller will..."
Ack!
>
>> a new command is pushed into the TX FIFO or the transfer is aborted.
>>
>> When the dw_apb_i2c is holding SCL low, it cannot be disabled.
> "When the controller..."
Ack!
>
>> The transfer must first be aborted.
>> Also, the bus recover won't work because SCL is held low by the master.
>>
>> This patch checks if the master is holding SCL low in __i2c_dw_disable
> Grep for "This patch" in the Submitting Patches document and fix this
> accordingly.
Ok I didn't know, ack!
>
> __i2c_dw_disable()
>
>> before trying to disable the IP.
>> If SCL is held low, an abort is initiated.
>> When the abort is done, the disabling can then proceed.
>>
>> This whole situation can happen for instance during SMBUS read data block
>> if the slave just responds with "byte count == 0".
>> This puts the master in an unrecoverable state, holding SCL low and the
>> current __i2c_dw_disable procedure is not working. In this situation
> __i2c_dw_disable()
>
>> only a Linux reboot can fix the i2c bus.
> If reboot helps, what magic does it do that Linux OS can't repeat in software?
> Please, elaborate more.
Sorry I was not very clear. In fact I meant a SoC reset, not a Linux 
reboot. It's just that on our SoC with boot-from-flash a reset will also 
reboot the Linux. But indeed what fixes the issue is the reset of the SoC.
>
> ...
>
>>   	int timeout = 100;
>>   	u32 status;
>> +	u32 raw_intr_stats;
>> +	u32 enable;
>> +	bool abort_needed;
>> +	bool abort_done = false;
> Perhaps reversed xmas tree order?
Oh, I didn't know about this, thanks, ack!
>
> 	bool abort_done = false;
> 	bool abort_needed;
> 	u32 raw_intr_stats;
> 	int timeout = 100;
> 	u32 status;
> 	u32 enable;
>
> ...
>
>> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>> +	if (abort_needed)
>> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>>   
>>   	do {
>> +		if (abort_needed && !abort_done) {
>> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
>> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
>> +			continue;
> This will exhaust the timeout and below can be run at most once,
> is it a problem?
I was also wondering about this... I can propose to extract this in 2 
loops. First loop to wait for the abort to finish, with its own timeout. 
Then the untouched second loop that waits for the disabling to finish.
>
> Also it's a tight busyloop, are you sure it's what you need?

I don't know, I would expect that this would not take much time, I 
already have a V2 for this patch with all your remarks taken into 
account, including the splitting into 2 loops (previous comment).

I am waiting before sending it to have the opportunity to test it on the 
real device, it will be done on the August 21st since I am in holidays 
for now.

I will print the number of iterations it takes for the abort to finish. 
If the abort is quick, maybe there is no need for sleeping. I didn't see 
any info about the time it takes inside the IP documentation.

Thanks for the review!

I'll get back to you when I have done the testing of the V2 patch :) (or 
maybe you want it on the mailing list now as an RFC ?)

-- 

Yann

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

[Yann Sionneau]

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!

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

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 04:42:05PM +0200, Yann Sionneau wrote:
> Le 11/08/2023 à 16:01, Andy Shevchenko a écrit :
> > On Fri, Aug 11, 2023 at 02:46:24PM +0200, Yann Sionneau wrote:

...

> > > +				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 :)

IIRC it's unsigned, so you may use

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

which will be more explicit.

> > > +					len = i2c_dw_recv_len(dev, tmp);
> > > +				else
> > > +					i2c_dw_abort(dev);

-- 
With Best Regards,
Andy Shevchenko