[PATCH 0/3] i2c: designware: Misc small fixes

[PATCH 0/3] i2c: designware: Misc small fixes

[Jean Delvare]

Hi all,

While working on CVE-2025-38380, I noticed a few issues in the
i2c-designware driver, which I think are worth fixing now to make
future driver development safer and easier.

[PATCH 1/3] i2c: designware: Use msgs[0] to validate the slave address
[PATCH 2/3] i2c: designware: Extend check for mixed slave addresses
[PATCH 3/3] i2c: designware: Turn models back to enumerated values

Note that I do not own any supported device so I can't test these
changes. Even though the fixes are rather straightforward, I would
appreciate if someone can test them on actual hardware, to be on the
safe side.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/3] i2c: designware: Use msgs[0] to validate the slave address

[Jean Delvare]

Function i2c_dw_xfer_init() makes use of dev->msg_write_idx to index
into the array of messages. The purpose is to program the controller
with the slave address.

However, this driver only supports transfers where all messages have
the same address. Therefore checking the first message leads to the
same result, without depending on dev->msg_write_idx having been
initialized elsewhere before.

This function was always called with dev->msg_write_idx == 0 anyway,
so this does not change anything, only makes the intentions clearer
and the code more simple.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 drivers/i2c/busses/i2c-designware-master.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- linux-6.16.orig/drivers/i2c/busses/i2c-designware-master.c
+++ linux-6.16/drivers/i2c/busses/i2c-designware-master.c
@@ -254,7 +254,7 @@ static void i2c_dw_xfer_init(struct dw_i
 	__i2c_dw_disable(dev);
 
 	/* If the slave address is ten bit address, enable 10BITADDR */
-	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
+	if (msgs[0].flags & I2C_M_TEN) {
 		ic_con = DW_IC_CON_10BITADDR_MASTER;
 		/*
 		 * If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
@@ -272,8 +272,7 @@ static void i2c_dw_xfer_init(struct dw_i
 	 * Set the slave (target) address and enable 10-bit addressing mode
 	 * if applicable.
 	 */
-	regmap_write(dev->map, DW_IC_TAR,
-		     msgs[dev->msg_write_idx].addr | ic_tar);
+	regmap_write(dev->map, DW_IC_TAR, msgs[0].addr | ic_tar);
 
 	/* Enforce disabled interrupts (due to HW issues) */
 	__i2c_dw_write_intr_mask(dev, 0);
@@ -363,7 +362,6 @@ static int amd_i2c_dw_xfer_quirk(struct
 
 	dev->msgs = msgs;
 	dev->msgs_num = num_msgs;
-	dev->msg_write_idx = 0;
 	i2c_dw_xfer_init(dev);
 
 	/* Initiate messages read/write transaction */

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/3] i2c: designware: Extend check for mixed slave addresses

[Jean Delvare]

The i2c-designware driver only supports transfers where all messages
use the same slave address. This condition is currently tested in
i2c_dw_xfer_msg(), with 2 limitations:
* The code only checks the address value, not the 10-bit address
  flag, so it could miss an address change.
* For the AMD Navi GPU devices, the driver uses a dedicated function
  instead of i2c_dw_xfer_msg(), so the check is not performed.

Move the check to the common code path, and add the 10-bit address
flag comparison, to catch and report early if a given transfer is not
supported.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 drivers/i2c/busses/i2c-designware-master.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

--- linux-6.16.orig/drivers/i2c/busses/i2c-designware-master.c
+++ linux-6.16/drivers/i2c/busses/i2c-designware-master.c
@@ -429,7 +429,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	struct i2c_msg *msgs = dev->msgs;
 	u32 intr_mask;
 	int tx_limit, rx_limit;
-	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;
 	bool need_restart = false;
@@ -440,18 +439,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		u32 flags = msgs[dev->msg_write_idx].flags;
 
-		/*
-		 * If target address has changed, we need to
-		 * reprogram the target address in the I2C
-		 * adapter when we are done with this transfer.
-		 */
-		if (msgs[dev->msg_write_idx].addr != addr) {
-			dev_err(dev->dev,
-				"%s: invalid target address\n", __func__);
-			dev->msg_err = -EINVAL;
-			break;
-		}
-
 		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 			/* new i2c_msg */
 			buf = msgs[dev->msg_write_idx].buf;
@@ -806,10 +793,23 @@ static int
 i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
-	int ret;
+	int ret, i;
 
 	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
 
+	/*
+	 * This driver only supports I2C transfers where all the messages
+	 * use the same address.
+	 */
+	for (i = 1; i < num; i++) {
+		if (msgs[i].addr != msgs[0].addr ||
+		    (msgs[i].flags & I2C_M_TEN) != (msgs[0].flags & I2C_M_TEN)) {
+			dev_err(dev->dev,
+				"Mixed slave addresses not supported\n");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	pm_runtime_get_sync(dev->dev);
 
 	switch (dev->flags & MODEL_MASK) {

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/3] i2c: designware: Turn models back to enumerated values

[Jean Delvare]

There are 4 flag bits reserved to store the device model. All
accesses to this information is properly masked with MODEL_MASK to
extract only these bits and compare them with a given model value.

However the model definitions isn't an enumeration as you would
expect. Instead each model uses a separate flag, meaning that the
reserved space is already exhausted with the 4 models which have been
defined so far.

The error seems to originate from commit a5df4c14b9a9 ("i2c:
designware: Switch header to use BIT() and GENMASK()") where:

define MODEL_MSCC_OCELOT      0x00000100
define MODEL_BAIKAL_BT1       0x00000200

was erroneously converted to:

define MODEL_MSCC_OCELOT      BIT(8)
define MODEL_BAIKAL_BT1       BIT(9)

While numerically equivalent, conceptually it wasn't, and it caused
the models added later to get bit-based definitions instead of
continuing with the next enumerated value (0x00000300).

Turn back these definitions to enumerated values to clear the
confusion, avoid future mistakes, and free some space for more models
to be supported in the future.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 drivers/i2c/busses/i2c-designware-core.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-6.16.orig/drivers/i2c/busses/i2c-designware-core.h
+++ linux-6.16/drivers/i2c/busses/i2c-designware-core.h
@@ -312,10 +312,10 @@ struct dw_i2c_dev {
 #define ARBITRATION_SEMAPHORE			BIT(2)
 #define ACCESS_POLLING				BIT(3)
 
-#define MODEL_MSCC_OCELOT			BIT(8)
-#define MODEL_BAIKAL_BT1			BIT(9)
-#define MODEL_AMD_NAVI_GPU			BIT(10)
-#define MODEL_WANGXUN_SP			BIT(11)
+#define MODEL_MSCC_OCELOT			(1 << 8)
+#define MODEL_BAIKAL_BT1			(2 << 8)
+#define MODEL_AMD_NAVI_GPU			(3 << 8)
+#define MODEL_WANGXUN_SP			(4 << 8)
 #define MODEL_MASK				GENMASK(11, 8)
 
 /*

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/3] i2c: designware: Turn models back to enumerated values

[Andy Shevchenko]

On Thu, Sep 18, 2025 at 04:13:01PM +0200, Jean Delvare wrote:
> There are 4 flag bits reserved to store the device model. All
> accesses to this information is properly masked with MODEL_MASK to
> extract only these bits and compare them with a given model value.
> 
> However the model definitions isn't an enumeration as you would
> expect. Instead each model uses a separate flag, meaning that the
> reserved space is already exhausted with the 4 models which have been
> defined so far.
> 
> The error seems to originate from commit a5df4c14b9a9 ("i2c:
> designware: Switch header to use BIT() and GENMASK()") where:
> 
> define MODEL_MSCC_OCELOT      0x00000100
> define MODEL_BAIKAL_BT1       0x00000200
> 
> was erroneously converted to:

I don't think "erroneously" is correct word here. The code before that commit
as you mentioned starts with a bit set, rather than from 0. I would argue that
the intention was to use a bitmask instead of plain number.

> define MODEL_MSCC_OCELOT      BIT(8)
> define MODEL_BAIKAL_BT1       BIT(9)
> 
> While numerically equivalent, conceptually it wasn't, and it caused
> the models added later to get bit-based definitions instead of
> continuing with the next enumerated value (0x00000300).
> 
> Turn back these definitions to enumerated values to clear the
> confusion, avoid future mistakes, and free some space for more models
> to be supported in the future.

...

> -#define MODEL_MSCC_OCELOT			BIT(8)
> -#define MODEL_BAIKAL_BT1			BIT(9)
> -#define MODEL_AMD_NAVI_GPU			BIT(10)
> -#define MODEL_WANGXUN_SP			BIT(11)
> +#define MODEL_MSCC_OCELOT			(1 << 8)
> +#define MODEL_BAIKAL_BT1			(2 << 8)
> +#define MODEL_AMD_NAVI_GPU			(3 << 8)
> +#define MODEL_WANGXUN_SP			(4 << 8)
>  #define MODEL_MASK				GENMASK(11, 8)

Taking above into consideration, why can't we start them from 0?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] i2c: designware: Turn models back to enumerated values

[Jean Delvare]

Hi Andy,

On Thu, 18 Sep 2025 21:50:55 +0300, Andy Shevchenko wrote:
> On Thu, Sep 18, 2025 at 04:13:01PM +0200, Jean Delvare wrote:
> > There are 4 flag bits reserved to store the device model. All
> > accesses to this information is properly masked with MODEL_MASK to
> > extract only these bits and compare them with a given model value.
> > 
> > However the model definitions isn't an enumeration as you would
> > expect. Instead each model uses a separate flag, meaning that the
> > reserved space is already exhausted with the 4 models which have been
> > defined so far.
> > 
> > The error seems to originate from commit a5df4c14b9a9 ("i2c:
> > designware: Switch header to use BIT() and GENMASK()") where:
> > 
> > define MODEL_MSCC_OCELOT      0x00000100
> > define MODEL_BAIKAL_BT1       0x00000200
> > 
> > was erroneously converted to:  
> 
> I don't think "erroneously" is correct word here. The code before that commit
> as you mentioned starts with a bit set, rather than from 0. I would argue that
> the intention was to use a bitmask instead of plain number.

Models are mutually exclusive, so using a bit array to store that
information doesn't make sense. As a matter of fact, the driver code
which makes use of these values clearly assumes that they can't be
combined:

	switch (dev->flags & MODEL_MASK) {
	case MODEL_BAIKAL_BT1:
		ret = bt1_i2c_request_regs(dev);
		break;
	case MODEL_WANGXUN_SP:
		ret = txgbe_i2c_request_regs(dev);
		break;
	(...)
	}

> > define MODEL_MSCC_OCELOT      BIT(8)
> > define MODEL_BAIKAL_BT1       BIT(9)
> > 
> > While numerically equivalent, conceptually it wasn't, and it caused
> > the models added later to get bit-based definitions instead of
> > continuing with the next enumerated value (0x00000300).
> > 
> > Turn back these definitions to enumerated values to clear the
> > confusion, avoid future mistakes, and free some space for more models
> > to be supported in the future.  
> 
> ...
> 
> > -#define MODEL_MSCC_OCELOT			BIT(8)
> > -#define MODEL_BAIKAL_BT1			BIT(9)
> > -#define MODEL_AMD_NAVI_GPU			BIT(10)
> > -#define MODEL_WANGXUN_SP			BIT(11)
> > +#define MODEL_MSCC_OCELOT			(1 << 8)
> > +#define MODEL_BAIKAL_BT1			(2 << 8)
> > +#define MODEL_AMD_NAVI_GPU			(3 << 8)
> > +#define MODEL_WANGXUN_SP			(4 << 8)
> >  #define MODEL_MASK				GENMASK(11, 8)  
> 
> Taking above into consideration, why can't we start them from 0?

Because model 0 is every other device which is neither MSCC Ocelot nor
Baikal BT1 nor AMD Navi nor Wangxun SP. We could define MODEL_OTHER as
(0 << 8), but there's not reason to ever use it in the code
(switch/case statements simply use default if something needs to be
done for standard devices), which is why nobody bothered defining it so
far.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/3] i2c: designware: Misc small fixes

[Jarkko Nikula]

Hi

On 9/18/25 5:03 PM, Jean Delvare wrote:
> Hi all,
> 
> While working on CVE-2025-38380, I noticed a few issues in the
> i2c-designware driver, which I think are worth fixing now to make
> future driver development safer and easier.
> 
> [PATCH 1/3] i2c: designware: Use msgs[0] to validate the slave address
> [PATCH 2/3] i2c: designware: Extend check for mixed slave addresses
> [PATCH 3/3] i2c: designware: Turn models back to enumerated values
> 
> Note that I do not own any supported device so I can't test these
> changes. Even though the fixes are rather straightforward, I would
> appreciate if someone can test them on actual hardware, to be on the
> safe side.
> 
I'm fine with the whole set.

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 2/3] i2c: designware: Extend check for mixed slave addresses

[Jean Delvare]

On Thu, 18 Sep 2025 16:10:54 +0200, Jean Delvare wrote:
> The i2c-designware driver only supports transfers where all messages
> use the same slave address. This condition is currently tested in
> i2c_dw_xfer_msg(), with 2 limitations:
> * The code only checks the address value, not the 10-bit address
>   flag, so it could miss an address change.
> * For the AMD Navi GPU devices, the driver uses a dedicated function
>   instead of i2c_dw_xfer_msg(), so the check is not performed.
> 
> Move the check to the common code path, and add the 10-bit address
> flag comparison, to catch and report early if a given transfer is not
> supported.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
>  drivers/i2c/busses/i2c-designware-master.c |   28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> --- linux-6.16.orig/drivers/i2c/busses/i2c-designware-master.c
> +++ linux-6.16/drivers/i2c/busses/i2c-designware-master.c
> @@ -429,7 +429,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 intr_mask;
>  	int tx_limit, rx_limit;
> -	u32 addr = msgs[dev->msg_write_idx].addr;
>  	u32 buf_len = dev->tx_buf_len;
>  	u8 *buf = dev->tx_buf;
>  	bool need_restart = false;
> @@ -440,18 +439,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
>  		u32 flags = msgs[dev->msg_write_idx].flags;
>  
> -		/*
> -		 * If target address has changed, we need to
> -		 * reprogram the target address in the I2C
> -		 * adapter when we are done with this transfer.
> -		 */
> -		if (msgs[dev->msg_write_idx].addr != addr) {
> -			dev_err(dev->dev,
> -				"%s: invalid target address\n", __func__);
> -			dev->msg_err = -EINVAL;
> -			break;
> -		}
> -
>  		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
>  			/* new i2c_msg */
>  			buf = msgs[dev->msg_write_idx].buf;
> @@ -806,10 +793,23 @@ static int
>  i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
>  	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> -	int ret;
> +	int ret, i;
>  
>  	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
>  
> +	/*
> +	 * This driver only supports I2C transfers where all the messages
> +	 * use the same address.
> +	 */
> +	for (i = 1; i < num; i++) {
> +		if (msgs[i].addr != msgs[0].addr ||

Note, I found meanwhile that some drivers (i2c-amd-mp2, i2c-mlxcpld)
use unlikely() to limit the performance penalty incurred by these
tests. Sounds like a good idea?

> +		    (msgs[i].flags & I2C_M_TEN) != (msgs[0].flags & I2C_M_TEN)) {
> +			dev_err(dev->dev,
> +				"Mixed slave addresses not supported\n");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
>  	pm_runtime_get_sync(dev->dev);
>  
>  	switch (dev->flags & MODEL_MASK) {
> 


-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/3 v2] i2c: designware: Extend check for mixed slave addresses

[Jean Delvare]

The i2c-designware driver only supports transfers where all messages
use the same slave address. This condition is currently tested in
i2c_dw_xfer_msg(), with 2 limitations:
* The code only checks the address value, not the 10-bit address
  flag, so it could miss an address change.
* For the AMD Navi GPU devices, the driver uses a dedicated function
  instead of i2c_dw_xfer_msg(), so the check is not performed.

Move the check to the common code path, and add the 10-bit address
flag comparison, to catch and report early if a given transfer is not
supported.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
Changes since v1:
* Use unlikely() to help the compiler minimize the performance penalty.

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

--- linux-6.16.orig/drivers/i2c/busses/i2c-designware-master.c
+++ linux-6.16/drivers/i2c/busses/i2c-designware-master.c
@@ -429,7 +429,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	struct i2c_msg *msgs = dev->msgs;
 	u32 intr_mask;
 	int tx_limit, rx_limit;
-	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;
 	bool need_restart = false;
@@ -440,18 +439,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		u32 flags = msgs[dev->msg_write_idx].flags;
 
-		/*
-		 * If target address has changed, we need to
-		 * reprogram the target address in the I2C
-		 * adapter when we are done with this transfer.
-		 */
-		if (msgs[dev->msg_write_idx].addr != addr) {
-			dev_err(dev->dev,
-				"%s: invalid target address\n", __func__);
-			dev->msg_err = -EINVAL;
-			break;
-		}
-
 		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 			/* new i2c_msg */
 			buf = msgs[dev->msg_write_idx].buf;
@@ -806,10 +793,23 @@ static int
 i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
-	int ret;
+	int ret, i;
 
 	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
 
+	/*
+	 * This driver only supports I2C transfers where all the messages
+	 * use the same address.
+	 */
+	for (i = 1; i < num; i++) {
+		if (unlikely(msgs[i].addr != msgs[0].addr ||
+			     (msgs[i].flags & I2C_M_TEN) != (msgs[0].flags & I2C_M_TEN))) {
+			dev_err(dev->dev,
+				"Mixed slave addresses not supported\n");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	pm_runtime_get_sync(dev->dev);
 
 	switch (dev->flags & MODEL_MASK) {

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3 v2] i2c: designware: Extend check for mixed slave addresses

[Jarkko Nikula]

On 9/22/25 11:14 AM, Jean Delvare wrote:
> The i2c-designware driver only supports transfers where all messages
> use the same slave address. This condition is currently tested in
> i2c_dw_xfer_msg(), with 2 limitations:
> * The code only checks the address value, not the 10-bit address
>    flag, so it could miss an address change.
> * For the AMD Navi GPU devices, the driver uses a dedicated function
>    instead of i2c_dw_xfer_msg(), so the check is not performed.
> 
> Move the check to the common code path, and add the 10-bit address
> flag comparison, to catch and report early if a given transfer is not
> supported.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
> Changes since v1:
> * Use unlikely() to help the compiler minimize the performance penalty.
> 
>   drivers/i2c/busses/i2c-designware-master.c |   28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>