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

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

[0v3rdr0n3]

From: Samuel Morris <samorris@lexmark.com>

Signed-off-by: Samuel Morris <samorris@lexmark.com>
---
 drivers/leds/led-class.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f2b0a80..9e9e265 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
+#include <linux/pm_qos.h>
 #include <uapi/linux/uleds.h>
 #include "leds.h"
 
@@ -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 (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);
+
 	if (!led_cdev->max_brightness)
 		led_cdev->max_brightness = LED_FULL;
 
-- 
1.9.3

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

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.

Because at least some LEDs (keyboard LEDs on PC) can't be powered on
during suspend.

Does this work for your LEDs? Do we need a way for userspace to tell
if LED supports it or not?

> @@ -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.

> +
>  	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);
> +
>  	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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Samuel Morris]

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.

>
>> @@ -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.

>
>> +
>>       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);
>> +
>>       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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

Hi Samuel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.15]
[also build test ERROR on next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/0v3rdr0n3-gmail-com/leds-use-QoS-to-control-LED-suspend-behavior-from-userspace/20180201-142937
config: i386-randconfig-s0-201804 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   WARNING: modpost: missing MODULE_LICENSE() in drivers/mtd/nand/denali_pci.o
   see include/linux/module.h for more information
>> ERROR: "dev_pm_qos_update_flags" [drivers/leds/led-class.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30962 bytes --]

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

[Jacek Anaszewski]

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

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

[Samuel Morris]

On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> 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.

Sounds good, I'll start working on it. Though I can't help but think,
if all devices share this pm_qos interface, couldn't the device tree
interface be shared as well?

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

I'm okay with dt only configurability. Though, if you're suggesting we
should expose the flag to userspace based on whether or not it's dt
configurable, I'm not so sure. I think those decisions should be
independent. When you expose that flag with dev_pm_qos_expose_flags(),
you also expose other flags. I will probably just remove the userspace
configurability for now. That can go in a separate patch, and maybe
that can be configurable from the dt, or a kernel config parameter.

>
>>>
>>>> @@ -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].

Will do.

>
>>>> +
>>>>       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.

Sounds good.

>
>>>>       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

Thanks for the feedback,
Sam

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

[Jacek Anaszewski]

On 02/02/2018 04:40 PM, Samuel Morris wrote:
> On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> 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.
> 
> Sounds good, I'll start working on it. Though I can't help but think,
> if all devices share this pm_qos interface, couldn't the device tree
> interface be shared as well?

Could you please elaborate on that?

>> Then, if the property is present, we shouldn't expose this setting
>> to the userspace, see below.
> 
> I'm okay with dt only configurability. Though, if you're suggesting we
> should expose the flag to userspace based on whether or not it's dt
> configurable, I'm not so sure. I think those decisions should be
> independent. 

My concern here is backward compatibility - current users expect that
once the property is set in DT, the behavior on suspend is guaranteed
to not change.

> When you expose that flag with dev_pm_qos_expose_flags(),
> you also expose other flags.

How so? You're exposing only one flag:

dev_pm_qos_expose_flags(led_cdev->dev, PM_QOS_FLAG_NO_POWER_OFF)

I'm not familiar with this interface, though.

> I will probably just remove the userspace
> configurability for now. That can go in a separate patch, and maybe
> that can be configurable from the dt, or a kernel config parameter.

I'm fine with that.

>>>>
>>>>> @@ -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].
> 
> Will do.
> 
>>
>>>>> +
>>>>>       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.
> 
> Sounds good.
> 
>>
>>>>>       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
> 
> Thanks for the feedback,
> Sam
> 

-- 
Best regards,
Jacek Anaszewski

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

[Samuel Morris]

On Fri, Feb 2, 2018 at 3:52 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 02/02/2018 04:40 PM, Samuel Morris wrote:
>> On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>> 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.
>>
>> Sounds good, I'll start working on it. Though I can't help but think,
>> if all devices share this pm_qos interface, couldn't the device tree
>> interface be shared as well?
>
> Could you please elaborate on that?

It just seems odd to me that there does not seem to be a standard way
to set QoS parameters like PM_QOS_FLAG_NO_POWER_OFF from the device
tree. Each subsystem must define its own way of saying essentially the
same thing afaik: leds-gpio chose 'retain-state-suspended' but it
could be any variation on "don't power off this device in suspend".

