Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[xiaolei wang <xiaolei.wang@windriver.com>]

Hi

On 12/2/25 00:06, Dave Stevenson wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Sakari
>
> On Mon, 1 Dec 2025 at 13:58, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>> Hello
>>
>> On Mon, Dec 01, 2025 at 03:02:11PM +0200, Sakari Ailus wrote:
>>> Hi Hans, Xiaolei,
>>>
>>> On Mon, Dec 01, 2025 at 10:31:59AM +0100, johannes.goede@oss.qualcomm.com wrote:
>>>> Hi,
>>>>
>>>> On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
>>>>> __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
>>>>> contains an assertion to verify that the v4l2_ctrl_handler::lock
>>>>> is held, as it should only be called when the lock has already
>>>>> been acquired. Therefore use our own mutex for the ctrl lock,
>>>>> otherwise a warning will be  reported.
>>>>>
>>>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>>>> Generally speaking as a default locking setup for sensor
>>>> drivers we are moving in the direction of removing driver
>>>> specific locks and instead using the control-handler
>>>> lock everywhere, including using it as the active state
>>>> lock, see e.g. :
>>>>
>>>> https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/
>>>>
>>>> which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
>>>> and then removes a bunch of manual mutex_lock / unlock calls
>>>> since all ops which get called with a sd_state will already
>>>> have the lock called when operating on the active_state
>>>> (and when called in try mode they should not touch anything
>>>> needing locking).
>>>>
>>>> Note if you also want to make the ctrl_handler lock
>>>> the active state lock then you need to add calls to
>>>> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
>>>> to allocate the active-state to probe().
>>> I agree with the above, but the driver is old and it uses its own lock to
>>> serialise access to its data structures while it uses the control lock
>>> separately. So this looks like a bugfix that could be backported.
>>>
>>> I wonder if anyone still has a system with this sensor.
>> ov5647 is the rpi camera module v1, so I guess it's still around even
>> if a bit old. Dave in cc is the expert and maintainer of this driver.
> Raspberry Pi stopped selling OV5647 in about 2016 after Omnivision
> gave a last-time-buy date in 2014/5, and we brought out the v2 camera
> module based on imx219. However there are still modules being sold
> even now - stick "OV5647" into eBay or Amazon and you'll get loads of
> hits.
>
> We still support the modules, but have little enthusiasm for investing
> significant development effort into it whilst it remains functional.
>
> As this is a bug fix for a genuine issue and has minimal impact, I'd
> be tempted to accept it. Reworking the driver to use the same mutex
> and all the subdev state can be done at a separate time (unless
> Xiaolei is really keen to do it now).
Thanks for the feedback and support. I appreciate the context about
the OV5647 module.

Currently, I think maintaining the current minimal fix is ​​better, as 
it also

facilitates stable backporting. The driver rework can be done separately

in the future

thanks

xiaolei

>
>    Dave
>
>> Jai has a series in review to upstream all the remaining BSP patches
>> for this driver.
>> https://lore.kernel.org/all/20251118-b4-rpi-ov5647-v2-0-5e78e7cb7f9b@ideasonboard.com/
>>
>> I'll cc him as well
>>
>>> --
>>> Regards,
>>>
>>> Sakari Ailus
>>>