Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Heiner Kallweit]

In led_classdev_unregister the LED gets switched off.
This is fine when the driver module is removed but causes issues
when the physical LED device is removed (e.g. USB LED devices).

In case of the thingm driver (hid/hid-thingm.c) it complains with
ENODEV because the physical LED device is no longer available.

When I switched this driver to use the generic workqueue in the
LED core then this error was also propagated to set_brightness_delayed 
and it complains with "Setting an LED's brightness failed (-19)".

Recent commit d1aa577f5e19 [turn off the LED and wait for completion
on unregistering LED class device] tackled a first potential issue
in led_classdev_unregister but it seems like the case of removal
of the physical LED device hasn't been considered yet.

At a first glance I see no way for the LED core to tell between
the two unregister cases (driver module removal vs. physical LED
device removal), but maybe I miss something.

If we can't tell between the two cases them I'm not sure what's the
best solution: Not touching the brightness is general in
led_classdev_unregister, live with the situation as it is or add
a special handling for ENODEV.

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Milo Kim]

On 01/17/2016 06:34 AM, Heiner Kallweit wrote:
> In led_classdev_unregister the LED gets switched off.
> This is fine when the driver module is removed but causes issues
> when the physical LED device is removed (e.g. USB LED devices).
>
> In case of the thingm driver (hid/hid-thingm.c) it complains with
> ENODEV because the physical LED device is no longer available.

I'd like to understand this situation clearly.

rmmod hid_thingm
-> detach the USB LED device
-> -ENODEV is reported.

Is it correct? I think -ENODEV seems reasonable because HID device is 
gone. Could you tell us what the problem is?

> When I switched this driver to use the generic workqueue in the
> LED core then this error was also propagated to set_brightness_delayed
> and it complains with "Setting an LED's brightness failed (-19)".
>
> Recent commit d1aa577f5e19 [turn off the LED and wait for completion
> on unregistering LED class device] tackled a first potential issue
> in led_classdev_unregister but it seems like the case of removal
> of the physical LED device hasn't been considered yet.

What happens if resetting only this commit?

> At a first glance I see no way for the LED core to tell between
> the two unregister cases (driver module removal vs. physical LED
> device removal), but maybe I miss something.
>
> If we can't tell between the two cases them I'm not sure what's the
> best solution: Not touching the brightness is general in
> led_classdev_unregister, live with the situation as it is or add
> a special handling for ENODEV.

We may handle this by using internal flag in LED subsystem, but I'd like 
to understand what the problem is and what expected behavior is.

Best regards,
Milo

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Heiner Kallweit]

Am 18.01.2016 um 01:20 schrieb Milo Kim:
> On 01/17/2016 06:34 AM, Heiner Kallweit wrote:
>> In led_classdev_unregister the LED gets switched off.
>> This is fine when the driver module is removed but causes issues
>> when the physical LED device is removed (e.g. USB LED devices).
>>
>> In case of the thingm driver (hid/hid-thingm.c) it complains with
>> ENODEV because the physical LED device is no longer available.
> 
> I'd like to understand this situation clearly.
> 
> rmmod hid_thingm
> -> detach the USB LED device
> -> -ENODEV is reported.
> 
> Is it correct? I think -ENODEV seems reasonable because HID device is gone. Could you tell us what the problem is?
> 
Sure, this is reasonable. The problem is that this causes dev_err messages.
And if it's a normal situation it shouldn't result in error messages (at least not on error level).

>> When I switched this driver to use the generic workqueue in the
>> LED core then this error was also propagated to set_brightness_delayed
>> and it complains with "Setting an LED's brightness failed (-19)".
>>
>> Recent commit d1aa577f5e19 [turn off the LED and wait for completion
>> on unregistering LED class device] tackled a first potential issue
>> in led_classdev_unregister but it seems like the case of removal
>> of the physical LED device hasn't been considered yet.
> 
> What happens if resetting only this commit?
> 
It's not that something with this commit is wrong. The behaviour was the same before, I just mention it
because you addressed the same part of the code and maybe stumbled across the same issue.

>> At a first glance I see no way for the LED core to tell between
>> the two unregister cases (driver module removal vs. physical LED
>> device removal), but maybe I miss something.
>>
>> If we can't tell between the two cases them I'm not sure what's the
>> best solution: Not touching the brightness is general in
>> led_classdev_unregister, live with the situation as it is or add
>> a special handling for ENODEV.
> 
> We may handle this by using internal flag in LED subsystem, but I'd like to understand what the problem is and what expected behavior is.
> 
Ideally we would detect that the device is gone in the unregister function and skip trying to write to it.

