Re: [PATCH v3 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] Unhandled negative error code from pm_runtime_get_if_in_use() causes silent failure of exposure/vblank controls and PM usage count underflow.
--

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

media: hi846: fix hi846_write_reg_16 handling

This commit fixes an issue where a positive return value from
pm_runtime_get_if_in_use() propagates back to callers as an error. It resets
the error pointer to 0 inside hi846_write_reg_16() before performing I2C
operations.

> 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;

While this patch handles the positive return code, does it miss the case
where pm_runtime_get_if_in_use() returns a negative error code like -EINVAL?

If runtime PM is disabled, pm_runtime_get_if_in_use() returns -EINVAL. In
hi846_set_ctrl(), the error check misses this negative value:

drivers/media/i2c/hi846.c:hi846_set_ctrl() {
    ...
	ret = pm_runtime_get_if_in_use(&client->dev);
	if (!ret || ret == -EAGAIN)
		return 0;
    ...
}

This allows the negative ret to propagate to hi846_write_reg_16(). Since
*err is less than zero, the function returns immediately, silently skipping
all hardware writes for V4L2_CID_EXPOSURE and V4L2_CID_VBLANK.

Additionally, does this cause a PM usage count underflow?

At the end of hi846_set_ctrl(), pm_runtime_put() is called unconditionally:

drivers/media/i2c/hi846.c:hi846_set_ctrl() {
    ...
	pm_runtime_put(&client->dev);

	return ret;
}

Since the usage count was never incremented when pm_runtime_get_if_in_use()
returned -EINVAL, this might trigger a usage count underflow warning.

Could hi846_set_ctrl() explicitly isolate the PM status, set ret to 0 before
making hardware writes, and only call pm_runtime_put() if the get function
returned a value greater than 0?

> +	*err = 0;
> +
>  	put_unaligned_be16(reg, buf);
>  	put_unaligned_be16(val, buf + 2);
>  	ret = i2c_master_send(client, buf, sizeof(buf));

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