Re: [PATCH v3 02/11] hwmon: chipcap2: fix off-by-one in command mode retry loop

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

On Wed Oct 14, 2026 at 8:01 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] hwmon: chipcap2: premature timeout due to 1-jiffy wait_for_completion_timeout
> --
>
> commit 529f4f489744495e9cdde8072b4894cc25895048
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: fix off-by-one in command mode retry loop
>
> i == CC2_CM_RETRIES can never be true inside the loop, since it
> only runs while i < CC2_CM_RETRIES. Use CC2_CM_RETRIES - 1 so the
> last failed retry breaks out immediately instead of doing a useless
> disable + 80 ms power-cycle delay before the loop condition ends it
> anyway.
>
>> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
>> index 8550578928523..8102b6851d3d1 100644
>> --- a/drivers/hwmon/chipcap2.c
>> +++ b/drivers/hwmon/chipcap2.c
>> @@ -226,7 +226,7 @@ static int cc2_command_mode_start(struct cc2_data *data)
>>  				     2 * CC2_RESP_START_CM_US);
>>  		}
>>  		ret = cc2_read_command_status(data->client);
>> -		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES)
>> +		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES - 1)
>
> [Severity: High]
> Can this still cause premature timeouts during device initialization and reads?
>
> Looking at cc2_command_mode_start() just above this check, the timeout
> calculation evaluates to exactly 1 jiffy on standard HZ configurations
> (e.g., HZ=100 or HZ=250), because 200 microseconds is significantly less
> than one tick:
>
> 	timeout = usecs_to_jiffies(2 * CC2_RESP_START_CM_US);
> 	ret = wait_for_completion_timeout(&data->complete, timeout);
>
> When a 1-jiffy timeout is passed to wait_for_completion_timeout(), it can
> expire almost instantly if the next timer interrupt occurs immediately after
> the call.
>
> This can cause the driver to prematurely abort the device initialization
> without triggering a retry, returning -ETIMEDOUT sporadically. This leads
> to random failures in device communication and user-space reads even if
> the device properly signals completion within its 100-microsecond window.
>
> This same 1-jiffy timeout issue also affects the EEPROM read sequence in
> cc2_read_reg():
>
> 	timeout = usecs_to_jiffies(2 * CC2_RESP_EEPROM_R_US);
> 	ret = wait_for_completion_timeout(&data->complete, timeout);
>
>>  			break;
>>
>>  		/* command window missed, prepare for a retry */

This is a separate issue from what the patch addresses, which is the
off-by-one error in the retry loop, but it makes sense.

I have never observed a timeout during communication with the device,
but statistically, this seems plausible given the conversion from
microseconds to jiffies.

The most sensible approach would be to send a follow-up patch that
ensures the timeout is at least 2 jiffies, regardless of the HZ
configuration, using something like:

timeout = max(usecs_to_jiffies(2 * CC2_RESP_START_CM_US), 2UL);

The same change would apply to the EEPROM read timeout in cc2_read_reg().

Best regards,
Javier