Re: Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, 7 August 2007 23:17, David Brownell wrote:
> On Tuesday 07 August 2007, Russell King wrote:
> 
> > 4. the description of the function implies that this function may be
> >    called when we are not suspending:
> > 
> >   + * On platforms that support reduced functionality operating states, the
> >   + * constraint may also need to be tested during resume() and probe() calls.
> 
> Right ... support for multiple "run" states will imply this same kind
> of predicate, and I couldn't justify chosing a name which forces the
> existence of more than one such predicate.  Ergo a generic name.
> 
> However, right now no platforms support them (cpufreq aside), so this
> text could be rather harmlessly removed:  the second half of the sentence
> doesn't apply to anything yet.  And maybe it *should* be removed, since
> such platforms might do arbitrarily surprising stuff ... they might use
> driver-level "reclock yourself" callbacks/notifiers (like the original
> DPM stuff).
> 
> 
> >    With SoCs with multiple power states affecting which clocks are
> >    available, and the need in point (2) for the architecture code to
> >    record which PM mode we're entering via the pm_ops set_target method,
> 
> Your point (2) was that it's "unclear" how that information becomes
> available.  But the presumption of (4) here is that it's set_target().
> So either it's clear enough, and (2) was wrong; or (4) has a false
> premise; or both.  From false premises, anything can be proven...
> 
> I'd certainly not expect pm_ops.set_target() to apply in any context
> other than the start of a suspend sequence.  Any platform choosing
> to implement multiple run states necessarily provides some more
> mechanisms...
> 
> (Purely as an example ... this is much the same kind of thing that goes
> on with cpufreq updating a CPU clock that's also exposed through the
> clock API:  multiple interfaces to the same PM-related state.  In both
> cases, the public interface doesn't say how the implementation might
> be done; it doesn't even care.)
> 
> 
> >    calling clk_must_disable() outside of the suspend methods results in
> >    this function essentially returning undefined values at driver probe
> >    time.  Note: there is no locking between driver probing and the
> >    set_target method.
> > 
> >    Moreover, if used in a driver probe() path, the return value could
> >    well depend on the _last_ system suspend state entered, and would be
> >    undefined for a system which hasn't been suspended from boot.
> 
> No, this platform maintains an invariant that the target PM state is
> always PM_SUSPEND_ON except while suspending.  (So does ACPI, as I
> recall.)  You're presuming a broken platform; but it's not broken...
> 
> Re locking, that issue would be addressed as part of supporting
> multiple run states.  I still don't see suspend ops being involved
> in non-suspend transitions.

Well, I'll go further and say that in principle the .suspend() callbacks have
nothing to do with non-suspend transitions.  In some situations they may
perform similar operations, but in that case the common code should be put
into a separate function and called _explicitly_ from the two places, to avoid
confusion.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth