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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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@richtek.com wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> 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@vger.kernel.org
>>> Fixes: 42bd6f59ae90 ("media: Add registration helpers for V4L2 flash sub-devices")
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>> 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