linux-kernel - Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEs41JAYytgJMD4L_ZVxAm4v29gQdjSvMrQ-jx5zE1TydtZ2WA@mail.gmail.com>
Date: Tue, 22 Jul 2025 12:39:23 -0700
From: Allen Ballway <ballway@...omium.org>
To: Hans de Goede <hansg@...nel.org>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

Hello,

On Tue, Jul 22, 2025 at 2:19 AM Hans de Goede <hansg@...nel.org> wrote:
>
> Hi,
>
> On 21-Jul-25 7:46 PM, Allen Ballway wrote:
> > Hello,
> >
> > On Mon, Jul 21, 2025 at 4:51 AM Hans de Goede <hansg@...nel.org> wrote:
> >>
> >> Hi,
> >>
> >> On 17-Jul-25 11:07 PM, Allen Ballway wrote:
> >>> Prevents ov8865_mode_binning_configure from overwriting the hflip
> >>> register values. Allows programs to configure the hflip.
> >>>
> >>> Signed-off-by: Allen Ballway <ballway@...omium.org>
> >>
> >> Thank you for your patch.
> >>
> >>> ---
> >>>  drivers/media/i2c/ov8865.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> >>> index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
> >>> --- a/drivers/media/i2c/ov8865.c
> >>> +++ b/drivers/media/i2c/ov8865.c
> >>> @@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> -     value = OV8865_FORMAT2_HSYNC_EN;
> >>> +     ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
> >>> +               OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
> >>> +     value |= OV8865_FORMAT2_HSYNC_EN;
> >>>
> >>>       if (mode->binning_x)
> >>>               value |= OV8865_FORMAT2_FST_HBIN_EN;
> >>
> >> this change should not be necessary. Lets assume we start
> >> with the sensor runtime-suspended, then ov8865_resume()
> >> will call:
> >>
> >> ov8865_sensor_power(true)
> >> ov8865_sensor_init()
> >>   ov8865_state_configure()
> >>     ov8865_mode_configure()
> >>       ov8865_mode_binning_configure()
> >> __v4l2_ctrl_handler_setup()
> >>
> >> Where the __v4l2_ctrl_handler_setup() call will apply
> >> all control settings including hflip.
> >>
> >> So unless you manage to hit a code-path where somehow
> >> ov8865_state_configure() gets called without calling
> >> __v4l2_ctrl_handler_setup() afterwards then this should
> >> not be necessary.
> >
> > ov8865_state_configure() is also being called from ov8865_set_fmt(),
> > and makes no calls to __v4l2_ctrl_handler_setup(). I'm not sure if
> > calling __v4l2_ctrl_handler_setup() here as well is the right fix, but
> > the driver ov8865 seems to be based upon, ov5648, seems to avoid
> > this issue by preserving the flip values when setting the binning
> > register values in ov5648_mode_configure by using
> > ov5648_update_bits() rather than ov5648_write(). I believe that we
> > just need to preserve the register values unrelated to binning inside
> > ov8865_mode_binning_configure, possibly by just using
> > ov8865_update_bits() instead of ov8865_write().
>
> But you cannot call ov8865_set_fmt() while streaming, since
> it has :
>
>         if (sensor->state.streaming) {
>                 ret = -EBUSY;
>                 goto complete;
>         }
>
> in there.
>
> And when not streaming the sensor is off. So inside ov8865_state_configure()
> the ov8865_mode_configure() and thus ov8865_mode_binning_configure()
> will be skipped since that is protected by if (!pm_runtime_suspended())
> as mentioned before this is all a bit messy in this driver and it would
> be good to untangle this a bit, I think the ov8865_mode_configure()
> should be moved out of ov8865_state_configure() and instead done
> separately on power-up.

I see now. ov8865_set_fmt() will be called multiple times after the sensor is
resumed but before it is streaming, calling through to ov8865_mode_configure()
and then stomping the register values. Moving the ov8865_mode_configure()
call out of ov8865_state_configure() and into ov8865_sensor_init() prevents
this and allows the flip to be configured correctly.

Thanks for the feedback, I'll send out a new patch for the above change soon.

Allen
>
> Regards,
>
> Hans
>