[RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > Based on David's patch
> > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > I slightly changed it.
> > > 
> > > Add a helper routine, which gets the sleep state of a ACPI device.
> > 
> > Is it going to work with the recent code ordering changes?  I mean,
> > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > the hibernation), so the target ACPI state is not known when the drivers'
> > .suspend() routines are being called.
> Not. Could pm_message_t have a member indicating the suspend state?

Well, I thought about that, but I did't know what people on linux-pm would
think about that.

Alternatively, we could introduce a pm_target() global PM operation that will
set the target sleep state for the entire system.

I think we should discuss that on linux-pm before any decision is made.

Greetings,
Rafael


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

[RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > Based on David's patch
> > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > I slightly changed it.
> > > 
> > > Add a helper routine, which gets the sleep state of a ACPI device.
> > 
> > Is it going to work with the recent code ordering changes?  I mean,
> > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > the hibernation), so the target ACPI state is not known when the drivers'
> > .suspend() routines are being called.
> Not. Could pm_message_t have a member indicating the suspend state?

Well, I thought about that, but I did't know what people on linux-pm would
think about that.

Alternatively, we could introduce a pm_target() global PM operation that will
set the target sleep state for the entire system.

I think we should discuss that on linux-pm before any decision is made.

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:

> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.
> 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.
> 
> I think we should discuss that on linux-pm before any decision is made.

Pardon me for asking what may be a dumb question.  Why does ACPI (or
anything else) need to know the target system state in order to decide
how a device should be suspended or whether wakeup should be enabled?

Alan Stern

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Wednesday, 20 June 2007 16:08, Alan Stern wrote:
> On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:
> 
> > > Not. Could pm_message_t have a member indicating the suspend state?
> > 
> > Well, I thought about that, but I did't know what people on linux-pm would
> > think about that.
> > 
> > Alternatively, we could introduce a pm_target() global PM operation that will
> > set the target sleep state for the entire system.
> > 
> > I think we should discuss that on linux-pm before any decision is made.
> 
> Pardon me for asking what may be a dumb question.  Why does ACPI (or
> anything else) need to know the target system state in order to decide
> how a device should be suspended or whether wakeup should be enabled?

The question isn't so dumb. ;-)

For each device (handled by it) ACPI defines _SxD and _SxW methods returning
the highest power (lowest number) D-state supported by the device in the
(system-wide) state Sx and the lowest power (highest number) D-state in which
the device can wake up the system being in the Sx sleep state, respectively.
The target power state of the device should be determined on the basis of these
values (along with the device's wake up setting) and they depend on the target
system sleep state.

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Len Brown]

On Wednesday 20 June 2007 07:32, Rafael J. Wysocki wrote:
> On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > Based on David's patch
> > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > I slightly changed it.
> > > > 
> > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > 
> > > Is it going to work with the recent code ordering changes?  I mean,
> > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > the hibernation), so the target ACPI state is not known when the drivers'
> > > .suspend() routines are being called.
> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.
> 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.
> 
> I think we should discuss that on linux-pm before any decision is made.

okay, this thread include linux-pm....

I support the proposal that pm_message_t include the target state
in addition to the phase of entering that state.
The reasoning is simple, device drivers that receive a pm_message_t
will do different things depending on the target state.
The example at hand is ACPI devices that need to know how deep a D-state
to go into based on the S-state, and this in-turn depends on if they
are enabled as wakeup devices or not.

thanks,
-Len

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Alan Stern wrote:
> On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:
> 
> Pardon me for asking what may be a dumb question.  Why does ACPI (or
> anything else) need to know the target system state in order to decide
> how a device should be suspended or whether wakeup should be enabled?

Because the things that distinguish different states are
the particular resources that are available in that state.

A device that needs resource X to be active (e.g. to wake
that part of the system) can't stay active in states where
X is unavailable.  A device that won't issue wakeup events
can probably enter lower power states.

