Re: [PATCH 6/8] platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch adds retry logic for EC NACKs to `qcom_ec_write()` and `qcom_ec_read()`, but fails to add equivalent retries for the numerous `i2c_smbus_*` calls made throughout the driver, leaving them vulnerable to the exact same failure.
- [High] Race condition during device removal where cooling device callbacks can execute after debug mode is disabled.
- [Low] Initialized arrays of type `struct i2c_device_id` must use named initializers rather than positional initializers.
--

--- Patch [6]: [PATCH 6/8] platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK ---
commit 42a41f4860b9d5a24932df8c69069cb7fd89b4d1
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK

The patch adds bounded retry logic to qcom_ec_write() and qcom_ec_read()
to handle EC firmware NACKs that occur during busy windows right after
boot or during SCI event bursts.

> The EC firmware NACKs the bus instead of clock-stretching while it is
> busy servicing internal work (e.g. right after boot, when it is also
> emitting a burst of SCI events). A transfer issued in that window fails
> with -ENXIO even though the EC is otherwise healthy.

[Severity: High]
This isn't a bug introduced by this patch, but since the EC NACKs the bus
right after boot and during SCI event bursts, will the un-retried i2c_smbus
calls also fail?

For example, qcom_ec_sci_evt_control() is called directly during probe and
uses i2c_smbus_write_byte_data() without retry logic:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_sci_evt_control() {
    ...
    ret = i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, enable ? 1 : 0);
    ...
}

Similarly, qcom_ec_irq() reads events during an SCI burst using
i2c_smbus_read_byte_data():

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_irq() {
    ...
    val = i2c_smbus_read_byte_data(ec->client, EC_SCI_EVT_READ_CMD);
    ...
}

Should these SMBus calls also be updated to retry on NACK?

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index a420a180a1948..122ae27c2b483 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c

[ ... ]

> @@ -281,11 +302,18 @@ static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, const u8 *subcmd,
>  	msgs[1].len   = resp_len;
>  	msgs[1].buf   = read_buf;
>  
> -	ret = i2c_transfer(client->adapter, msgs, 2);
> -	if (ret != 2) {
> -		ret = ret < 0 ? ret : -EIO;
> -		goto out;
> +	for (int i = 0; i < EC_I2C_NACK_MAX_ATTEMPTS; i++) {
> +		ret = i2c_transfer(client->adapter, msgs, 2);
> +		ret = ret == 2 ? 0 : (ret < 0 ? ret : -EIO);
> +		if (ret != -ENXIO || i == EC_I2C_NACK_MAX_ATTEMPTS - 1)
> +			break;
> +
> +		dev_dbg(&client->dev,
> +			"EC busy (NACK), retrying read of cmd 0x%02x\n", cmd);
> +		msleep(EC_I2C_NACK_RETRY_DELAY_MS);
>  	}
> +	if (ret)
> +		goto out;
>  
>  	if (read_buf[0] == 0 || read_buf[0] == 0xff) {
>  		ret = -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but does the cooling device teardown have a
race condition that could leave the fan stuck in debug (manual PWM) mode?

In qcom_ec_remove(), the driver turns off debug mode:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_remove() {
    ...
    qcom_ec_fan_debug_mode_off(ec_cdev);
    ...
}

However, since the thermal cooling device is managed by devres, it remains
registered until after remove() completes. Can the thermal core invoke
qcom_ec_fan_set_cur_state() during this window, which re-enables debug mode?

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_fan_set_cur_state() {
    ...
    ret = qcom_ec_fan_write_pwm(ec_cdev, state);
    ...
}

[Severity: Low]
This is a pre-existing issue, but does qcom_ec_i2c_id_table violate the I2C
subsystem API guidelines by using positional initializers?

drivers/platform/arm64/qcom-hamoa-ec.c:
static const struct i2c_device_id qcom_ec_i2c_id_table[] = {
	{ "qcom-hamoa-ec", },
	{}
};

The subsystem guidelines state that initialized arrays of type
struct i2c_device_id must use named initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=6