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 for Android: free password hash cracker in your pocket

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

Message-ID: <2ea2202c-d9bf-4fc1-a33f-2565ebe1d425@kernel.org>
Date: Tue, 22 Jul 2025 11:19:19 +0200
From: Hans de Goede <hansg@...nel.org>
To: Allen Ballway <ballway@...omium.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

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.

Regards,

Hans