On Sun, Feb 4, 2018 at 3:01 PM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
> On 02/02/2018 11:19 PM, Samuel Morris wrote:
>> On Fri, Feb 2, 2018 at 3:52 PM, Jacek Anaszewski
>> <jacek.anaszewski@xxxxxxxxx> wrote:
>>> On 02/02/2018 04:40 PM, Samuel Morris wrote:
>>>> On Thu, Feb 1, 2018 at 4:29 PM, Jacek Anaszewski
>>>> <jacek.anaszewski@xxxxxxxxx> 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@xxxxxx> wrote:
>>>>>>> On Mon 2018-01-29 19:49:47, 0v3rdr0n3@xxxxxxxxx wrote:
>>>>>>>> From: Samuel Morris <samorris@xxxxxxxxxxx>
>>>>>>>>
>>>>>>>> Signed-off-by: Samuel Morris <samorris@xxxxxxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxxxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
