Re: [PATCH 4/9] media: vimc: Ensure that pixel_rate fits in 32 bits

[Ricardo Ribalda]

Hi Faizel

I was not on cc on
https://lore.kernel.org/lkml/CAMuHMdVx80Eh8w5-OioH-Uyahea0ZRQHUVg9MBfQ4uEjaxM5Lg@xxxxxxxxxxxxxx/
so I was unaware of that proposal

I agree, something like this looks cleaner and removes the sanity check.

My patch has already landed in media-committers/next so you need to
send a patch on top of it.

Regards!




 diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c
b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 83dcc9d61ee0..7cb544fdfddb 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -97,18 +97,14 @@ static void vimc_sensor_update_frame_timing(struct
v4l2_subdev *sd,
 {
        struct vimc_sensor_device *vsensor =
                container_of(sd, struct vimc_sensor_device, sd);
-       u64 pixel_rate = vsensor->pixel_rate->val;
+       u32 pixel_rate = vsensor->pixel_rate->val;
        u32 hts = width + vsensor->hblank->val;
        u32 vts = height + vsensor->vblank->val;
        u64 total_pixels = (u64)hts * vts;
        u64 frame_interval_ns;

-       /* Sanity check, pixel rate is fixed and fits in 32 bits. */
-       if (WARN_ON(pixel_rate >= 0x100000000))
-               return;
-
        frame_interval_ns = total_pixels * NSEC_PER_SEC;
-       do_div(frame_interval_ns, (u32)pixel_rate);
+       do_div(frame_interval_ns, pixel_rate);
        vsensor->hw.fps_jiffies = nsecs_to_jiffies(frame_interval_ns);
        if (vsensor->hw.fps_jiffies == 0)
                vsensor->hw.fps_jiffies = 1;

On Sat, 25 Jul 2026 at 06:23, Faizel K B <faizel.kb@xxxxxxxxx> wrote:
>
> On Mon, Jun 29, 2026 at 11:30:45AM +0000, Ricardo Ribalda wrote:
> >pixel_rate is set to VIMC_PIXEL_RATE_FIXED, which the code expects to
> >fit in 32 bits. Make that constraint into a WARN_ON, so if we ever break
> >that constraint a kernel warning will be triggered.
> >
> >It also fixes the following cocci warning:
> >./test-drivers/vimc/vimc-sensor.c:107:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.
> >
> >Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> >---
> > drivers/media/test-drivers/vimc/vimc-sensor.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> >index d125a79fec8c..83dcc9d61ee0 100644
> >--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> >+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> >@@ -103,8 +103,12 @@ static void vimc_sensor_update_frame_timing(struct v4l2_subdev *sd,
> >       u64 total_pixels = (u64)hts * vts;
> >       u64 frame_interval_ns;
> >
> >+      /* Sanity check, pixel rate is fixed and fits in 32 bits. */
> >+      if (WARN_ON(pixel_rate >= 0x100000000))
> >+              return;
> >+
> >       frame_interval_ns = total_pixels * NSEC_PER_SEC;
> >-      do_div(frame_interval_ns, pixel_rate);
> >+      do_div(frame_interval_ns, (u32)pixel_rate);
>
> There was an error in the data type of pixel_rate. Actually it is u32 instead
> of u64. There are two changes suggested by Geert in reply to the original
> report. v4l2_ctrl.val is s32, not u64 and using mul_u32_u32() for total_pixels
> calculation. We can avoid the sanity check also. Would you be able to consider
> these two changes which avoid the warning?
>
> -       u64 pixel_rate = vsensor->pixel_rate->val;
> +       u32 pixel_rate = vsensor->pixel_rate->val;
>          u32 hts = width + vsensor->hblank->val;
>          u32 vts = height + vsensor->vblank->val;
> -       u64 total_pixels = (u64)hts * vts;
> +       u64 total_pixels = mul_u32_u32(hts, vts);
>          u64 frame_interval_ns;
>
> Link for the suggested changes
> https://lore.kernel.org/lkml/CAMuHMdVx80Eh8w5-OioH-Uyahea0ZRQHUVg9MBfQ4uEjaxM5Lg@xxxxxxxxxxxxxx/
>
>
>


-- 
Ricardo Ribalda