| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPY8ntDy0NmU5D6Q=v+X0nN9beGFLWHQp0jpfNYq+shGaam87g@mail.gmail.com>
Date: Mon, 3 Nov 2025 18:17:31 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Tarang Raval <tarang.raval@...iconsignals.io>
Cc: sakari.ailus@...ux.intel.com, Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control
range updates
Hi Tarang
On Fri, 31 Oct 2025 at 11:49, Tarang Raval
<tarang.raval@...iconsignals.io> wrote:
>
> Propagate return values from __v4l2_ctrl_modify_range() and
> __v4l2_ctrl_s_ctrl() in imx219_set_ctrl() and imx219_set_pad_format().
> This ensures proper error handling instead of ignoring possible
> failures. Also return the result of imx219_set_pad_format() from
> imx219_init_state().
It should be impossible for a number of these to fail unless someone
has messed up in updating the driver, but it does little harm in
returning the error code back out.
The slight hesitation would be that in imx219_set_pad_format you could
have updated the ranges/values of one or more controls, and then fail
leaving a partially implemented mode change. It has returned an error,
but an application would be reasonable in thinking that the previous
working state has been retained when it hasn't.
As long as it would only trigger due to a driver bug rather than user
interaction, I would *not* propose that all values have to be saved
and have to be restored on failure. It just gets too ugly.
> Signed-off-by: Tarang Raval <tarang.raval@...iconsignals.io>
Reviewed-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> ---
> drivers/media/i2c/imx219.c | 61 +++++++++++++++++++++++++-------------
> 1 file changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 48efdcd2a8f9..40693635c0c3 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -453,10 +453,14 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> exposure_max = format->height + ctrl->val - 4;
> exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> exposure_max : IMX219_EXPOSURE_DEFAULT;
> - __v4l2_ctrl_modify_range(imx219->exposure,
> - imx219->exposure->minimum,
> - exposure_max, imx219->exposure->step,
> - exposure_def);
> + ret = __v4l2_ctrl_modify_range(imx219->exposure,
> + imx219->exposure->minimum,
> + exposure_max,
> + imx219->exposure->step,
> + exposure_def);
> + if (ret)
> + return ret;
> +
> }
>
> /*
> @@ -848,6 +852,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_rect *crop;
> u8 bin_h, bin_v, binning;
> u32 prev_line_len;
> + int ret;
>
> format = v4l2_subdev_state_get_format(state, 0);
> prev_line_len = format->width + imx219->hblank->val;
> @@ -883,19 +888,28 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> int pixel_rate;
>
> /* Update limits and set FPS to default */
> - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> - IMX219_FLL_MAX - mode->height, 1,
> + ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> + IMX219_FLL_MAX - mode->height, 1,
> + mode->fll_def - mode->height);
> + if (ret)
> + return ret;
> +
> + ret = __v4l2_ctrl_s_ctrl(imx219->vblank,
> mode->fll_def - mode->height);
> - __v4l2_ctrl_s_ctrl(imx219->vblank,
> - mode->fll_def - mode->height);
> + if (ret)
> + return ret;
> +
> /* Update max exposure while meeting expected vblanking */
> exposure_max = mode->fll_def - 4;
> exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> exposure_max : IMX219_EXPOSURE_DEFAULT;
> - __v4l2_ctrl_modify_range(imx219->exposure,
> - imx219->exposure->minimum,
> - exposure_max, imx219->exposure->step,
> - exposure_def);
> + ret = __v4l2_ctrl_modify_range(imx219->exposure,
> + imx219->exposure->minimum,
> + exposure_max,
> + imx219->exposure->step,
> + exposure_def);
> + if (ret)
> + return ret;
>
> /*
> * With analog binning the default minimum line length of 3448
> @@ -906,9 +920,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> imx219_get_binning(state, &bin_h, &bin_v);
> llp_min = (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ?
> IMX219_BINNED_LLP_MIN : IMX219_LLP_MIN;
> - __v4l2_ctrl_modify_range(imx219->hblank, llp_min - mode->width,
> - IMX219_LLP_MAX - mode->width, 1,
> - llp_min - mode->width);
> + ret = __v4l2_ctrl_modify_range(imx219->hblank,
> + llp_min - mode->width,
> + IMX219_LLP_MAX - mode->width, 1,
> + llp_min - mode->width);
> + if (ret)
> + return ret;
> /*
> * Retain PPL setting from previous mode so that the
> * line time does not change on a mode change.
> @@ -917,13 +934,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> * mode width subtracted.
> */
> hblank = prev_line_len - mode->width;
> - __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> + if (ret)
> + return ret;
>
> /* Scale the pixel rate based on the mode specific factor */
> pixel_rate = imx219_get_pixel_rate(imx219) *
> imx219_get_rate_factor(state);
> - __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> - pixel_rate, 1, pixel_rate);
> + ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> + pixel_rate, 1, pixel_rate);
> + if (ret)
> + return ret;
> }
>
> return 0;
> @@ -972,9 +993,7 @@ static int imx219_init_state(struct v4l2_subdev *sd,
> },
> };
>
> - imx219_set_pad_format(sd, state, &fmt);
> -
> - return 0;
> + return imx219_set_pad_format(sd, state, &fmt);
> }
>
> static const struct v4l2_subdev_video_ops imx219_video_ops = {
> --
> 2.34.1
>
Powered by blists - more mailing lists