Re: [PATCH v3 8/8] iio: light: si1133: prevent race condition on timeout

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:56PM +0200, Joshua Crofts via B4 Relay wrote:

> Sashiko reported a bug where the si1133_command exits on timeout
> without halting the sensor or masking the interrupt. If the sensor
> completes the command later, any subsequent command to the sensor
> will cause the IRQ handler to complete immediately, returning stale
> data to the driver all while the command hasn't finished yet, shifting
> all potential reads in the future.
> 
> Fix this by masking the IRQ if wait_for_completion_timeout() fails.
> When initiating a new command, do a dummy read of the IRQ_STATUS
> register and turn the IRQ back on.

> Closes: https://sashiko.dev/#/message/20260428-si1133-checkup-v2-5-70ad14bfefe2%40gmail.com
> Assisted-by: gemini:gemini-3.1-pro-preview
> Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>

Reported-by?

Also with this seems the Fixes tag is appropriate (and in the other patch).
Also note, fixes should go first in the series.

...

> -		if (!wait_for_completion_timeout(&data->completion, timeout))
> +		if (!wait_for_completion_timeout(&data->completion, timeout)) {
> +			/* Mask the IRQ to prevent delayed interrupt waking up
> +			 * any subsequent command.
> +			 */
> +			regmap_write(data->regmap, SI1133_REG_IRQ_ENABLE, 0);
>  			return -ETIMEDOUT;
> +		}

Seems like you dropped {} and reinstantiated them. It's not good style. Just
make sure that the first patch that does something leaves it in the form that
next patches won't have too many + or - lines that basically revert previous
subchanges. I call this style ping-pong and highly discourage from using it.

...

>  			/*

> -			 * reset counter on err to prevent sofware and hardware
> -			 * counters being out of sync
> +			 * Reset counter on err to prevent sofware and hardware
> +			 * counters being out of sync.
>  			 */

Ha-ha, doesn't belong to this change (see also previous reply).

-- 
With Best Regards,
Andy Shevchenko