Re: [PATCH v3 0/2] Add stop callback to the cpufreq_driver interface.

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Wednesday, March 19, 2014 11:03:56 AM Viresh Kumar wrote:
> On 19 March 2014 06:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, March 18, 2014 12:25:14 PM Dirk Brandewie wrote:
> >> On 03/18/2014 12:08 PM, Srivatsa S. Bhat wrote:
> >> > On 03/18/2014 10:52 PM, dirk.brandewie@gmail.com wrote:
> >> >> From: Dirk Brandewie <dirk.j.brandewie@intel.com>
> >> >>
> >> >
> >> > I don't mean to nitpick, but generally its easier to deal with
> >> > patchsets if you post the subsequent versions in fresh email threads.
> >> > Otherwise it can get a bit muddled along with too many other email
> >> > discussions in the same thread :-(
> >> >
> >> >> Changes:
> >> >> v2->v3
> >> >> Changed the calling of the ->stop() callback to be conditional on the
> >> >> core being the last core controlled by a given policy.
> >> >>
> >> >
> >> > Wait, why? I'm sorry if I am not catching up with the discussions on
> >> > this issue quickly enough, but I don't see why we should make it
> >> > conditional on _that_. I thought we agreed that we should make it
> >> > conditional in the sense that ->stop() should be invoked only for
> >> > ->setpolicy drivers, right?
> >>
> >> This was done at Viresh's suggestion since thought there might be value
> >> for ->target drivers.
> >>
> >> Any of the options work for me
> >>     called only for set_policy scaling drivers
> >
> > And that's what we should do *today* in my opinion, unless we want to add
> > it to any ->target() drivers *right* now.  Do we want that?
> 
> We don't have a platform right now that might want to use it, but people
> are doing this during suspend and so there is a high possibility that they
> will use it while normal cpu offline as well..
> 
> This is what I think:
> - We are adding a new callback to struct cpufreq_driver..
> - We have a classic case here because a driver (set-policy ones) is both a
> driver and governor. And that's why its special..
> - All we want here is to give drivers a chance to do something useful on the
> CPUs that are going down..
> - There is nothing like GOV_STOP for setpolicy drivers as we never needed
> it and if we really want that, probably we better register setpolicy drivers as
> governors as well (only to a level where they would get GOV_START|STOP|etc)
> callbacks and nothing else.
> - And so I wanted the ->stop() callback to be more about the driver than the
> governor.
> - And because a policy contains only the CPUs that share clock line, its
> only required to call stop() for last CPU of the policy.

So you're still insisting on putting ->setpolicy and ->target drivers into one
bag, which in my opinion is a mistake.  Moreover, it has always been a mistake.

Let's add ->stop() (or whatever to call it) *specifically* for ->setpolicy
drivers, so that the meaning of it is entirely clear.  And *if* any ->target
drivers need a similar callback, let's add it for them *separately* (just as
a different callback pointer).

Yes, we'll potentially waste a pointer size worth of storage this way, but then
it will be clear who's supposed to use those new callbacks and when.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.