On Fri, Jan 5, 2018 at 6:12 PM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
> On 5 January 2018 at 13:57, Rafael J. Wysocki <raf...@kernel.org> wrote:
>> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
>>> Currently the wakeup_path status flag becomes propagated from a child
>>> device to its parent device at __device_suspend(). This allows a driver
>>> dealing with a parent device to act on the flag from its ->suspend()
>>> callback.
>>>
>>> However, in situations when the wakeup_path status flag needs to be set
>>> from a ->suspend_late() callback, its value doesn't get propagated to the
>>> parent by the PM core. Let's address this limitation, by also propagating
>>> the flag at __device_suspend_late().
>>
>> This doesn't need to be done twice, so doing it once in
>> __device_suspend_late() should be sufficient.
>
> Unfortunately no.
>
> For example that would break drivers/ssb/pcihost_wrapper.c, as it uses
> the flag from its ->suspend() callback.
But what drivers/ssb/pcihost_wrapper.c does is just wrong!
I guess we'd need a special variant of pci_prepare_to_sleep() with an
extra "wakeup" arg for it to do the right thing and I don't see why it
cannot do all that in ->suspend_late.
> Also, I expect there may be more cases when the flag may be useful to
> check from a ->suspend() callback, rather than from a ->suspend_late()
> (or in later phase).
I'm not inclined to believe it until I see a convincing example. :-)
And it obviously won't take the later updates of power.wakeup_path into account.
Anyway, for the time being please at least add a comment next to the
first invocation of this routine to explain why it is needed in there.
>
>>
>>> Signed-off-by: Ulf Hansson <ulf.hans...@linaro.org>
>>> ---
>>> drivers/base/power/main.c | 33 +++++++++++++++++----------------
>>> 1 file changed, 17 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index 7aeb91d..612bf1b 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1476,6 +1476,22 @@ static pm_callback_t
>>> dpm_subsys_suspend_late_cb(struct device *dev,
>>> return callback;
>>> }
>>>
>>> +static void dpm_propagate_to_parent(struct device *dev)
>>> +{
>>> + struct device *parent = dev->parent;
>>> +
>>> + if (!parent)
>>> + return;
>>> +
>>> + spin_lock_irq(&parent->power.lock);
>>> +
>>> + parent->power.direct_complete = false;
>>> + if (dev->power.wakeup_path && !parent->power.ignore_children)
>>> + parent->power.wakeup_path = true;
>>> +
>>> + spin_unlock_irq(&parent->power.lock);
>>> +}
>>
>> Clearing the parent's direct_complete in __device_suspend_late() is
>> incorrect, because it potentially overwrites the value previously used
>> by __device_suspend() for the parent.
>
> That is already taken care of in __device_suspend_late(), with the below
> check:
>
> if (dev->power.syscore || dev->power.direct_complete)
> goto Complete;
>
> In other words, the dpm_propagate_to_parent() isn't called when
> dev->power.direct_complete is set.
Which means that the parent's direct_complete is only cleared when it
is unset already, so this isn't incorrect, but just pointless.
Well, "pointless" doesn't really sound good to me too ...
>>
>> IMO it also need not be done under parent->power.lock, however,
>> because the other parent's power. flags should not be updated in
>> parallel with this update anyway, so maybe just move the parent's
>> direct_complete update to __device_suspend(), rename
>> dpm_propagate_to_parent() to something like
>> dpm_propagate_wakeup_to_parent() and call it from
>> __device_suspend_late() only?
>
> I can do this, if you think this is more clear, but according to the
> above, it's shouldn't be needed.
Yes, please.
Apart from avoiding a pointless update of the parent's direct_complete
I think that it's better to keep it close to the update of
direct_complete for suppliers.
Thanks,
Rafael
