Re: [PATCH v1 2/3] i2c: designware: Export symbols and add __weak for DWC I2C driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Exporting core driver functions using `__weak` allows global overrides, completely breaking generic DesignWare I2C hardware in multi-platform kernels.
- [High] Compile-time `#if IS_ENABLED(CONFIG_I2C_DWC_CORE)` checks unconditionally replace generic hardware logic with StarFive DWC-specific logic.
- [High] The function `i2c_dw_set_fifo_size` acquires the bus lock but fails to release it if `CONFIG_I2C_DWC_CORE` is enabled.
- [High] Return values of probe functions are ignored, and standard adapter initialization is skipped when `CONFIG_I2C_DWC_CORE` is enabled.
- [High] The `adap->quirks` field is completely uninitialized when `CONFIG_I2C_DWC_SLAVE` is enabled.
- [High] Incorrect packing of FIFO depths leads to bitfield overlap and severely corrupted hardware configurations.
- [High] Skipped interrupt mask initialization for DWC I2C slaves opens the system to unhandled interrupt storms.
--

commit 49b74b77ebfe8943f63b3e20e42d019503ae6640
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
Subject: i2c: designware: Export symbols and add __weak for DWC I2C driver

The upcoming StarFive DWC I2C driver is based on the DesignWare I2C core but
requires its own probe and configuration routines due to register layout
differences. Export several key functions and mark them as __weak to allow
the DWC driver to override the implementations.

> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 4dc57fd561703..cfeec5d338bba 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
[ ... ]
> @@ -411,7 +416,7 @@ static void i2c_dw_write_timings(struct dw_i2c_dev *dev)
>   *
>   * The controller must be disabled before this function is called.
>   */
> -void i2c_dw_set_mode(struct dw_i2c_dev *dev, int mode)
> +__weak void i2c_dw_set_mode(struct dw_i2c_dev *dev, int mode)

[Severity: High]
Does marking generic driver functions like i2c_dw_set_mode() as __weak risk
breaking generic DesignWare I2C hardware in multi-platform kernels?

If both the standard DesignWare and StarFive drivers are enabled, strong
symbols from the StarFive driver could globally override these functions for
all controllers. Could hardware-specific variants be implemented using an
operations structure with function pointers instead?

[ ... ]
> @@ -806,10 +811,25 @@ static int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
>  	if (ret)
>  		return ret;
>  
> +#if IS_ENABLED(CONFIG_I2C_DWC_CORE)
> +	u32 tx_fifo_cfg = 8, rx_fifo_cfg = 8;
> +
> +#ifdef CONFIG_OF
> +	ret = of_property_read_u32(dev->dev->of_node, "dwc-i2c-tx-fifo-depth", &tx_fifo_cfg);
> +	if (!ret && (tx_fifo_cfg < 2 || tx_fifo_cfg > 256))
> +		tx_fifo_cfg = 8;
> +
> +	ret = of_property_read_u32(dev->dev->of_node, "dwc-i2c-rx-fifo-depth", &rx_fifo_cfg);
> +	if (!ret && (rx_fifo_cfg < 2 || rx_fifo_cfg > 256))
> +		rx_fifo_cfg = 8;
> +#endif
> +	param = rx_fifo_cfg << 8 | tx_fifo_cfg << 16;

[Severity: High]
Can this bitwise packing corrupt the hardware configuration if rx_fifo_cfg is
set to 256?

Since 256 requires 9 bits, shifting rx_fifo_cfg by 8 yields 0x10000, which
sets bit 16 and overflows into the tx_fifo_cfg field. This causes FIELD_GET
to return 0 later on, incorrectly setting the FIFO depths to 1.

> +#else
>  	ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, &param);
>  	i2c_dw_release_lock(dev);
>  	if (ret)
>  		return ret;
> +#endif

[Severity: High]
Does this code leak the bus lock when CONFIG_I2C_DWC_CORE is enabled?

i2c_dw_acquire_lock(dev) is called earlier in this function, but the matching
i2c_dw_release_lock(dev) call is placed in the #else block. When the DWC core
is enabled, the function returns without releasing the lock, which can cause
a deadlock during the next lock acquisition.

