Re: [PATCH v3 2/2] i2c: cadence: Add support for Axiado AX3000

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The I2C_AQ_NO_ZERO_LEN_READ adapter quirk is conditionally applied only when CDNS_I2C_ENABLE_SMBUS_QUICK is set, leaving all other Cadence I2C controllers vulnerable to unprivileged hardware hangs triggered by 0-length reads.
- [High] Atomic mode transfers completely bypass error checking for short messages, silently ignoring NACKs and incorrectly reporting success.
- [High] Incorrect teardown order in `cdns_i2c_remove` disables runtime PM before unregistering the I2C adapter, preventing client devices from communicating during removal.
--

commit 64d28f7fd176831d6ec144e8d435ebe00982b497
Author: Swark Yang <syang@axiado.com>

i2c: cadence: Add support for Axiado AX3000

The Axiado AX3000 SoC integrates a Cadence I2C controller that supports
SMBus Quick commands. This introduces the compatible string and a quirk
to enable this functionality. It also adds I2C_AQ_NO_ZERO_LEN_READ to
prevent hardware hangs from 0-length reads.

> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 0fb728ade92e4..1964ea1650c59 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
[ ... ]
> @@ -1175,10 +1176,14 @@ static int cdns_i2c_master_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg
>   */
>  static u32 cdns_i2c_func(struct i2c_adapter *adap)
>  {
> +	struct cdns_i2c *id = adap->algo_data;
>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
>  			(I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
>  			I2C_FUNC_SMBUS_BLOCK_DATA;
>  
> +	if (id->quirks & CDNS_I2C_ENABLE_SMBUS_QUICK)
> +		func |= I2C_FUNC_SMBUS_QUICK;
> +

[Severity: High]
This is a pre-existing issue, but enabling SMBus Quick exposes a bug in
atomic transfers where short messages bypass error checking.

When performing a short write in atomic mode (like an SMBus Quick write
for bus scanning), cdns_i2c_msend() writes data to the FIFO and decrements
id->send_count to 0. This bypasses cdns_i2c_msend_rem_atomic(), which means
cdns_i2c_error_check() is never invoked.

cdns_i2c_process_msg() then polls for the completion bit but fails to
verify the NACK error bits because id->err_status was not updated.

Does this completely break atomic bus scanning, since all polled addresses
will falsely appear to exist and ACK? Furthermore, if the hardware does not
set the completion bit on a NACK, could the polling loop hit its maximum
timeout in atomic context for every NACKed address, leading to stalls?

>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	func |= I2C_FUNC_SLAVE;
>  #endif
[ ... ]
> @@ -1510,6 +1530,9 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>  		id->quirks = data->quirks;
>  	}
>  
> +	if (id->quirks & CDNS_I2C_ENABLE_SMBUS_QUICK)
> +		id->adap.quirks = &cdns_i2c_quirks;
> +

[Severity: High]
This isn't a bug introduced by this patch, but should the 0-length read
quirk be applied unconditionally to all Cadence controllers?

The commit message notes that writing 0 to the transfer size register leaves
the hardware in an unsupported state. As written, the quirk is conditionally
applied only when CDNS_I2C_ENABLE_SMBUS_QUICK is set.

Does this leave other Cadence I2C controllers (like legacy Zynq platforms)
vulnerable to unprivileged hardware hangs triggered by 0-length reads via
the I2C_RDWR ioctl?

[Severity: High]
This is another pre-existing issue, but there appears to be an incorrect
teardown order in cdns_i2c_remove() that prevents client devices from
communicating during removal:

drivers/i2c/busses/i2c-cadence.c:cdns_i2c_remove() {
    ...
	pm_runtime_disable(&pdev->dev);
	pm_runtime_set_suspended(&pdev->dev);
	pm_runtime_dont_use_autosuspend(&pdev->dev);

	i2c_del_adapter(&id->adap);
    ...
}

Because runtime PM is disabled before unregistering the adapter and its
child devices, any child driver's remove callback that performs I2C
transfers to quiesce the hardware will fail. The transfer invokes
cdns_i2c_master_xfer(), which calls pm_runtime_resume_and_get(). Since
runtime PM is already disabled, this returns -EACCES.

Could this prevent clean module unloads or suspend by leaving hardware in a
bad state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-axiado-ax3000-cadence-i2c-support-v3-0-4e217cfe5904@axiado.com?part=2