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: <CAEs41JAt5Hjp7G6LPr36e+BT0dp6RU5p25kzCwnwBpBfF-3dJw@mail.gmail.com>
Date: Mon, 21 Jul 2025 10:46:46 -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 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().

>
> Note the whole runtime-pm / calling of ov8865_sensor_init() /
> ov8865_state_configure() code in this driver is somewhat
> unusual. So it could be there is a bug there.
>
> But I don't believe that this patch is the correct fix.
>
> Regards,
>
> Hans

Thanks,
Allen
>
>
>
>
>
> >
> > ---
> > base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf
> > change-id: 20250717-su-94b187fa3d1e
> >
> > Best regards,
>