On 5 January 2018 at 13:57, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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.
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).
>
>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>> ---
>> 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.
>
> 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.
Kind regards
Uffe
