* [PATCH] leds: use QoS to control LED suspend behavior from userspace @ 2018-01-29 19:49 0v3rdr0n3 2018-01-30 8:28 ` Pavel Machek 2018-02-01 10:52 ` kbuild test robot 0 siblings, 2 replies; 10+ messages in thread From: 0v3rdr0n3 @ 2018-01-29 19:49 UTC (permalink / raw) To: linux-pm, linux-leds; +Cc: samorris 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-01-29 19:49 [PATCH] leds: use QoS to control LED suspend behavior from userspace 0v3rdr0n3 @ 2018-01-30 8:28 ` Pavel Machek 2018-01-30 14:55 ` Samuel Morris 2018-02-01 10:52 ` kbuild test robot 1 sibling, 1 reply; 10+ messages in thread From: Pavel Machek @ 2018-01-30 8:28 UTC (permalink / raw) To: 0v3rdr0n3; +Cc: linux-pm, linux-leds, samorris [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-01-30 8:28 ` Pavel Machek @ 2018-01-30 14:55 ` Samuel Morris 2018-02-01 21:29 ` Jacek Anaszewski 0 siblings, 1 reply; 10+ messages in thread From: Samuel Morris @ 2018-01-30 14:55 UTC (permalink / raw) To: Pavel Machek; +Cc: 0v3rdr0n3, Linux PM, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-01-30 14:55 ` Samuel Morris @ 2018-02-01 21:29 ` Jacek Anaszewski 2018-02-02 15:40 ` Samuel Morris 0 siblings, 1 reply; 10+ messages in thread From: Jacek Anaszewski @ 2018-02-01 21:29 UTC (permalink / raw) To: Samuel Morris, Pavel Machek; +Cc: 0v3rdr0n3, Linux PM, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-02-01 21:29 ` Jacek Anaszewski @ 2018-02-02 15:40 ` Samuel Morris 2018-02-02 20:52 ` Jacek Anaszewski 0 siblings, 1 reply; 10+ messages in thread From: Samuel Morris @ 2018-02-02 15:40 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Pavel Machek, Sam Morris, Linux PM, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-02-02 15:40 ` Samuel Morris @ 2018-02-02 20:52 ` Jacek Anaszewski 2018-02-02 22:19 ` Samuel Morris 0 siblings, 1 reply; 10+ messages in thread From: Jacek Anaszewski @ 2018-02-02 20:52 UTC (permalink / raw) To: Samuel Morris; +Cc: Pavel Machek, Sam Morris, Linux PM, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-02-02 20:52 ` Jacek Anaszewski @ 2018-02-02 22:19 ` Samuel Morris 2018-02-04 20:01 ` Jacek Anaszewski 0 siblings, 1 reply; 10+ messages in thread From: Samuel Morris @ 2018-02-02 22:19 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Pavel Machek, Sam Morris, Linux PM, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-02-02 22:19 ` Samuel Morris @ 2018-02-04 20:01 ` Jacek Anaszewski 2018-02-05 15:22 ` Samuel Morris 0 siblings, 1 reply; 10+ messages in thread From: Jacek Anaszewski @ 2018-02-04 20:01 UTC (permalink / raw) To: Samuel Morris Cc: Pavel Machek, Sam Morris, Linux PM, linux-leds, devicetree@vger.kernel.org, Rob Herring, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-02-04 20:01 ` Jacek Anaszewski @ 2018-02-05 15:22 ` Samuel Morris 0 siblings, 0 replies; 10+ messages in thread From: Samuel Morris @ 2018-02-05 15:22 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Sam Morris, Linux PM, linux-leds, devicetree@vger.kernel.org, Rob Herring, Rafael J. Wysocki, Rob Herring, Mark Rutland 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] leds: use QoS to control LED suspend behavior from userspace 2018-01-29 19:49 [PATCH] leds: use QoS to control LED suspend behavior from userspace 0v3rdr0n3 2018-01-30 8:28 ` Pavel Machek @ 2018-02-01 10:52 ` kbuild test robot 1 sibling, 0 replies; 10+ messages in thread From: kbuild test robot @ 2018-02-01 10:52 UTC (permalink / raw) To: 0v3rdr0n3; +Cc: kbuild-all, linux-pm, linux-leds, samorris [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-02-05 15:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-29 19:49 [PATCH] leds: use QoS to control LED suspend behavior from userspace 0v3rdr0n3 2018-01-30 8:28 ` Pavel Machek 2018-01-30 14:55 ` Samuel Morris 2018-02-01 21:29 ` Jacek Anaszewski 2018-02-02 15:40 ` Samuel Morris 2018-02-02 20:52 ` Jacek Anaszewski 2018-02-02 22:19 ` Samuel Morris 2018-02-04 20:01 ` Jacek Anaszewski 2018-02-05 15:22 ` Samuel Morris 2018-02-01 10:52 ` kbuild test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox