Message-ID: <CAKohpo=PoAaYgLNEVkkTC6tzAH=5hKw4VWTsrjt2jQnr7+6Q_w@mail.gmail.com>
Date:	Sun, 17 Nov 2013 22:27:43 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	Patch Tracking <patches@...aro.org>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Lan Tianyu <tianyu.lan@...el.com>, Nishanth Menon <nm@...com>,
	jinchoi@...adcom.com,
	Sebastian Capella <sebastian.capella@...aro.org>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Subject: Re: [PATCH] cpufreq: suspend/resume governors with PM notifiers

On 17 November 2013 20:39, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> On Sunday, November 17, 2013 01:52:15 PM viresh kumar wrote:

>> Do you see anything extra that might stop working?
>
> Well, the code would be racy with the patch as is.  User space might manipulate
> the sysfs knobs in parallel with your PM notifiers, for example, and I'm not
> entirely sure what can happen then.  And the lock in there is pointless,
> because it doesn't prevent any races from happening.

Hmm..

>> A unrelated question here. Why are we offlining CPUs after suspending all the
>> devices? Because the problem Nishanth mentioned was that he required few
>> devices, i2c, to be available when CPUs are getting down. And there might be
>> similar requirements at other places too. Was there any specific bottleneck due
>> to which it is implemented this way?
>
> No, this is because the ACPI spec mandates powering down devices before CPUs
> during system suspend.  The way it is done today, however, I think we don't
> need to keep that ordering so strictly any more.  We definitely don't need to
> do that on non-ACPI systems.

Okay.. But different behavior for acpi and non-acpi at such core code doesn't
look great to me.. Lets see if we can work with existing code

> So while I hate the PM notifiers idea (sorry, but that's how it goes), I think
> it would be OK to suspend *some* devices after disabling CPUs (not all of them,
> of course).

Hmm...

> And as I said, I think it would be OK to introduce suspend/resume callbacks for
> CPU devices and use those callbacks to work around the ordering issues, when
> necessary.  The main point is that the changes made for this purpose should
> only affect systems where they are necessary and not everyone.  I don't want
> to change the way things work today in general in cpufreq too much unless they
> are plain bugs that affect everyone.
>
>> > We may introduce suspend_noirq and resume_noirq for cpu_subsys, for example,
>> > and handle things from there.  Or something similar.  But slapping PM notifiers
>> > on top of the existing code just because it appears to be easy (and making that
>> > code even more overdesigned than it already is this way) doesn't seem quite
>> > right.

Okay.. Even these notifiers would be fine for me. To make things more clear
before I start implementing them:
- What about implementing syscore's suspend_prepare and post_suspend?
- Or you want to extend only CPU subsystems notifiers? What notifiers exactly?
And at which places we want to issue them from?

>> - I have concerns with Tianyu's patch as policies should be better taken care of
>> in cpufreq core instead of passing them over to governors.
>
> Well, this is all too tangled anyway, but quite frankly I'm not sure if it is
> worth untangling at this point.  We're deprecating cpufreq anyway.

I don't really think cpufreq is going anywhere even after we move bits
into scheduler. All the backend CPUFreq stuff will probably stay as is,
as they can't go anywhere.

One thing that will surely go away is the governor's layer, which would
be directly embedded into scheduler.

Lets see how that goes..

>> - Not all platforms have problem with changing frequency during suspend/resume
>> and so we may not require disabling of governors for all of them. Probably can
>> add another field based on which we may/may-not disable governors from PM or
>> syscore notifiers.
>
> What exactly is wrong with adding suspend/resume callbacks to cpu_subsys?

Okay, so you were asking about extending following list: CPU_ONLINE,
CPU_UP_PREPARE, CPU_UP_CANCELED, CPU_DOWN_PREPARE, etc.. to
include suspend/resume ones as well?

Logically speaking, all existing ones does look correct as they are more or
less cpu related. But suspend/resume doesn't look any similar, Atleast to me.

Suspend/resume are system's state rather than CPU's.. We aren't suspending
or resuming CPUs, we are shutting them off.. So I thought maybe syscore ops
is a better place (which is already used by cpufreq)..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists