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

Andy Shevchenko
2026-04-29T19:27:51Z

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@gmail.com>

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