>
>>> Then, if the property is present, we shouldn't expose this setting
>>> to the userspace, see below.
>>
>> I'm okay with dt only configurability. Though, if you're suggesting we
>> should expose the flag to userspace based on whether or not it's dt
>> configurable, I'm not so sure. I think those decisions should be
>> independent.
>
> My concern here is backward compatibility - current users expect that
> once the property is set in DT, the behavior on suspend is guaranteed
> to not change.

I see now, yes, in a later patch, if we do expose this to userspace,
it would only initialize to zero if there is no dt setting.

>
>> When you expose that flag with dev_pm_qos_expose_flags(),
>> you also expose other flags.
>
> How so? You're exposing only one flag:
>
> dev_pm_qos_expose_flags(led_cdev->dev, PM_QOS_FLAG_NO_POWER_OFF)

Oh, yes, you're right, I don't know what I was thinking...

>
> I'm not familiar with this interface, though.
>
>> I will probably just remove the userspace
>> configurability for now. That can go in a separate patch, and maybe
>> that can be configurable from the dt, or a kernel config parameter.
>
> I'm fine with that.
>
>>>>>
>>>>>> @@ -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].
>>
>> Will do.
>>
>>>
>>>>>> +
>>>>>>       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.
>>
>> Sounds good.
>>
>>>
>>>>>>       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
>>
>> Thanks for the feedback,
>> Sam
>>
>
> --
> Best regards,
> Jacek Anaszewski

thanks,
Sam

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

[Jacek Anaszewski]

On 02/02/2018 11:19 PM, Samuel Morris wrote:
> On Fri, Feb 2, 2018 at 3:52 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 02/02/2018 04:40 PM, Samuel Morris wrote:
>>> On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
>>> <jacek.anaszewski@gmail.com> wrote:
>>>> 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.
>>>
>>> Sounds good, I'll start working on it. Though I can't help but think,
>>> if all devices share this pm_qos interface, couldn't the device tree
>>> interface be shared as well?
>>
>> Could you please elaborate on that?
> 
> It just seems odd to me that there does not seem to be a standard way
> to set QoS parameters like PM_QOS_FLAG_NO_POWER_OFF from the device
> tree. Each subsystem must define its own way of saying essentially the
> same thing afaik: leds-gpio chose 'retain-state-suspended' but it
> could be any variation on "don't power off this device in suspend".

Well, that seems to be a question to pm and devicetree guys.

>>>> Then, if the property is present, we shouldn't expose this setting
>>>> to the userspace, see below.
>>>
>>> I'm okay with dt only configurability. Though, if you're suggesting we
>>> should expose the flag to userspace based on whether or not it's dt
>>> configurable, I'm not so sure. I think those decisions should be
>>> independent.
>>
>> My concern here is backward compatibility - current users expect that
>> once the property is set in DT, the behavior on suspend is guaranteed
>> to not change.
> 
> I see now, yes, in a later patch, if we do expose this to userspace,
> it would only initialize to zero if there is no dt setting.

I'd just not expose PM_QOS_FLAG_NO_POWER_OFF at all if DT provides
the property but I think it needs broader discussion and involvement
of pm and DT people.

Adding devicetree@vger.kernel.org and maintainers.

>>> When you expose that flag with dev_pm_qos_expose_flags(),
>>> you also expose other flags.
>>
>> How so? You're exposing only one flag:
>>
>> dev_pm_qos_expose_flags(led_cdev->dev, PM_QOS_FLAG_NO_POWER_OFF)
> 
> Oh, yes, you're right, I don't know what I was thinking...
> 
>>
>> I'm not familiar with this interface, though.
>>
>>> I will probably just remove the userspace
>>> configurability for now. That can go in a separate patch, and maybe
>>> that can be configurable from the dt, or a kernel config parameter.
>>
>> I'm fine with that.
>>
>>>>>>
>>>>>>> @@ -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].
>>>
>>> Will do.
>>>
>>>>
>>>>>>> +
>>>>>>>       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.
>>>
>>> Sounds good.
>>>
>>>>
>>>>>>>       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
>>>
>>> Thanks for the feedback,
>>> Sam
>>>
>>
>> --
>> Best regards,
>> Jacek Anaszewski
> 
> thanks,
> Sam
> 