I think it'd be pure confusion to care about whether wakeup
"should" be enabled in this state versus that one.  Enable it
when it's requested and possible, but otherwise there's nothing
to be done.  Runtime PM policies will mostly avoid device states
where wakeup can't work (unless it's disabled for that device),
but if the system enters a state where it can't, tough!


One canonical example is portions of the clock tree that
are available in one state but not another.  My pet example
being the USB clock, often 48 MHz, not being available in
deeper sleep states ... e.g.

  http://lkml.org/lkml/2007/3/22/241
  http://lkml.org/lkml/2007/3/22/242

It's routine for system power states to care about clocks
like that.  PXA 25x docs are probably where I first ran
across that issue, but docs for pretty much any SOC will
talk first about clocks when discussing power management.
(Current flows when clocks tick ...)


Another is power domains.  ACPI talks about those (but not
clocks!) as board level things ... e.g. turn off this power
supply on the mainboard.  Newer SOCs must manage these for
on-chip devices too, since newer manufacturing processes
(with smaller feature sizes) involve higher leakage current
(flowing even when clocks don't tick).

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Rafael J. Wysocki wrote:
> On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > Based on David's patch
> > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > I slightly changed it.
> > > > 
> > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > 
> > > Is it going to work with the recent code ordering changes?  I mean,
> > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > the hibernation), so the target ACPI state is not known when the drivers'
> > > .suspend() routines are being called.
>
> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.

Let's get rid of pm_message_t entirely.  Didn't we already discuss
how the main reasons for it will vanish if drivers get new PM methods?

 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.

I hope you mean "get the target state"!!

If drivers actually need a handle on that state, that'd be a fair
approach; make it an opaque type though, platform-specific.

But actually I don't see much point to having such a struct.  What
matters is the attributes of the target state (what resources will
be present, especially), and that rarely needs to be indicated by
any kind of cookie.  Consider the "current" task ... it's implicit,
always present (except in IRQ contexts), and hardly ever accessed
despite being more fundamental than "target PM state".

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Len Brown wrote:

> I support the proposal that pm_message_t include the target state
> in addition to the phase of entering that state.
> The reasoning is simple, device drivers that receive a pm_message_t
> will do different things depending on the target state.

They should do that based on attributes of the target state,
not any particular notion (e.g. from ACPI) of what the states
may be ...


> The example at hand is ACPI devices that need to know how deep a D-state
> to go into based on the S-state, and this in-turn depends on if they
> are enabled as wakeup devices or not.

None of that needs to involve growing pm_message_t (yeech!),
or even knowing that this system's PM infrastructure uses ACPI.

The ACPI bits need to know about ACPI target state, agreed, but
such stuff can (and should!) be interfaces that normal drivers
never see.

In fact, that's why pci_choose_state() exists.  Its implementation
has always sucked, sure, but not the notion that ACPI can answer
the question following APCI rules, while other PCI platforms can
answer the question using other (non-ACPI) rules.  And likewise
for other device types, including SOC devices.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 09:04, David Brownell wrote:
> On Wednesday 20 June 2007, Rafael J. Wysocki wrote:
> > On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > > Based on David's patch
> > > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > > I slightly changed it.
> > > > > 
> > > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > > 
> > > > Is it going to work with the recent code ordering changes?  I mean,
> > > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > > the hibernation), so the target ACPI state is not known when the drivers'
> > > > .suspend() routines are being called.
> >
> > > Not. Could pm_message_t have a member indicating the suspend state?
> > 
> > Well, I thought about that, but I did't know what people on linux-pm would
> > think about that.
> 
> Let's get rid of pm_message_t entirely.  Didn't we already discuss
> how the main reasons for it will vanish if drivers get new PM methods?
> 
>  
> > Alternatively, we could introduce a pm_target() global PM operation that will
> > set the target sleep state for the entire system.
> 
> I hope you mean "get the target state"!!
> 
> If drivers actually need a handle on that state, that'd be a fair
> approach; make it an opaque type though, platform-specific.
> 
> But actually I don't see much point to having such a struct.  What
> matters is the attributes of the target state (what resources will
> be present, especially), and that rarely needs to be indicated by
> any kind of cookie.  Consider the "current" task ... it's implicit,
> always present (except in IRQ contexts), and hardly ever accessed
> despite being more fundamental than "target PM state".

The issue at hand is that some device drivers may need to know what the
target sleep state of the system will be when their .suspend() routines are
being executed.  Currently, there's no means of passing that information to the
drivers and my question is how to do this.

IMO it can be done in two different ways:
1) via a .suspend() argument
2) via a global variable that the drivers can read.

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Pavel Machek]

Hi!

> > > > Not. Could pm_message_t have a member indicating the suspend state?
> > > 
> > > Well, I thought about that, but I did't know what people on linux-pm would
> > > think about that.
> > 
> > Let's get rid of pm_message_t entirely.  Didn't we already discuss
> > how the main reasons for it will vanish if drivers get new PM methods?
> > 
> >  
> > > Alternatively, we could introduce a pm_target() global PM operation that will
> > > set the target sleep state for the entire system.
> > 
> > I hope you mean "get the target state"!!
> > 
> > If drivers actually need a handle on that state, that'd be a fair
> > approach; make it an opaque type though, platform-specific.
> > 
> > But actually I don't see much point to having such a struct.  What
> > matters is the attributes of the target state (what resources will
> > be present, especially), and that rarely needs to be indicated by
> > any kind of cookie.  Consider the "current" task ... it's implicit,
> > always present (except in IRQ contexts), and hardly ever accessed
> > despite being more fundamental than "target PM state".
> 
> The issue at hand is that some device drivers may need to know what the
> target sleep state of the system will be when their .suspend() routines are
> being executed.  Currently, there's no means of passing that information to the
> drivers and my question is how to do this.
> 
> IMO it can be done in two different ways:
> 1) via a .suspend() argument
> 2) via a global variable that the drivers can read.

Just do 1). Global variables are ugly, and we already have space in
pm_message_t. 
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

Hi,

On Thursday, 21 June 2007 15:03, Pavel Machek wrote:
> Hi!
> 
> > > > > Not. Could pm_message_t have a member indicating the suspend state?
> > > > 
> > > > Well, I thought about that, but I did't know what people on linux-pm would
> > > > think about that.
> > > 
> > > Let's get rid of pm_message_t entirely.  Didn't we already discuss
> > > how the main reasons for it will vanish if drivers get new PM methods?
> > > 
> > >  
> > > > Alternatively, we could introduce a pm_target() global PM operation that will
> > > > set the target sleep state for the entire system.
> > > 
> > > I hope you mean "get the target state"!!
> > > 
> > > If drivers actually need a handle on that state, that'd be a fair
> > > approach; make it an opaque type though, platform-specific.
> > > 
> > > But actually I don't see much point to having such a struct.  What
> > > matters is the attributes of the target state (what resources will
> > > be present, especially), and that rarely needs to be indicated by
> > > any kind of cookie.  Consider the "current" task ... it's implicit,
> > > always present (except in IRQ contexts), and hardly ever accessed
> > > despite being more fundamental than "target PM state".
> > 
> > The issue at hand is that some device drivers may need to know what the
> > target sleep state of the system will be when their .suspend() routines are
> > being executed.  Currently, there's no means of passing that information to the
> > drivers and my question is how to do this.
> > 
> > IMO it can be done in two different ways:
> > 1) via a .suspend() argument
> > 2) via a global variable that the drivers can read.
> 
> Just do 1). Global variables are ugly, and we already have space in
> pm_message_t. 

Well, this is what Len voted for, I think.  David is against it.

I also think that the cleanest way would be to pass that as an argument
to .suspend(), but currently pm_message_t. is passed by value and if we
made it a real struct (ie. with more than one field), that would also become
ugly, IMHO.

So, can we make pm_message_t consist only of the target state?

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> The issue at hand is that some device drivers may need to know what the
> target sleep state of the system will be when their .suspend() routines are
> being executed.  Currently, there's no means of passing that information to the
> drivers and my question is how to do this.

Actually what they need to know is some *attribute* of that state.

They really don't care what the state is.  The $SUBJECT patch isn't
driver code ... it's for platform hooks that expose attributes to
the drivers.  Specifically, it's ACPI code, talking to drivers that
must run on non-ACPI systems.  Any driver that thinks it needs to
understand anything about ACPI states is sadly broken.

Remember also that the Linux "states" (in /sys/power/state) are an
inadequate representation of what most hardware can do.  Common
hardware can support a lot more low power sleep modes than the two
states Linux currently defines ... a limitation inherited from
first APM, and them more recently ACPI, which doesn't fit embedded
systems well at all.

- Dave

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, Rafael J. Wysocki wrote:

> > Just do 1). Global variables are ugly, and we already have space in
> > pm_message_t. 
> 
> Well, this is what Len voted for, I think.  David is against it.
> 
> I also think that the cleanest way would be to pass that as an argument
> to .suspend(), but currently pm_message_t. is passed by value and if we
> made it a real struct (ie. with more than one field), that would also become
> ugly, IMHO.
> 
> So, can we make pm_message_t consist only of the target state?

You're both repeating the mistakes from two years ago.

You can't use a simple type to describe a target system state.  While 
it might work for ACPI states, it won't work on general (i.e., 
non-ACPI) systems.  It's probably a mistake even to use a data 
structure to describe a system state, since the requirements are so 
complex.  The only reasonable approach is to describe it in code.

What you can do is this: Pick a small enumerated set of labels for some 
selected system states.  For example:

enum pm_system_state {
	PM_STATE_RUNNING,
	PM_STATE_STANDBY,
	PM_STATE_SUSPEND,
	PM_STATE_HIBERNATE,
};

(It might be preferable to make the list more configurable, perhaps 
even allow changes at runtime.  Never mind all that for now.)

These are merely labels, they don't actually describe anything.  To use 
them, you would have to pass them to a subsystem routine for 
interpretation.  For example, pci_select_state() might pass one of 
these things to an ACPI routine, which would know what ACPI state 
corresponded to the given pm_system_state and would be able to say what 
D-state would be appropriate for a given PCI device.  On a non-ACPI 
platform, pci_select_state() would have to call a different routine -- 
something platform-dependent -- to do the same job.


On the other hand, maybe you don't need anything like this at all.  
What would happen if a PCI driver put its device into a D-state which 
wasn't supported under the target ACPI state?  Would it be so terrible?
I can imagine that the requested wakeup functionality might not be 
available, but would it prevent the device from working properly when 
it was resumed?

Alan Stern

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

> > IMO it can be done in two different ways:
> > 1) via a .suspend() argument
> > 2) via a global variable that the drivers can read.

For sufficiently small values of "two" that is.  

Other solutions that have been described on the PM list include

  3) Providing accessors to the information actually needed
     in drivers ... e.g. say whether this clock or power domain
     will be available in that target state.

  4) Act more like "current" ... there's a function returning
     whatever "state" struct is settled on.  (But ideally
     without the pseudo-global.)

I'm amused that nobody really reacted to the technical comments in
my previous posts on this thread.  That's unfortunate, since from
where I sit it feels to me like everyone else is a johnny-come-lately
on this issue, and is now grasping at the quickest and dirtiest ways
to work around the issue instead of coming to grasp with the various
underlying issues.

IMO #3 is strongly preferable.


> Just do 1). Global variables are ugly, and we already have space in
> pm_message_t. 

There is no space in the ugly pm_message_t structure.  Adding to
that would involve creating a **larger** structure and passing it
around on the stack all the time.

Pavel, I know that for some perverse reason you actually like
that structure, and the notion of passing it around *BY VALUE*
instead of by reference.  But that approach has never been
universally acclaimed, and has in fact always had opposition;
the only way you got it merged in the first place was to send
in mountains of patches and ignore the negative feedback.

But I really thought the discussion on new PM methods, back a
couple months now, was going to finally get rid of that.

- Dave

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, David Brownell wrote:

> If drivers actually need a handle on that state, that'd be a fair
> approach; make it an opaque type though, platform-specific.
> 
> But actually I don't see much point to having such a struct.  What
> matters is the attributes of the target state (what resources will
> be present, especially), and that rarely needs to be indicated by
> any kind of cookie.

Your meaning isn't clear.  You just said there isn't much point in 
having a struct, and now you say that there rarely needs to be a 
cookie.  Did you mean to say that a cookie can do a better job of 
encapsulating the information than a struct can?

System suspend is different from runtime suspend in that the 
requirements are passed from the top down: "The whole system is going 
to enter this state, so prepare yourself".  Not "All your children are 
in low-power states, so feel free to reduce your own power level".  A 
driver can't rely on purely local information to know what will happen.

If a driver wants to find out whether some resource will be available
in the target system state, the only way is to ask the resource's
provider.  Hence the driver needs to have some cookie (representing the
target state) that it can pass to the provider.

Alan Stern

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Alan Stern wrote:
> On Thu, 21 Jun 2007, David Brownell wrote:
> 
> > If drivers actually need a handle on that state, that'd be a fair
> > approach; make it an opaque type though, platform-specific.
> > 
> > But actually I don't see much point to having such a struct.  What
> > matters is the attributes of the target state (what resources will
> > be present, especially), and that rarely needs to be indicated by
> > any kind of cookie.
> 
> Your meaning isn't clear.  You just said there isn't much point in 
> having a struct, and now you say that there rarely needs to be a 
> cookie.  Did you mean to say that a cookie can do a better job of 
> encapsulating the information than a struct can?

Struct, cookie, it's all the same thing.  Nothing is really needed.
The contents would be platform-specific, so there's no point in
having a struct vs some other identifier.

Remember that system suspend/sleep states are global transitions, so
there's no value in having a struct/cookie/... to help distinguish
between multiple concurrent transitions.  There is at most one such
transition under way at a time.  It's implicit.

There's no point in having *ANY* identifier.  That's good, since
updating all the APIs to pass such an identifier -- top to bottom of
all driver and framework stacks -- would be a lot of make-work.  Not
much code currently cares about how the various states differ.

As I think Linus also observed not that long ago, most driver writers
are having a hard time even understanding how to recover from power-off
"suspend" states.  Now, we need to get way beyond that for at least
some systems.  And to help those systems create the base of experience
that other developers can build on.  A key part of that is ensuring
that smarter drivers can be written, while not inflicting complexity
on other systems (and driver writers still struggling with 'resume').


> System suspend is different from runtime suspend in that the 
> requirements are passed from the top down: "The whole system is going 
> to enter this state, so prepare yourself".  Not "All your children are 
> in low-power states, so feel free to reduce your own power level".  A 
> driver can't rely on purely local information to know what will happen.

Right, the same global info applies ... globally.

 
> If a driver wants to find out whether some resource will be available
> in the target system state, the only way is to ask the resource's
> provider.  Hence the driver needs to have some cookie (representing the
> target state) that it can pass to the provider.

Not true.  The provider knows the target state.  Just ask it whether
the resource will be available.  It doesn't need a cookie to distinguish
between multiple target states, since there can be only one.

ACPI-aware providers will of course have to live within the constraints
of ACPI, which include knowing that there are only a handful of possible
states -- numbered S0, S1, S3, S4, S5 -- and that details of those states
are computed by interpreting AML bytecodes.  Those providers can talk to
the ACPI subsystem to get whatever info they need ... not just calling the
AML interpreter, but other details like what S-state is involved.

Providers for non-ACPI systems will naturally work quite differently,
and (by observation!) provide different kinds of resources.  Like clocks,
to pick just one of the more basic examples.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:

> So, can we make pm_message_t consist only of the target state?

Let's just finally get rid of pm_message_t.

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Pavel Machek]

Hi!

> > > IMO it can be done in two different ways:
> > > 1) via a .suspend() argument
> > > 2) via a global variable that the drivers can read.
> 
> For sufficiently small values of "two" that is.  
> 
> Other solutions that have been described on the PM list include
> 
>   3) Providing accessors to the information actually needed
>      in drivers ... e.g. say whether this clock or power domain
>      will be available in that target state.
> 
>   4) Act more like "current" ... there's a function returning
>      whatever "state" struct is settled on.  (But ideally
>      without the pseudo-global.)
> 
> I'm amused that nobody really reacted to the technical comments in
> my previous posts on this thread.  That's unfortunate, since from
> where I sit it feels to me like everyone else is a johnny-come-lately
> on this issue, and is now grasping at the quickest and dirtiest ways
> to work around the issue instead of coming to grasp with the various
> underlying issues.

Well, rest of the word is still using PC here, so johny-come-lately
may not be completely unexpected.

Could you push framework for some embedded system you care about? OLPC
by chance?

> IMO #3 is strongly preferable.

3) actually looks ok to me. For acpi it would mean

int acpi_state_we_are_entering(void)

returning S1..S4, right?

> But I really thought the discussion on new PM methods, back a
> couple months now, was going to finally get rid of that.

Umm, well, when someone gets to implement that...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 18:35, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> 
> > So, can we make pm_message_t consist only of the target state?
> 
> Let's just finally get rid of pm_message_t.

Fine.  How to we solve the problem at hand, then?


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 17:37, David Brownell wrote:
> 
> > > IMO it can be done in two different ways:
> > > 1) via a .suspend() argument
> > > 2) via a global variable that the drivers can read.
> 
> For sufficiently small values of "two" that is.  
> 
> Other solutions that have been described on the PM list include
> 
>   3) Providing accessors to the information actually needed
>      in drivers ... e.g. say whether this clock or power domain
>      will be available in that target state.

Well, you need to store that information somewhere.  The way in which
it will be provided to drivers is a secondary thing.

To me, the most important question is whether we want to pass that information
as a .suspend() argument or in any different way, which involves the use of a
global variable (or a set of variables) and that's 2).

>   4) Act more like "current" ... there's a function returning
>      whatever "state" struct is settled on.  (But ideally
>      without the pseudo-global.)

How would you be going to arrange that in practice?

> I'm amused that nobody really reacted to the technical comments in
> my previous posts on this thread.  That's unfortunate, since from
> where I sit it feels to me like everyone else is a johnny-come-lately
> on this issue, and is now grasping at the quickest and dirtiest ways
> to work around the issue instead of coming to grasp with the various
> underlying issues.
> 
> IMO #3 is strongly preferable.

Of course we can do that.  At least I don't have any objections.

Greetings,
Rafael


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

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Pavel Machek wrote:
> Hi!
> 
> > > > IMO it can be done in two different ways:
> > > > 1) via a .suspend() argument
> > > > 2) via a global variable that the drivers can read.
> > 
> > For sufficiently small values of "two" that is.  
> > 
> > Other solutions that have been described on the PM list include
> > 
> >   3) Providing accessors to the information actually needed
> >      in drivers ... e.g. say whether this clock or power domain
> >      will be available in that target state.
> > 
> >   4) Act more like "current" ... there's a function returning
> >      whatever "state" struct is settled on.  (But ideally
> >      without the pseudo-global.)
> > 
> > I'm amused that nobody really reacted to the technical comments in
> > my previous posts on this thread.  That's unfortunate, since from
> > where I sit it feels to me like everyone else is a johnny-come-lately
> > on this issue, and is now grasping at the quickest and dirtiest ways
> > to work around the issue instead of coming to grasp with the various
> > underlying issues.
> 
> Well, rest of the word is still using PC here, so johny-come-lately
> may not be completely unexpected.

True.  I could hardly escape noticing this problem though, given
what it takes to get USB remote wakeup working on various systems.
We've had a few years now to get that infrastructure in place.


> Could you push framework for some embedded system you care about? OLPC
> by chance?

I'll probably push those two patches (clock core, AT91 implementation)
since there seemed to be no objections.  I don't work on OLPC.  :)


> > IMO #3 is strongly preferable.
> 
> 3) actually looks ok to me. For acpi it would mean
> 
> int acpi_state_we_are_entering(void)
> 
> returning S1..S4, right?

My original patch included acpi_get_target_sleep_state()
returning ACPI_STATE_Sx values, yes.  However, that was
purely internal to ACPI-aware logic ... it was used to
implement pci_choose_state().

Drivers would never make such ACPI calls themselves, they'd
use pci_choose_state() instead.


And the clk_must_disable() call is another instance of the
same approach as pci_choose_state():  drivers getting access
to the PM-reated information they need, without needing to
use platform-specific calls or care about "what the target
sleep state is".

 
> > But I really thought the discussion on new PM methods, back a
> > couple months now, was going to finally get rid of that.
> 
> Umm, well, when someone gets to implement that...

Oh.  *THAT* little problem.  Sorry, I thought it was in the works.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 16:48, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> > The issue at hand is that some device drivers may need to know what the
> > target sleep state of the system will be when their .suspend() routines are
> > being executed.  Currently, there's no means of passing that information to the
> > drivers and my question is how to do this.
> 
> Actually what they need to know is some *attribute* of that state.

Generally, yes.

> They really don't care what the state is.  The $SUBJECT patch isn't
> driver code ... it's for platform hooks that expose attributes to
> the drivers.  Specifically, it's ACPI code, talking to drivers that
> must run on non-ACPI systems.  Any driver that thinks it needs to
> understand anything about ACPI states is sadly broken.

But finally it has to place the device into a specific state and that state
needs to be determined somehow.  And, if ACPI is involved, it needs to be
told at one point if the final system state is supposed to be S1 or S2 or S3
(I guess we can discard S2 safely) just to tell the driver which power states
of the device are suitable.

That's why I thought we might introduce an additional global pm_op that
could be used to provide ACPI with that information without engaging drivers in
passing it.

> Remember also that the Linux "states" (in /sys/power/state) are an
> inadequate representation of what most hardware can do.

Do we really want to introduce more system sleep states right at this point?

> Common hardware can support a lot more low power sleep modes than the two
> states Linux currently defines ... a limitation inherited from first APM, and
> them more recently ACPI, which doesn't fit embedded systems well at all.

I agree with that, but right now we have an ACPI-related problem at hand.

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> On Thursday, 21 June 2007 16:48, David Brownell wrote:

> > They really don't care what the state is.  The $SUBJECT patch isn't
> > driver code ... it's for platform hooks that expose attributes to
> > the drivers.  Specifically, it's ACPI code, talking to drivers that
> > must run on non-ACPI systems.  Any driver that thinks it needs to
> > understand anything about ACPI states is sadly broken.
> 
> But finally it has to place the device into a specific state and that state
> needs to be determined somehow. 

I suppose I'm still thinking that the approach in my original
patch works Just Fine.  Layering is kind of like this, going
from top to bottom (and omitting the go-to-pci-hardware stack,
and the initial ACPI pm hook before suspension starts):

	PM infrastructure ... calling suspend() for everything

	PCI bus support ... translates to PCI-specific typed call

	PCI driver ... suspend() calling pci_choose_state()

	ACPI support for PCI ... implementing choose_state()

	ACPI core code ... remembering ACPI_STATE_Sx, calling AML

That is, ACPI gets invoked at various points, but the driver and
core code doesn't need to know ACPI from Rumpelstiltskin.

- Dave

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:03, David Brownell wrote:
> On Thursday 21 June 2007, Pavel Machek wrote:
> > Hi!
> > 
> > > > > IMO it can be done in two different ways:
> > > > > 1) via a .suspend() argument
> > > > > 2) via a global variable that the drivers can read.
> > > 
> > > For sufficiently small values of "two" that is.  
> > > 
> > > Other solutions that have been described on the PM list include
> > > 
> > >   3) Providing accessors to the information actually needed
> > >      in drivers ... e.g. say whether this clock or power domain
> > >      will be available in that target state.
> > > 
> > >   4) Act more like "current" ... there's a function returning
> > >      whatever "state" struct is settled on.  (But ideally
> > >      without the pseudo-global.)
> > > 
> > > I'm amused that nobody really reacted to the technical comments in
> > > my previous posts on this thread.  That's unfortunate, since from
> > > where I sit it feels to me like everyone else is a johnny-come-lately
> > > on this issue, and is now grasping at the quickest and dirtiest ways
> > > to work around the issue instead of coming to grasp with the various
> > > underlying issues.
> > 
> > Well, rest of the word is still using PC here, so johny-come-lately
> > may not be completely unexpected.
> 
> True.  I could hardly escape noticing this problem though, given
> what it takes to get USB remote wakeup working on various systems.
> We've had a few years now to get that infrastructure in place.
> 
> 
> > Could you push framework for some embedded system you care about? OLPC
> > by chance?
> 
> I'll probably push those two patches (clock core, AT91 implementation)
> since there seemed to be no objections.  I don't work on OLPC.  :)
> 
> 
> > > IMO #3 is strongly preferable.
> > 
> > 3) actually looks ok to me. For acpi it would mean
> > 
> > int acpi_state_we_are_entering(void)
> > 
> > returning S1..S4, right?
> 
> My original patch included acpi_get_target_sleep_state()
> returning ACPI_STATE_Sx values, yes.  However, that was
> purely internal to ACPI-aware logic ... it was used to
> implement pci_choose_state().
> 
> Drivers would never make such ACPI calls themselves, they'd
> use pci_choose_state() instead.
> 
> 
> And the clk_must_disable() call is another instance of the
> same approach as pci_choose_state():  drivers getting access
> to the PM-reated information they need, without needing to
> use platform-specific calls or care about "what the target
> sleep state is".
> 
>  
> > > But I really thought the discussion on new PM methods, back a
> > > couple months now, was going to finally get rid of that.
> > 
> > Umm, well, when someone gets to implement that...
> 
> Oh.  *THAT* little problem.  Sorry, I thought it was in the works.

In fact it is, but I had no time to complete it yet.

Greetings,
Rafael


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

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:22, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> > On Thursday, 21 June 2007 16:48, David Brownell wrote:
> 
> > > They really don't care what the state is.  The $SUBJECT patch isn't
> > > driver code ... it's for platform hooks that expose attributes to
> > > the drivers.  Specifically, it's ACPI code, talking to drivers that
> > > must run on non-ACPI systems.  Any driver that thinks it needs to
> > > understand anything about ACPI states is sadly broken.
> > 
> > But finally it has to place the device into a specific state and that state
> > needs to be determined somehow. 
> 
> I suppose I'm still thinking that the approach in my original
> patch works Just Fine.  Layering is kind of like this, going
> from top to bottom (and omitting the go-to-pci-hardware stack,
> and the initial ACPI pm hook before suspension starts):

We're missing that hook right now. ;-)

> 	PM infrastructure ... calling suspend() for everything
> 
> 	PCI bus support ... translates to PCI-specific typed call
> 
> 	PCI driver ... suspend() calling pci_choose_state()
> 
> 	ACPI support for PCI ... implementing choose_state()
> 
> 	ACPI core code ... remembering ACPI_STATE_Sx, calling AML
> 
> That is, ACPI gets invoked at various points, but the driver and
> core code doesn't need to know ACPI from Rumpelstiltskin.

I agree with that, but we need to add a mechanism to tell the ACPI core
what it needs to know (ie. the target system sleep state) before we suspend
devices or while we are suspending them.

Greetings,
Rafael


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