Re: [PATCH v5 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:

Pre-existing issues:
- [High] Improper error handling of `pm_runtime_get_if_in_use()` causes skipped I2C writes when Runtime PM is disabled (e.g., CONFIG_PM=n).
--

--- Patch [1]: [PATCH v5 1/5] media: hi846: Fix hi846_write_reg_16 handling ---
commit a93dfb63bfce92d91d72fc1b7b2f261ebe6609ab
Author: Pengyu Luo <mitltlatltl@gmail.com>

    media: hi846: Fix hi846_write_reg_16 handling
    
    hi846_write_reg_16() does not clear a positive *err value on success.
    pm_runtime_get_if_in_use() returns a positive value when the device
    is already in use. When hi846_set_ctrl() passes &ret holding this
    positive value) to hi846_write_reg_16(), the function returns with ret
    as is, the positive value propagates back as a return code, which
    callers interpret as an error.
    
    Fix this by resetting *err to 0 only when it is positive.
    
    Fixes: 04fc06f6dc15 ("media: hi846: fix usage of pm_runtime_get_if_in_use()")

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

[Severity: High]
This is a pre-existing issue, but does hi846_set_ctrl() properly handle
negative errors like -EINVAL from pm_runtime_get_if_in_use() when CONFIG_PM
is disabled?

When CONFIG_PM is disabled, pm_runtime_get_if_in_use() returns -EINVAL.
Because hi846_set_ctrl() checks if (!ret || ret == -EAGAIN), it will fall
through and pass the -EINVAL via &ret to hi846_write_reg_16():

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

	switch (ctrl->id) {
    ...
	case V4L2_CID_VBLANK:
		/* Update FLL that meets expected vertical blanking */
		hi846_write_reg_16(hi846, HI846_REG_FLL,
				   hi846->cur_mode->height + ctrl->val, &ret);
		break;
    ...
}

Because hi846_write_reg_16() exits early on negative errors, won't this
skip all I2C register writes when CONFIG_PM is disabled?

>  	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/20260712115012.91600-1-mitltlatltl@gmail.com?part=1