Re: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs across suspend/resume

[Rafael J. Wysocki]

On Tue, Nov 29, 2016 at 2:27 AM, Chen, Yu C <yu.c.chen@xxxxxxxxx> wrote:
> Hi,
>> -----Original Message-----
>> From: rjwysocki@xxxxxxxxx [mailto:rjwysocki@xxxxxxxxx] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Wednesday, November 23, 2016 7:03 AM

[cut]

>>
>> > +               work_on_cpu(cpu, tstate_pm_fn, &save);
>>
>> Does work_on_cpu() wait for the work to complete?
>>
> Yes, it might increase the suspend/resume time, a 'queue_work_on' might be better?

Not really, because you'd need to wait before doing the
put_online_cpus() anyway.

>> > +       put_online_cpus();
>> > +}
>> > +
>> > +static int tstate_suspend(struct notifier_block *nb,
>> > +                               unsigned long mode, void *_unused) {
>> > +       tstate_check(mode, true);
>> > +       return 0;
>> > +}
>> > +
>> > +static int tstate_resume(struct notifier_block *nb,
>> > +                               unsigned long mode, void *_unused) {
>> > +       tstate_check(mode, false);
>> > +       return 0;
>> > +}
>> > +
>> > +static int __init tstate_pm_init(void) {
>> > +       /*
>> > +        * tstate_suspend should save tstate after
>> > +        * thermal zone's update in thermal_pm_notify,
>> > +        * vice versa tstate_resume restore tstate before
>> > +        * thermal_pm_notify, thus the thermal framework
>> > +        * has a chance to re-adjust tstate according to the
>> > +        * temperature trend.
>> > +        */
>> > +       pm_notifier(tstate_suspend, -1);
>> > +       pm_notifier(tstate_resume, 1);
>>
>> I don't think this is going to do what you really want.
>>
>> Each of these notifiers is going to be invoked during both suspend and resume,
> Yes,
>
>> so I guess you only need one notifier?
>
> Here's my original thought:  tstate_suspend needs to be invoked after
>  thermal_pm_notify, which has a priority of '0',
> so the notifier of tstate_suspend should be lower than '0',
> thus '-1'. And the same for tstate_resume,
> it should be invoked before thermal_pm_notify,
> thus priority is '1' ?

If there's a dependency like that, it needs to be explicit.  That is,
thermal_pm_notify() needs to invoke your new code in the right order
with respect to what it does already.

Thanks,
Rafael