-- 
Best regards,
Jacek Anaszewski

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

[Samuel Morris]

On Sun, Feb 4, 2018 at 3:01 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 02/02/2018 11:19 PM, Samuel Morris wrote:
>> On Fri, Feb 2, 2018 at 3:52 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>> On 02/02/2018 04:40 PM, Samuel Morris wrote:
>>>> On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>> 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.
>>>>
>>>> Sounds good, I'll start working on it. Though I can't help but think,
>>>> if all devices share this pm_qos interface, couldn't the device tree
>>>> interface be shared as well?
>>>
>>> Could you please elaborate on that?
>>
>> It just seems odd to me that there does not seem to be a standard way
>> to set QoS parameters like PM_QOS_FLAG_NO_POWER_OFF from the device
>> tree. Each subsystem must define its own way of saying essentially the
>> same thing afaik: leds-gpio chose 'retain-state-suspended' but it
>> could be any variation on "don't power off this device in suspend".
>
> Well, that seems to be a question to pm and devicetree guys.

Yes. And further, another thing that's bothered me, I noticed this in
linux/Documentation/power/runtime_pm.txt:

Once the subsystem-level suspend callback (or the driver suspend
callback, if invoked directly) has completed successfully for the
given device, the PM core regards the device as suspended, which need
not mean that it has been put into a low power state.  It is supposed
to mean, however, that the device will not process data and will not
communicate with the CPU(s) and RAM until the appropriate resume
callback is executed for it.  The runtime PM status of a device after
successful execution of the suspend callback is 'suspended'.

I assume this applies to both runtime_suspend and system suspend. I
imagine we might not be able to make the guarantee that if we allow
any led(or any device for that matter) to remain powered that it will
not communicate with CPU(s) and RAM. Is that so? Could that cause
serious breakage if so? In my case, the pwm led does not, but if it
did communicate with CPU/RAM, and there was no serious breakage, I
would like to be able to make the decision to keep it powered.
Otherwise I simply wouldn't be able to use system suspend at all
without significant hardware changes. Maybe I'm misunderstanding
something basic though...

>
>>>>> Then, if the property is present, we shouldn't expose this setting
>>>>> to the userspace, see below.
>>>>
>>>> I'm okay with dt only configurability. Though, if you're suggesting we
>>>> should expose the flag to userspace based on whether or not it's dt
>>>> configurable, I'm not so sure. I think those decisions should be
>>>> independent.
>>>
>>> My concern here is backward compatibility - current users expect that
>>> once the property is set in DT, the behavior on suspend is guaranteed
>>> to not change.
>>
>> I see now, yes, in a later patch, if we do expose this to userspace,
>> it would only initialize to zero if there is no dt setting.
>
> I'd just not expose PM_QOS_FLAG_NO_POWER_OFF at all if DT provides
> the property but I think it needs broader discussion and involvement
> of pm and DT people.
>
> Adding devicetree@vger.kernel.org and maintainers.
>
>>>> When you expose that flag with dev_pm_qos_expose_flags(),
>>>> you also expose other flags.
>>>
>>> How so? You're exposing only one flag:
>>>
>>> dev_pm_qos_expose_flags(led_cdev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>>
>> Oh, yes, you're right, I don't know what I was thinking...
>>
>>>
>>> I'm not familiar with this interface, though.
>>>
>>>> I will probably just remove the userspace
>>>> configurability for now. That can go in a separate patch, and maybe
>>>> that can be configurable from the dt, or a kernel config parameter.
>>>
>>> I'm fine with that.
>>>
>>>>>>>
>>>>>>>> @@ -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].
>>>>
>>>> Will do.
>>>>
>>>>>
>>>>>>>> +
>>>>>>>>       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.
>>>>
>>>> Sounds good.
>>>>
>>>>>
>>>>>>>>       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
>>>>
>>>> Thanks for the feedback,
>>>> Sam
>>>>
>>>
>>> --
>>> Best regards,
>>> Jacek Anaszewski
>>
>> thanks,
>> Sam
>>
>
> --
> Best regards,
> Jacek Anaszewski
>

thanks for the feedback,
Sam