> Best regards,
> Milo
> 
Regards, Heiner

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Jacek Anaszewski]

Hi Heiner, Milo,

On 01/18/2016 07:37 AM, Heiner Kallweit wrote:
> Am 18.01.2016 um 01:20 schrieb Milo Kim:
>> On 01/17/2016 06:34 AM, Heiner Kallweit wrote:
>>> In led_classdev_unregister the LED gets switched off.
>>> This is fine when the driver module is removed but causes issues
>>> when the physical LED device is removed (e.g. USB LED devices).
>>>
>>> In case of the thingm driver (hid/hid-thingm.c) it complains with
>>> ENODEV because the physical LED device is no longer available.
>>
>> I'd like to understand this situation clearly.
>>
>> rmmod hid_thingm
>> -> detach the USB LED device
>> -> -ENODEV is reported.
>>
>> Is it correct? I think -ENODEV seems reasonable because HID device is gone. Could you tell us what the problem is?
>>
> Sure, this is reasonable. The problem is that this causes dev_err messages.
> And if it's a normal situation it shouldn't result in error messages (at least not on error level).
>
>>> When I switched this driver to use the generic workqueue in the
>>> LED core then this error was also propagated to set_brightness_delayed
>>> and it complains with "Setting an LED's brightness failed (-19)".
>>>
>>> Recent commit d1aa577f5e19 [turn off the LED and wait for completion
>>> on unregistering LED class device] tackled a first potential issue
>>> in led_classdev_unregister but it seems like the case of removal
>>> of the physical LED device hasn't been considered yet.
>>
>> What happens if resetting only this commit?
>>
> It's not that something with this commit is wrong. The behaviour was the same before, I just mention it
> because you addressed the same part of the code and maybe stumbled across the same issue.
>
>>> At a first glance I see no way for the LED core to tell between
>>> the two unregister cases (driver module removal vs. physical LED
>>> device removal), but maybe I miss something.
>>>
>>> If we can't tell between the two cases them I'm not sure what's the
>>> best solution: Not touching the brightness is general in
>>> led_classdev_unregister, live with the situation as it is or add
>>> a special handling for ENODEV.
>>
>> We may handle this by using internal flag in LED subsystem, but I'd like to understand what the problem is and what expected behavior is.

Yes, this seems to be the best solution. Driver should set this flag
before calling led_classdev_unregister. If the flag is set the -ENODEV
error will not be reported. It could be named LED_HW_ABSENT for
instance. Feel free to propose other ideas.

> Ideally we would detect that the device is gone in the unregister function and skip trying to write to it.
>
>> Best regards,
>> Milo
>>
> Regards, Heiner
>
>
>


-- 
Best Regards,
Jacek Anaszewski

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Heiner Kallweit]

