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

[Sakari Ailus]

Hi Jacek,

On Mon, Feb 23, 2026 at 07:32:16PM +0100, Jacek Anaszewski wrote:
> 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.

My concern really is that this is not a bugfix but a change of an existing
UAPI that's been out there for a decade. Such a change is likely to cause
troubles to the users. Let's see what others think.

-- 
Kind regards,

Sakari Ailus