Re: [PATCH v2 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 return value check of pm_runtime_get_if_active() leads to unbalanced PM usage count and potential log-spam.
--

commit 77fd9b09419f467242cfd9a487b0ac45ff5344ef
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

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

This patch moves sensor-specific parameters into a new
struct og0ve1b_sensor_data to prepare for supporting additional
OmniVision sensors. The sensor data is selected at probe time
using i2c_get_match_data().

> diff --git a/drivers/media/i2c/og0ve1b.c b/drivers/media/i2c/og0ve1b.c
> index 84a28cdcade10..acc06b10bf896 100644
> --- a/drivers/media/i2c/og0ve1b.c
> +++ b/drivers/media/i2c/og0ve1b.c
[ ... ]
> @@ -261,23 +272,39 @@ static const struct og0ve1b_mode 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->sensor->modes[0];
>  	s64 exposure_max;
>  	int ret;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing
issue in this function regarding the return value check of
pm_runtime_get_if_active().

Later in og0ve1b_set_ctrl(), the code does:

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

Because pm_runtime_get_if_active() can return a negative error code
(like -EINVAL when PM is disabled), the logical NOT operator will evaluate
the negative error as false. This bypasses the error condition entirely,
allowing execution to proceed to I2C hardware register writes and
unconditionally calling pm_runtime_put().

Since the initial 'get' call did not increment the usage counter on error,
the subsequent 'put' call decrements it improperly. Could this cause an
unbalanced PM usage count underflow and trigger kernel warnings?

Should this condition be updated to explicitly check if the return value
is less than or equal to zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-og0va1b-v2-0-0071442caa2a@oss.qualcomm.com?part=2