Am 18.01.2016 um 09:46 schrieb Jacek Anaszewski:
> Hi Heiner, Milo,
> 
> On 01/18/2016 07:37 AM, Heiner Kallweit wrote:
>> Am 18.01.2016 um 01:20 schrieb Milo Kim:
>>> On 01/17/2016 06:34 AM, Heiner Kallweit wrote:
>>>> In led_classdev_unregister the LED gets switched off.
>>>> This is fine when the driver module is removed but causes issues
>>>> when the physical LED device is removed (e.g. USB LED devices).
>>>>
>>>> In case of the thingm driver (hid/hid-thingm.c) it complains with
>>>> ENODEV because the physical LED device is no longer available.
>>>
>>> I'd like to understand this situation clearly.
>>>
>>> rmmod hid_thingm
>>> -> detach the USB LED device
>>> -> -ENODEV is reported.
>>>
>>> Is it correct? I think -ENODEV seems reasonable because HID device is gone. Could you tell us what the problem is?
>>>
>> Sure, this is reasonable. The problem is that this causes dev_err messages.
>> And if it's a normal situation it shouldn't result in error messages (at least not on error level).
>>
>>>> When I switched this driver to use the generic workqueue in the
>>>> LED core then this error was also propagated to set_brightness_delayed
>>>> and it complains with "Setting an LED's brightness failed (-19)".
>>>>
>>>> Recent commit d1aa577f5e19 [turn off the LED and wait for completion
>>>> on unregistering LED class device] tackled a first potential issue
>>>> in led_classdev_unregister but it seems like the case of removal
>>>> of the physical LED device hasn't been considered yet.
>>>
>>> What happens if resetting only this commit?
>>>
>> It's not that something with this commit is wrong. The behaviour was the same before, I just mention it
>> because you addressed the same part of the code and maybe stumbled across the same issue.
>>
>>>> At a first glance I see no way for the LED core to tell between
>>>> the two unregister cases (driver module removal vs. physical LED
>>>> device removal), but maybe I miss something.
>>>>
>>>> If we can't tell between the two cases them I'm not sure what's the
>>>> best solution: Not touching the brightness is general in
>>>> led_classdev_unregister, live with the situation as it is or add
>>>> a special handling for ENODEV.
>>>
>>> We may handle this by using internal flag in LED subsystem, but I'd like to understand what the problem is and what expected behavior is.
> 
> Yes, this seems to be the best solution. Driver should set this flag
> before calling led_classdev_unregister. If the flag is set the -ENODEV
> error will not be reported. It could be named LED_HW_ABSENT for
> instance. Feel free to propose other ideas.
> 
Setting such a flag from the driver might cause significant effort in different layers.
When we talk about thingm as an example, it uses the hid subsystem with the usbhid low level driver.
We would need a callback in the usbhid driver (to be notified when the device is unplugged)
and a way to propagate this event to the hid core.

Maybe simpler: We could ignore ENODEV errors if a function is called from led_classdev_unregister.
This way we wouldn't have to touch drivers. I think of something like this:

---
 drivers/leds/led-class.c | 2 ++
 drivers/leds/led-core.c  | 4 +++-
 include/linux/leds.h     | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 14139c3..aa84e5b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -245,6 +245,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	up_write(&led_cdev->trigger_lock);
 #endif
 
+	led_cdev->flags |= LED_UNREGISTERING;
+
 	/* Stop blinking */
 	led_stop_software_blink(led_cdev);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60..7a1bd98 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -98,7 +98,9 @@ static void set_brightness_delayed(struct work_struct *ws)
 						led_cdev->delayed_set_value);
 	else
 		ret = -ENOTSUPP;
-	if (ret < 0)
+	if (ret < 0 &&
+	    /* LED HW might have been unplugged, therefore don't warn */
+	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING)))
 		dev_err(led_cdev->dev,
 			"Setting an LED's brightness failed (%d)\n", ret);
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 4429887..b8ee1ad 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -39,6 +39,7 @@ struct led_classdev {
 
 	/* Lower 16 bits reflect status */
 #define LED_SUSPENDED		(1 << 0)
+#define LED_UNREGISTERING	(1 << 1)
 	/* Upper 16 bits reflect control information */
 #define LED_CORE_SUSPENDRESUME	(1 << 16)
 #define LED_BLINK_ONESHOT	(1 << 17)
-- 
2.7.0

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Milo Kim]

On 01/19/2016 05:52 AM, Heiner Kallweit wrote:
> Setting such a flag from the driver might cause significant effort in different layers.
> When we talk about thingm as an example, it uses the hid subsystem with the usbhid low level driver.
> We would need a callback in the usbhid driver (to be notified when the device is unplugged)
> and a way to propagate this event to the hid core.
>
> Maybe simpler: We could ignore ENODEV errors if a function is called from led_classdev_unregister.
> This way we wouldn't have to touch drivers. I think of something like this:

Well, simple solution is good but I'm thinking about more generic handling.


LED subsystem				HID LED driver
-------------				--------------
					Create a LED device

Registers an event notifier

					Device is unplugged,
					notify an event to LED
					subsystem

Notification callback sets
a flag which means HW is removed

Set-brightness scheduler work
function checks this flag and
ignore the brightness update


blocking_notifier_chain_register() and blocking_notifier_call_chain() 
helpers can be used for this implementation.

However, I'm not sure how much latency will exist between step 3 (device 
is unplugged) and step 5 (check the flag and ignore brightness-set).

Best regards,
Milo

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Heiner Kallweit]

