Re: [PATCH v3 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect evaluation of pm_runtime_get_if_active() return value leads to unbalanced pm_runtime_put() and usage counter underflow when Runtime PM is disabled.
--

--- Patch [2]: [PATCH v3 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure ---
commit 7e9c88bb94dde2349f9f1531e2c8f8721c3dd997
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

    media: i2c: og0ve1b: Introduce per-sensor data structure

    Refactor the driver to separate sensor-specific data from the common
    driver code. Introduce a sensor_data structure, populated through
    i2c_get_match_data(), to hold the chip ID, MCLK frequency, link
    frequencies and supported modes.

> diff --git a/drivers/media/i2c/og0ve1b.c b/drivers/media/i2c/og0ve1b.c
> index 84a28cdcade10..041342fbe3c82 100644
> --- a/drivers/media/i2c/og0ve1b.c
> +++ b/drivers/media/i2c/og0ve1b.c
[ ... ]
> @@ -273,11 +287,21 @@ static int og0ve1b_enable_test_pattern(struct og0ve1b *og0ve1b, u32 pattern)
>  	return cci_write(og0ve1b->regmap, OG0VE1B_REG_PRE_ISP, val, NULL);
>  }
>  
> +static const struct og0ve1b_sensor_data og0ve1b_data = {
> +	.chip_id = OG0VE1B_CHIP_ID,
> +	.mclk_freq = OG0VE1B_MCLK_FREQ_24MHZ,
> +	.enable_test_pattern = og0ve1b_enable_test_pattern,
> +	.link_freq_menu = og0ve1b_link_freq_menu,
> +	.num_link_freqs = ARRAY_SIZE(og0ve1b_link_freq_menu),
> +	.modes = og0ve1b_supported_modes,
> +	.num_modes = ARRAY_SIZE(og0ve1b_supported_modes),
> +};
> +
>  static int og0ve1b_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct og0ve1b *og0ve1b = container_of(ctrl->handler, struct og0ve1b,
>  					       ctrl_handler);
> -	const struct og0ve1b_mode *mode = &supported_modes[0];
> +	const struct og0ve1b_mode *mode = &og0ve1b->data->modes[0];
>  	s64 exposure_max;
>  	int ret;
>  

[Severity: High]
This is a pre-existing issue, but does the pm_runtime_get_if_active()
check later in og0ve1b_set_ctrl() properly handle negative error codes?

The function currently checks the runtime PM state with:

	if (!pm_runtime_get_if_active(og0ve1b->dev))
		return 0;

If Runtime PM is disabled, pm_runtime_get_if_active() returns -EINVAL.
Since the logical NOT operator evaluates -EINVAL as false, the code
bypasses the early return and proceeds to the hardware accesses and the
pm_runtime_put() call at the end of the function.

Could this lead to usage counter underflows and unpowered hardware
accesses when the sensor isn't powered up? Should this be checked as
<= 0 instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-og0va1b-v3-0-de8e44455a42@oss.qualcomm.com?part=2