Re: [PATCH v3] media: v4l2-flash: Enter LED off state after file handle closed

[Jacek Anaszewski]

Hi Sakari,

On 2/23/26 10:43, Sakari Ailus wrote:
> Hi Jacek,
>
> On Sat, Feb 21, 2026 at 04:48:48PM +0100, Jacek Anaszewski wrote:
> > Hi ChiYuan,
> >
> > On 1/12/26 10:20, cy_huang@xxxxxxxxxxx wrote:
> > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > >
> > > To make sure LED enter off state after file handle is closed, initiatively
> > > configure LED_MODE to NONE. This can guarantee whatever the previous state
> > > is torch or strobe mode, the final state will be off.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 42bd6f59ae90 ("media: Add registration helpers for V4L2 flash sub-devices")
> > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > ---
> > > Still cannot pass patch integration check, send v3 patch to fix all.
> > >
> > > v3
> > > - Remove 'Reported-by' tag
> > > - Fix identation check for patch integration
> > >
> > > v2
> > > - Fix commit message redudant space cause patch robot parsing error
> > >
> > > Hi,
> > >     We encounter an issue. When the upper layer camera process is crashed,
> > > if the new process did not reinit the LED,  it will keeps the previous
> > > state whatever it's in torch or strobe mode
> > >
> > > OS will handle the resource management. So when the process is crashed
> > > or terminated, the 'close' API will be called to release resources.
> > > That's why we add the initiative action to trigger LED off in file
> > > handle close is called.
> > > ---
> > >    drivers/media/v4l2-core/v4l2-flash-led-class.c | 6 ++++++
> > >    1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > index 355595a0fefa..46606f5cc192 100644
> > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > @@ -623,6 +623,12 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > >    		return 0;
> > >    	if (led_cdev) {
> > > +		/* If file handle is released, make sure LED enter off state */
> > > +		ret = v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[LED_MODE],
> > > +				       V4L2_FLASH_LED_MODE_NONE);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > >    		mutex_lock(&led_cdev->led_access);
> > >    		if (v4l2_flash->ctrls[STROBE_SOURCE])
> > >
> > > base-commit: 8ac28a6642d1cc8bac0632222e66add800b027fa
> >
> > The patch itself looks good, but while at it I started wondering
> > if we shouldn't move below STROBE_SOURCE access before the lock.
> > I don't see now, why we placed it there.
> >
> > Adding Sakari.
>
> Thanks for cc'ing me.
>
> The behaviour this patch changes has been around for a decade or so. I
> wonder what breaks if we change it now. It works the same way as the sysfs
> LED API, too, and I think common behaviour between the two APIs was the
> reasoning back then as well.

The thing is that v4l2_flash_open() disables LED sysfs interface via
led_sysfs_disable() and v4l2_flash_close() enables it via 
led_sysfs_enable(). led_sysfs_{enable|disable}() modify the state of
LED_SYSFS_DISABLE flag.

Therefore it is only the led_sysfs_{enable|disable}() API that needs to
be called under led_access lock, since the LED subsystem sysfs handlers
test the LED_SYSFS_DISABLE flag under the same lock, and return -EBUSY
in case it is set.

The call to v4l2_flash_close() is synchronized internally in V4L2 core
I believe.

Therefore I think that we can safely move the
"if (v4l2_flash->ctrls[STROBE_SOURCE])" condition before the lock.

Otherwise we would have to put this new v4l2_ctrl_s_ctrl() call,
added in this patch also under the lock because why not. It would spark
questions in the future asking how it is different from the above
"v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[LED_MODE]" case.

--
Best regards,
Jacek Anaszewski