Am 19.01.2016 um 01:11 schrieb Milo Kim:
> On 01/19/2016 05:52 AM, Heiner Kallweit wrote:
>> Setting such a flag from the driver might cause significant effort in different layers.
>> When we talk about thingm as an example, it uses the hid subsystem with the usbhid low level driver.
>> We would need a callback in the usbhid driver (to be notified when the device is unplugged)
>> and a way to propagate this event to the hid core.
>>
>> Maybe simpler: We could ignore ENODEV errors if a function is called from led_classdev_unregister.
>> This way we wouldn't have to touch drivers. I think of something like this:
> 
> Well, simple solution is good but I'm thinking about more generic handling.
> 
> 
> LED subsystem                HID LED driver
> -------------                --------------
>                     Create a LED device
> 
> Registers an event notifier
> 
>                     Device is unplugged,
>                     notify an event to LED
>                     subsystem
> 
> Notification callback sets
> a flag which means HW is removed
> 
> Set-brightness scheduler work
> function checks this flag and
> ignore the brightness update
> 
Most likely not the LED subsystem but the respective driver would have to register the notifier as only the driver
knows what kind of subsystem (usb, ..) is used.

I agree that this would be somewhat cleaner. However I have my doubts that the relatively small benefit of getting read of one
a little annoying error message justifies the required efforts.
> 
> blocking_notifier_chain_register() and blocking_notifier_call_chain() helpers can be used for this implementation.
> 
> However, I'm not sure how much latency will exist between step 3 (device is unplugged) and step 5 (check the flag and ignore brightness-set).
> 
At a first glance I dont't think this is an issue because the notifier chain is blocking.

> Best regards,
> Milo
> 
Regards, Heiner

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Jacek Anaszewski]

Hi Heiner,

On 01/18/2016 09:52 PM, Heiner Kallweit wrote:

>> Yes, this seems to be the best solution. Driver should set this flag
>> before calling led_classdev_unregister. If the flag is set the -ENODEV
>> error will not be reported. It could be named LED_HW_ABSENT for
>> instance. Feel free to propose other ideas.
>>
> Setting such a flag from the driver might cause significant effort in different layers.
> When we talk about thingm as an example, it uses the hid subsystem with the usbhid low level driver.
> We would need a callback in the usbhid driver (to be notified when the device is unplugged)
> and a way to propagate this event to the hid core.

> Maybe simpler: We could ignore ENODEV errors if a function is called from led_classdev_unregister.
> This way we wouldn't have to touch drivers. I think of something like this:

Ignoring -ENODEV errors for all devices would filter out also valid
error cases and hinder debugging.

How about adding a flag LED_HW_PLUGGABLE in addition to
LED_UNREGISTERING, and make the pluggable LED class drivers having set
it all the time. The -ENODEV error would be reported only
if !(LED_UNREGISTERING && LED_HW_PLUGGABLE).

-- 
Best Regards,
Jacek Anaszewski

Re: Problem with resetting LED in led_classdev_unregister in case of USB LED device removal

[Heiner Kallweit]

Am 19.01.2016 um 10:10 schrieb Jacek Anaszewski:
> Hi Heiner,
> 
> On 01/18/2016 09:52 PM, Heiner Kallweit wrote:
> 
>>> Yes, this seems to be the best solution. Driver should set this flag
>>> before calling led_classdev_unregister. If the flag is set the -ENODEV
>>> error will not be reported. It could be named LED_HW_ABSENT for
>>> instance. Feel free to propose other ideas.
>>>
>> Setting such a flag from the driver might cause significant effort in different layers.
>> When we talk about thingm as an example, it uses the hid subsystem with the usbhid low level driver.
>> We would need a callback in the usbhid driver (to be notified when the device is unplugged)
>> and a way to propagate this event to the hid core.
> 
>> Maybe simpler: We could ignore ENODEV errors if a function is called from led_classdev_unregister.
>> This way we wouldn't have to touch drivers. I think of something like this:
> 
> Ignoring -ENODEV errors for all devices would filter out also valid
> error cases and hinder debugging.
> 
> How about adding a flag LED_HW_PLUGGABLE in addition to
> LED_UNREGISTERING, and make the pluggable LED class drivers having set
> it all the time. The -ENODEV error would be reported only
> if !(LED_UNREGISTERING && LED_HW_PLUGGABLE).
> 
Sounds good, I'll send a patch including the additional LED_HW_PLUGGABLE flag.
The patch may apply cleanly only after applying a comment fix I sent on Jan 10th.
(led: core: fix misleading comment after workqueue removal from drivers)

Regards, Heiner