Re: [PATCH v4 1/5] media: hi846: fix hi846_write_reg_16 handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Runtime PM usage count underflow and torn hardware state due to unhandled negative return values.
--

commit a7e288159f9d9020f112e24280a1cf27c80c8fca
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: fix hi846_write_reg_16 handling

This commit attempts to fix error propagation by resetting the error
variable to 0 when it is positive. It clears the positive return value
from pm_runtime_get_if_in_use() before performing I2C writes.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index a3f77b8434ca7..7f069aca0fce5 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1271,6 +1271,8 @@ static void hi846_write_reg_16(struct hi846 *hi846, u16 reg, u16 val, int *err)
>  	if (*err < 0)
>  		return;
>  
> +	*err = 0;
> +

While this clears the positive return values from pm_runtime_get_if_in_use(),
does this leave negative return values unhandled in the caller?

If pm_runtime_get_if_in_use() in hi846_set_ctrl() returns a negative error
like -EINVAL when Runtime PM is disabled, the usage count is not incremented.
The check for (!ret || ret == -EAGAIN) fails, so execution falls through with
a negative value.

Because this negative value is passed into hi846_write_reg_16(), the check
just above this change will cause I2C writes to be silently skipped for
controls like V4L2_CID_EXPOSURE. Other controls like V4L2_CID_ANALOGUE_GAIN
might ignore the error and execute unconditionally, leaving the hardware in a
torn state.

Finally, since execution continues, pm_runtime_put() is called unconditionally
at the end of hi846_set_ctrl().

Because the usage count was never incremented for the negative error case,
will this cause a PM usage count underflow?

Would it be better to correctly validate the PM return value directly in
hi846_set_ctrl() with a check like if (ret <= 0) return 0; instead of
clearing the positive error here?

>  	put_unaligned_be16(reg, buf);
>  	put_unaligned_be16(val, buf + 2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511103927.279550-1-mitltlatltl@gmail.com?part=1