[ ... ]
> @@ -835,7 +855,9 @@ u32 i2c_dw_func(struct i2c_adapter *adap)
>  
>  void i2c_dw_disable(struct dw_i2c_dev *dev)
>  {
> +#if !IS_ENABLED(CONFIG_I2C_DWC_CORE)
>  	unsigned int dummy;
> +#endif
>  	int ret;
>  
>  	ret = i2c_dw_acquire_lock(dev);
> @@ -847,7 +869,12 @@ void i2c_dw_disable(struct dw_i2c_dev *dev)
>  
>  	/* Disable all interrupts */
>  	__i2c_dw_write_intr_mask(dev, 0);
> +
> +#if IS_ENABLED(CONFIG_I2C_DWC_CORE)
> +	regmap_write(dev->map, DWC_IC_INTR_CLR, DWC_CLR_INTR);
> +#else
>  	regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
> +#endif

[Severity: High]
Will this compile-time check break standard DesignWare I2C controllers in
multi-platform kernels?

By using #if IS_ENABLED(), the mandatory read from DW_IC_CLR_INTR is replaced
globally by a write to DWC_IC_INTR_CLR when the DWC core is compiled in,
causing generic controllers to execute incorrect logic.

[ ... ]
> @@ -896,6 +923,12 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  	if (ret)
>  		return ret;
>  
> +#if IS_ENABLED(CONFIG_I2C_DWC_CORE)
> +	if (dev->mode == DW_IC_SLAVE)
> +		i2c_dw_probe_slave(dev);
> +	else
> +		i2c_dw_probe_master(dev);

[Severity: High]
Does this code ignore the return values of the probe functions?

If initialization fails inside i2c_dw_probe_slave() or i2c_dw_probe_master(),
the error is silently ignored and the driver continues probing.

> +#else
>  	ret = i2c_dw_probe_master(dev);
>  	if (ret)
>  		return ret;
> @@ -906,10 +939,16 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  
>  	if (!adap->name[0])
>  		strscpy(adap->name, "Synopsys DesignWare I2C adapter");
> +#endif

[Severity: High]
Are we skipping generic initialization steps here?

Because the calls to i2c_dw_init(dev) and initializing adap->name are enclosed
in the #else block, they are entirely skipped when CONFIG_I2C_DWC_CORE is
enabled, which could break standard adapter initialization.

>  	adap->retries = 3;
>  	adap->algo = &i2c_dw_algo;
> +#if IS_ENABLED(CONFIG_I2C_DWC_SLAVE)
> +	if (dev->mode == DW_IC_SLAVE)
> +		adap->algo = &i2c_dw_slave_algo;
> +#else
>  	adap->quirks = &i2c_dw_quirks;
> +#endif

[Severity: High]
Will adap->quirks remain uninitialized when CONFIG_I2C_DWC_SLAVE is enabled?

The assignment to adap->quirks is in the #else block. If CONFIG_I2C_DWC_SLAVE
is enabled, adap->quirks is never initialized for any device mode, including
master. This removes quirks like I2C_AQ_NO_ZERO_LEN, which could allow
invalid zero-length transfers to hang standard controllers indefinitely.

>  	adap->dev.parent = dev->dev;
>  	i2c_set_adapdata(adap, dev);
>  
> @@ -938,16 +977,18 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  	if (!dev->emptyfifo_hold_master)
>  		irq_flags |= IRQF_NO_THREAD;
>  
> -	ret = i2c_dw_acquire_lock(dev);
> -	if (ret)
> -		return ret;
> +	if (!IS_ENABLED(CONFIG_I2C_DWC_CORE) || dev->mode == DW_IC_MASTER) {
> +		ret = i2c_dw_acquire_lock(dev);
> +		if (ret)
> +			return ret;
>  
> -	__i2c_dw_write_intr_mask(dev, 0);
> -	i2c_dw_release_lock(dev);
> +		__i2c_dw_write_intr_mask(dev, 0);
> +		i2c_dw_release_lock(dev);
> +	}
>  
>  	if (!(dev->flags & ACCESS_POLLING)) {
>  		ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,

[Severity: High]
Could skipping the interrupt mask clearing for DWC slaves cause unhandled
interrupt storms?

If the unmasked hardware has pending interrupts, devm_request_irq() will
immediately trigger i2c_dw_isr_slave(). Because dev->slave is not yet
registered during probe, the ISR will return IRQ_NONE without clearing the
hardware interrupt, leading to a persistent storm.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521034340.27837-1-lianfeng.ouyang@starfivetech.com?part=2