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

On 7/28/2026 11:56 PM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
>

I'll add the same NACK-retry loop to qcom_ec_sci_evt_control()'s write
and qcom_ec_irq()'s SCI event read.

>> 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);
>     ...
> }
> 

This one's tied to how the cooling device gets registered — devm was
specifically requested there during the initial driver review

> [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.
> 

Yeah, I'll switch that to a named initializer instead of positional.

-- 
Best Regards,
Anvesh