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

[xiaolei wang]

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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxx/
> > > >
> > > > 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@xxxxxxxxxxxxxxxx/
> >
> > I'll cc him as well
> >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus