Re: [PATCH v5 2/5] tick: Add freeze timer events

[dbasehore .]

On Mon, Jul 17, 2017 at 6:33 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Tue, Jul 18, 2017 at 2:30 AM, dbasehore . <dbasehore@xxxxxxxxxxxx> wrote:
>> On Sat, Jul 15, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>>> On Thursday, July 13, 2017 03:58:53 PM dbasehore . wrote:
>>>> On Thu, Jul 13, 2017 at 8:09 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>>> > On Thu, Jul 13, 2017 at 9:32 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>>>> >> On Fri, Jul 07, 2017 at 05:03:00PM -0700, Derek Basehore wrote:
>>>> >>> Adds a new feature to tick to schedule wakeups on a CPU during freeze.
>>>> >>> This won't fully wake up the system (devices are not resumed), but
>>>> >>> allow simple platform functionality to be run during freeze with
>>>> >>> little power impact.
>>>> >>>
>>>> >>> This implementation allows an idle driver to setup a timer event with
>>>> >>> the clock event device when entering freeze by calling
>>>> >>> tick_set_freeze_event. Only one caller should exist for the function.
>>>> >>>
>>>> >>> tick_freeze_event_expired is used to check if the timer went off when
>>>> >>> the CPU wakes.
>>>> >>>
>>>> >>> The event is cleared by tick_clear_freeze_event.
>>>> >>
>>>> >> Why? What's wrong with using the RTC stuff? RTC should be able to wake
>>>> >> suspended systems, see RTCWAKE(8).
>>>> >
>>>> > The RTC interrupt is an SCI (on ACPI systems) and we don't handle it
>>>> > at this point, so we don't know what woke us up until we re-enable
>>>> > interrupt handlers and run the one for the SCI.
>>>>
>>>> To add to that point, RTC wake ups are valid for fully waking up the
>>>> system. The clock event wake up wasn't used for waking up the system
>>>> before, so we know that we don't have to check if it should wake up
>>>> the system entirely. The way rtc timers work right now, I think that
>>>> we'd have to go all the way through resume devices to figure out if we
>>>> should resume to user space or freeze again.
>>>
>>> Actually, that's not exactly the case any more.
>>>
>>> After some changes that went in for 4.13-rc1 there is an additional decision
>>> point in the resume path, after the noirq stage, where we can decide to go back
>>> to sleep if that's the right thing to do.
>>>
>>> This means that in principle you might hack the CMOS RTC driver to do something
>>> more sophisticated than just calling pm_wakeup_hard_event() in rtc_handler().
>>>
>>> That's ACPI-specific, but I think you have ACPI on all of the systems where the
>>> residency counders are going to be checked anyway.
>>
>> This will take more power than the current implementation I have.
>> While I'm fine with that since the power draw would probably be within
>> 100uW to 1mW, it's worth noting. This is because of the added overhead
>> of noirq suspend and resume which take a combined time of about 50 to
>> 100 ms depending on the platform. The implementation that used the
>> APIC uses about 3uW.
>
> That's correct, but I'm not quite sure how much of a difference it
> makes in practice.
>
>> Rather than make the change in rtc_handler for the CMOS RTC driver,
>> the change might be better in the drivers/rtc/interface.c code to
>> better handle multiple RTC alarms. For example, there might be 2
>> alarms set for the same time (one that won't wake the system and one
>> that will) or 2 alarms 1 second apart. In the later case, it's
>> possible that 1 second will pass before the second alarm is scheduled.
>> We need to make sure that the RTC irq runs before calling
>> dpm_suspend_noirq too.
>
> Well, I guess the choice will depend on which patch looks more
> straightforward. :-)

I could make a patch to try it out. I would probably add a flag to rtc
timers to indicate whether it wakes the system (default true). We
would have to add a sync with the rtc irq and the rtc irqwork. I would
probably add a rtc_timer_sync function that would flush the rtc irq
and flush the irqwork. I would call this after the freeze_ops sync
function since the sci irq needs to finish before syncing with the rtc
irq. Also, pm_wakeup_irq seems racy with the current implementation of
s2idle_loop since the RTC irq could be mistakenly set as pm_wakeup_irq
when something else actually triggered the full wakeup. Fortunately, I
don't think pm_wakeup_irq is used for anything except debugging, but
we might change that.

>
>> If I remember correctly, I proposed using the RTC to wakeup for this
>> check to you, but you recommended using the APIC instead. This was of
>> course before the additional decision point was added to freeze.
>
> Right.  That's why I recommended it at that time.
>
> Thanks,
> Rafael