Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace

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

Hi Samuel,

Thanks for the patch.

On 01/30/2018 03:55 PM, Samuel Morris wrote:
> On Tue, Jan 30, 2018 at 3:28 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> On Mon 2018-01-29 19:49:47, 0v3rdr0n3@gmail.com wrote:
>>> From: Samuel Morris <samorris@lexmark.com>
>>>
>>> Signed-off-by: Samuel Morris <samorris@lexmark.com>
>>
>> But... we'll really need description what this is supposed to do.
> 
> I have an LED that indicates to the user that the machine is still
> powered when in suspend, so it needs to remain powered. This LED uses
> the leds-pwm driver, but it may use a different driver on future
> products, so making this change in that driver only would not be
> ideal. I asked linux-pm a related question a week or two ago, and
> Raphael Wysocki suggested looking into the PM_QOS_FLAG_NO_POWER_OFF
> flag. This is what I came up with. I realize this is a pretty broad
> change, but I figured I'd try the most general thing first.
> 
>>
>> Because at least some LEDs (keyboard LEDs on PC) can't be powered on
>> during suspend.
> 
> That is why I set the default behavior to PM_QOS_FLAG_NO_POWER_OFF=0 on probe.
> 
>>
>> Does this work for your LEDs? Do we need a way for userspace to tell
>> if LED supports it or not?
> 
> Yes, this fixes my problem. I could try testing on other LEDs as well
> that use different drivers if need be.
> 
> I didn't see any reason not to make this userspace configurable. I
> imagine for some LEDs, the switch just won't work. Are you aware of
> any cases where attempting to keep an LED on would cause outright
> breakage? I would like these QoS parameters to be device tree, or
> otherwise per-board configurable, but I'm not aware of a standard way
> to do that. Maybe someone from linux-pm has an idea. Something like
> that might be more reasonable than allowing default userspace
> configurability.

There is already retain-state-suspended property defined in
Documentation/devicetree/bindings/leds/leds-gpio.txt and used
in drivers/leds/leds-gpio.c. Now if we are going to add generic
pm_qos support to the LED core, the DT property should be made generic
too and moved to the Documentation/devicetree/bindings/leds/common.txt.

Then, if the property is present, we shouldn't expose this setting
to the userspace, see below.

>>
>>> @@ -196,6 +197,11 @@ static int led_suspend(struct device *dev)
>>>  {
>>>       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>
>>> +     if(dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF) ==
>>> +                     PM_QOS_FLAGS_ALL) {
>>> +             return 0;
>>> +     }
>>
>> "if (". No need for { } s.
> 
> Ok, I'll generate a new patch later if this seems likely to be integrated.

Please also address problems detected by build bot [0].

>>> +
>>>       if (led_cdev->flags & LED_CORE_SUSPENDRESUME)
>>>               led_classdev_suspend(led_cdev);
>>>
>>> @@ -206,6 +212,11 @@ static int led_resume(struct device *dev)
>>>  {
>>>       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>
>>> +     if(dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF) ==
>>> +                     PM_QOS_FLAGS_ALL) {
>>> +             return 0;
>>> +     }
>>> +
>>>       if (led_cdev->flags & LED_CORE_SUSPENDRESUME)
>>>               led_classdev_resume(led_cdev);
>>>
>>> @@ -287,6 +298,18 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>>       list_add_tail(&led_cdev->node, &leds_list);
>>>       up_write(&leds_list_lock);
>>>
>>> +     /* Attempt to let userspace take over the policy. */
>>> +     ret = dev_pm_qos_expose_flags(led_cdev->dev,
>>> +                     PM_QOS_FLAG_NO_POWER_OFF);
>>> +     if (ret < 0) {
>>> +             dev_warn(led_cdev->dev, "failed to expose pm_qos_no_poweroff\n");
>>> +             return 0;
>>> +     }
>>> +
>>> +     ret = dev_pm_qos_update_flags(led_cdev->dev,
>>> +                     PM_QOS_FLAG_NO_POWER_OFF,
>>> +                     0);
>>> +

So this part should be executed conditionally only if
retain-state-suspended wasn't defined. BTW you will have
to rebase your code onto some more recent code base.

led_classdev_register() was renamed to of_led_classdev_register()
in 4.12 and it now accepts struct device_node. leds-gpio.c was
already changed to use it so it will be convenient to test the use case
with retain-state-suspended DT property.

>>>       if (!led_cdev->max_brightness)
>>>               led_cdev->max_brightness = LED_FULL;
>>
>> Best regards,
>>                                                                         Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
> thanks,
> Sam
> 

[0] https://www.spinics.net/lists/linux-leds/msg09031.html

-- 
Best regards,
Jacek Anaszewski