Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

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

On Monday, November 25, 2013 09:43:59 AM Dirk Brandewie wrote:
> On 11/25/2013 09:01 AM, Viresh Kumar wrote:
> > On 25 November 2013 22:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> >> IMHO this issue should be fixed in the scaling driver for the platform.
> >>
> >> The scaling driver sets policy->cur and fills in the frequency table and has
> >
> > Not anymore, policy->cur is set in the core for most of the drivers now.
> > Drivers just provide ->get() callbacks.
> >
> >> the ability to adjust the frequency of the CPU.
> >
> > I believe this kind of decisions should stay with the core, drivers should
> > just provide the backend instead of intelligence..
> >
> 
> 
> This is a platform specific bug fix AFAICT and belongs in a platform
> specific piece of code
> 
> 
> >> Letting this leak up into the core
> >> is wrong IMHO and just widens the window where the CPU will be running at
> >> the wrong frequency set by the bootloader.
> >
> > It doesn't stay there for long enough.. we get to this point in
> > cpufreq core just
> > after calling ->init(), and if the CPU is working without issues until
> > now, it will
> > stay stable for few more milliseconds.
> >
> 
> And this is where the scaling driver should detect and fix the issue in ->init()
> on platforms we know about today, What happens if platforms in the future are
> more sensitive to the issue?

So what should the core do if the driver is careless and lets the bug slip
through?  Should it blindly trust the driver and let go?

Honestly, I don't think so.

> >> Shouldn't there be a check to see if the problem exists at all?  Otherwise
> >> the core is setting a policy for ALL platform even those where the issue
> >> does
> >> not exist.
> >
> > That is taken care of by __cpufreq_driver_target(). It checks if we are
> > already running at requested frequency or not (after getting the next
> > higher frequency)... If current freq is present in table,
> > cpufreq_frequency_table_target() will return current frequency only for
> > policy->cur -1. And so we will not end up configuring hardware
> > unnecessarily.
> >
> 
> The core should not be working around bootloader bugs IMHO.  Silently
> fixing it is evil IMHO at a minimum the core should complain LOUDLY
> about this happening otherwise the bootloaders will have no incentive to
> get their act together.

Yes, we can add a WARN_ON() there.  Still, though, the core's responsibility
is to ensure that (a) either we can continue safely or (b) we can't, in which
case it should just fail the initialization.  Whether or not it should panic()
I'm not sure.

Thanks!

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