Re: s2idle breaks on machines without cpuidle support

[Kazuki]

On Wed, Feb 08, 2023 at 02:16:58PM +0000, Sudeep Holla wrote:
> On Wed, Feb 08, 2023 at 08:20:31PM +0900, Kazuki wrote:
> > On Wed, Feb 08, 2023 at 10:35:11AM +0000, Sudeep Holla wrote:
> > > On Wed, Feb 08, 2023 at 04:48:18AM +0900, Kazuki wrote:
> > > > On Mon, Feb 06, 2023 at 10:12:39AM +0000, Sudeep Holla wrote:
> > > > >
> > > > > What do you mean by break ? More details on the observation would be helpful.
> > > > For example, CLOCK_MONOTONIC doesn't stop even after suspend since
> > > > these chain of commands don't get called.
> > > >
> > > > call_cpuidle_s2idle->cpuidle_enter_s2idle->enter_s2idle_proper->tick_freeze->sched_clock_suspend (Function that pauses CLOCK_MONOTONIC)
> > > >
> > > > Which in turn causes programs like systemd to crash since it doesn't
> > > > expect this.
> > >
> > > Yes expected IIUC. The per-cpu timers and counters continue to tick in
> > > WFI and hence CLOCK_MONOTONIC can't stop.
> > Yes, but it shouldn't be the case when suspending[1]. Currently that's what
> > happens when we enter s2idle without a cpuidle driver. This doesn't seem
> > to happen with S3 sleep [2].
> >
> 
> Correct, but check the requirements to use syscore operations(mainly
> syscore_suspend/resume where only one CPU is online with interrupts
> disabled. In case of s2idle, all CPUs are idling and not offlined as
> required by the syscore operations and hence it can't be used.
> 
> I was about ask you earlier as why can't you implement just system
> suspend in PSCI where the last cpu just calls WFI if you are interested
> in system sleep state. Or you can implement CPU_SUSPEND with an additional
> retention state which enters PSCI implementation just to make sure there is
> an active cpuidle driver and the s2idle state machinery works as expected.
The machine I have (Macbook with Apple M1) doesn't have PSCI.
> It is built with those requirements and trying to work it out for WFI without
> any idle driver or firmware implementation to back it up is just not going
> to work.
Any reason why that's the case?

I guess we should ensure that systems without a cpuidle driver
will not suspend maybe around here then.

kernel/power/main.c:

static ssize_t mem_sleep_store(struct kobject *kobj, struct kobj_attribute *attr,
			       const char *buf, size_t n)
{
	suspend_state_t state;
	int error;

	error = pm_autosleep_lock();
	if (error)
		return error;

	if (pm_autosleep_state() > PM_SUSPEND_ON) {
		error = -EBUSY;
		goto out;
	}

	state = decode_suspend_state(buf, n);
	if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON)
		mem_sleep_current = state;
	else
		error = -EINVAL;

 out:
	pm_autosleep_unlock();
	return error ? error : n;
}
> 
> --
> Regards,
> Sudeep
Thanks,
Kazuki