[PATCH v2 0/2] i2c: designware: Handle invalid SMBus block data response length value

[PATCH v2 0/2] i2c: designware: Handle invalid SMBus block data response length value

[Tam Nguyen]

This v2 patch series updates the I2C DesignWare driver to handle invalid
SMBus block data response length value that causes the bus is hang and
can not be recovered.

v2:
  + Create new commit to correct length byte validation logic    [Quan]
  + Get right data length from IC_DATA_CMD register              [Jarkko]

Quan Nguyen (1):
  i2c: designware: Correct length byte validation logic

Tam Nguyen (1):
  i2c: designware: Handle invalid SMBus block data response length value

 drivers/i2c/busses/i2c-designware-master.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] i2c: designware: Correct length byte validation logic

[Tam Nguyen]

From: Quan Nguyen <quan@os.amperecomputing.com>

Commit 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
changes the logic to validate the whole 32-bit return value of
DW_IC_DATA_CMD register instead of 8-bit LSB without reason.

Later, commit f53f15ba5a85 ("i2c: designware: Get right data length"),
introduced partial fix but not enough because the "tmp > 0" still test
tmp as 32-bit value and is wrong in case the IC_DATA_CMD[11] is set.

Revert the logic to just before commit 0daede80f870
("i2c: designware: Convert driver to using regmap API").

Fixes: f53f15ba5a85 ("i2c: designware: Get right data length")
Fixes: 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
Cc: stable@vger.kernel.org
Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 55ea91a63382..e96276d1b002 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -526,9 +526,10 @@ 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 > 0) {
+			    tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
 				len = i2c_dw_recv_len(dev, tmp);
 			}
 			*buf++ = tmp;
-- 
2.25.1

[PATCH v2 2/2] i2c: designware: Handle invalid SMBus block data response length value

[Tam Nguyen]

In the I2C_FUNC_SMBUS_BLOCK_DATA case, the invalid length byte value
(outside of 1-32) of the SMBus block data response from the Slave device
is not correctly handled by the I2C Designware driver.

In case IC_EMPTYFIFO_HOLD_MASTER_EN==1, which cannot be detected
from the registers, the Master can be disabled only if the STOP bit
is set. Without STOP bit set, the Master remains active, holding the bus
until receiving a block data response length. This hangs the bus and
is unrecoverable.

Avoid this by issuing another dump read to reach the stop condition when
an invalid length byte is received.

Cc: stable@vger.kernel.org
Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index e96276d1b002..c51fc1f4b97e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -528,8 +528,19 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			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 <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
+			if (flags & I2C_M_RECV_LEN) {
+				/*
+				 * if IC_EMPTYFIFO_HOLD_MASTER_EN is set, which cannot be
+				 * detected from the registers, the controller can be
+				 * disabled if the STOP bit is set. But it is only set
+				 * after receiving block data response length in
+				 * I2C_FUNC_SMBUS_BLOCK_DATA case. That needs to read
+				 * another byte with STOP bit set when the block data
+				 * response length is invalid to complete the transaction.
+				 */
+				if (!tmp || tmp > I2C_SMBUS_BLOCK_MAX)
+					tmp = 1;
+
 				len = i2c_dw_recv_len(dev, tmp);
 			}
 			*buf++ = tmp;
-- 
2.25.1

Re: [PATCH v2 1/2] i2c: designware: Correct length byte validation logic

[Jarkko Nikula]

On 7/26/23 11:00, Tam Nguyen wrote:
> From: Quan Nguyen <quan@os.amperecomputing.com>
> 
> Commit 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
> changes the logic to validate the whole 32-bit return value of
> DW_IC_DATA_CMD register instead of 8-bit LSB without reason.
> 
> Later, commit f53f15ba5a85 ("i2c: designware: Get right data length"),
> introduced partial fix but not enough because the "tmp > 0" still test
> tmp as 32-bit value and is wrong in case the IC_DATA_CMD[11] is set.
> 
> Revert the logic to just before commit 0daede80f870
> ("i2c: designware: Convert driver to using regmap API").
> 
> Fixes: f53f15ba5a85 ("i2c: designware: Get right data length")
> Fixes: 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v2 2/2] i2c: designware: Handle invalid SMBus block data response length value

[Jarkko Nikula]

On 7/26/23 11:00, Tam Nguyen wrote:
> In the I2C_FUNC_SMBUS_BLOCK_DATA case, the invalid length byte value
> (outside of 1-32) of the SMBus block data response from the Slave device
> is not correctly handled by the I2C Designware driver.
> 
> In case IC_EMPTYFIFO_HOLD_MASTER_EN==1, which cannot be detected
> from the registers, the Master can be disabled only if the STOP bit
> is set. Without STOP bit set, the Master remains active, holding the bus
> until receiving a block data response length. This hangs the bus and
> is unrecoverable.
> 
> Avoid this by issuing another dump read to reach the stop condition when
> an invalid length byte is received.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v2 0/2] i2c: designware: Handle invalid SMBus block data response length value

[Andi Shyti]

Hi

On Wed, 26 Jul 2023 14:59:59 +0700, Tam Nguyen wrote:
> This v2 patch series updates the I2C DesignWare driver to handle invalid
> SMBus block data response length value that causes the bus is hang and
> can not be recovered.
> 
> v2:
>   + Create new commit to correct length byte validation logic    [Quan]
>   + Get right data length from IC_DATA_CMD register              [Jarkko]
> 
> [...]

Applied to i2c/andi-for-current on

https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Please note that this patch may still undergo further evaluation
and the final decision will be made in collaboration with
Wolfram.

Thank you,
Andi

Patches applied
===============
[1/2] i2c: designware: Correct length byte validation logic
      commit: 29a1ae0bd13f45da520a7106abfe347f9375f64e
[2/2] i2c: designware: Handle invalid SMBus block data response length value
      commit: dcd14feb235bed87a9cba63538bcf6a7a7e97b78

Re: [PATCH v2 1/2] i2c: designware: Correct length byte validation logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On Wed, Jul 26, 2023 at 03:00:00PM +0700, Tam Nguyen wrote:
> From: Quan Nguyen <quan@os.amperecomputing.com>
> 
> Commit 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
> changes the logic to validate the whole 32-bit return value of
> DW_IC_DATA_CMD register instead of 8-bit LSB without reason.
> 
> Later, commit f53f15ba5a85 ("i2c: designware: Get right data length"),
> introduced partial fix but not enough because the "tmp > 0" still test
> tmp as 32-bit value and is wrong in case the IC_DATA_CMD[11] is set.
> 
> Revert the logic to just before commit 0daede80f870
> ("i2c: designware: Convert driver to using regmap API").
> 
> Fixes: f53f15ba5a85 ("i2c: designware: Get right data length")
> Fixes: 0daede80f870 ("i2c: designware: Convert driver to using regmap API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: designware: Handle invalid SMBus block data response length value

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Wed, Jul 26, 2023 at 03:00:01PM +0700, Tam Nguyen wrote:
> In the I2C_FUNC_SMBUS_BLOCK_DATA case, the invalid length byte value
> (outside of 1-32) of the SMBus block data response from the Slave device
> is not correctly handled by the I2C Designware driver.
> 
> In case IC_EMPTYFIFO_HOLD_MASTER_EN==1, which cannot be detected
> from the registers, the Master can be disabled only if the STOP bit
> is set. Without STOP bit set, the Master remains active, holding the bus
> until receiving a block data response length. This hangs the bus and
> is unrecoverable.
> 
> Avoid this by issuing another dump read to reach the stop condition when
> an invalid length byte is received.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]