headers
Nigel Cunningham | 1 Apr 2008 10:15
Picon
Favicon

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

Hi Rafael.

Please excuse me, but I'm going to ask the questions you get from
someone who hasn't followed development to date, and is thus reading the
explanation without prior knowledge. Hopefully that will be helpful when
you come to finalising the commit message.

On Mon, 2008-03-31 at 23:29 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw <at> sisk.pl>
> 
> Introduce 'struct pm_ops' and 'struct pm_ext_ops' representing
> suspend and hibernation operations for bus types, device classes and
> device types.

Does ..._ext_... mean extended? (external?) If 'extended' (or if not),
does that imply that they're mutually exclusive alternatives for drivers
to use?

> Modify the PM core to use 'struct pm_ops' and 'struct pm_ext_ops'
> objects, if defined, instead of the ->suspend() and ->resume() or,
> respectively, ->suspend_late() and ->resume_early() callbacks that
> will be considered as legacy and gradually phased out.

'Respectively' doesn't look like the right word to use, but I'm not sure
I understand correctly what you're trying to say. The way it's written
at the moment, it sounds to me like you're saying that suspend_late()
and resume_early are deprecated, but you're modifying the PM core to use
them.

> Change the behavior of the PM core wrt the error codes returned by
(Continue reading)

Permalink | Reply | 0 comments |
headers
Benjamin Herrenschmidt | 1 Apr 2008 10:27

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)


On Tue, 2008-04-01 at 19:15 +1100, Nigel Cunningham wrote:
> > + *   However, drivers may NOT assume anything about the availability of the
> > + *   user space at that time and it is not correct to request firmware from
> > + *   within  <at> prepare() (it's too late to do that).
> 
> That doesn't sound good. It would be good to be able to get drivers to
> request firmware early in the process.

Agreed. Prepare() should still allow request_firmware and full userspace
communication / helper usage.

> > + *  <at> complete: Undo the changes made by  <at> prepare().  This method is executed for
> > + *   all kinds of resume transitions, following one of the resume callbacks:
> > + *    <at> resume(),  <at> thaw(),  <at> restore().  Also called if the state transition
> > + *   fails before the driver's suspend callback ( <at> suspend(),  <at> freeze(),
> > + *    <at> poweroff()) can be executed (e.g. if the suspend callback fails for one
> > + *   of the other devices that the PM core has unsucessfully attempted to

Ben.
Permalink | Reply | 0 comments |
headers
Pavel Machek | 1 Apr 2008 10:37
Picon

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

> On Sunday, 30 of March 2008, Rafael J. Wysocki wrote:
> > On Saturday, 29 of March 2008, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw <at> sisk.pl>
> > > 
> > > Introduce 'struct pm_ops' and 'struct pm_ext_ops' representing
> > > suspend and hibernation operations for bus types, device classes and
> > > device types.
> > > 
> > > Modify the PM core to use 'struct pm_ops' and 'struct pm_ext_ops'
> > > objects, if defined, instead of the ->suspend() and ->resume() or,
> > > respectively, ->suspend_late() and ->resume_early() callbacks that
> > > will be considered as legacy and gradually phased out.
> > 
> > Unfortunately I forgot to set dev->power.status to DPM_PREPARING before
> > calling ->prepare(), as documented.  Also, dpm_prepare() could cleaned up
> > a bit.
> > 
> > Fixed patch follows.
> 
> My testing shows that some drivers tend to return error codes from their
> ->resume() callbacks, even though the devices in question appear to work
> correctly after the seemingly failing suspend.

The drivers should be fixed not to do that, no?

--

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Permalink | Reply | 0 comments |
headers
Pavel Machek | 1 Apr 2008 10:45
Picon

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

Hi!

> > > > > For the reasons outlined above, the change of the suspend ordering
> > > > > should be reverted, which is done by the patch below.
> > > > 
> > > > But this will break those few nvidia-based systems, no?
> > > > 
> > > > this may have been a good idea in -rc1 days, but we are in -rc7
> > > > now... and the patch is slightly big.
> > > 
> > > It's quite obvious, though.
> > 
> > Yes, but breaking systems between -rc7 and final is _very_ unnice.
> 
> Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
> I've posted this patch.
> 
> IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
> because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
> the "fix" and go back to the design board.  That's all we can do so late in
> the release cycle, IMO.

Well, I agree that regression from 2.6.24 is worse, but it is
_slightly_ worse... -rcs are really expected to improve...

...plus it no longer looks like macbook regression is caused by _PTS
ordering?

> > > I think we _can_ do something about the failing NVidia systems in the 2.6.26
> > > time frame, but that will require some more consideration.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alan Stern | 1 Apr 2008 16:31
Picon
Favicon

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

On Tue, 1 Apr 2008, Benjamin Herrenschmidt wrote:

> On Tue, 2008-04-01 at 19:15 +1100, Nigel Cunningham wrote:
> > > + *   However, drivers may NOT assume anything about the availability of the
> > > + *   user space at that time and it is not correct to request firmware from
> > > + *   within  <at> prepare() (it's too late to do that).
> > 
> > That doesn't sound good. It would be good to be able to get drivers to
> > request firmware early in the process.
> 
> Agreed. Prepare() should still allow request_firmware and full userspace
> communication / helper usage.

Pepare() is called after userspace has been frozen.  (Of course, once 
the freezer goes away this won't matter any more.)

There is a separate notifier chain which drivers can subscribe to;
notifications about impending sleeps are sent out while userspace is
still alive.  Drivers can use that for request_firmware, memory
allocation, and other things.

Alan Stern
Permalink | Reply | 0 comments |
headers
Felix Möller | 1 Apr 2008 16:38
Picon

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

Hi,
>>>>>> For the reasons outlined above, the change of the suspend ordering
>>>>>> should be reverted, which is done by the patch below.
>>>>> But this will break those few nvidia-based systems, no?
>>>>>
>>>>> this may have been a good idea in -rc1 days, but we are in -rc7
>>>>> now... and the patch is slightly big.
>>>> It's quite obvious, though.
>>> Yes, but breaking systems between -rc7 and final is _very_ unnice.
>> Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
>> I've posted this patch.
>>
>> IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
>> because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
>> the "fix" and go back to the design board.  That's all we can do so late in
>> the release cycle, IMO.
> 
> Well, I agree that regression from 2.6.24 is worse, but it is
> _slightly_ worse... -rcs are really expected to improve...
> 
> ...plus it no longer looks like macbook regression is caused by _PTS
> ordering?
I am the reporter from the original Novell Bug: 
https://bugzilla.novell.com/show_bug.cgi?id=374217

I just tried current git head (two hours ago) with the patch (the one 
from the beginning of this thread) from Rafael and without it. With the 
patch my MacBook does suspend without it does not.

HTH
(Continue reading)

Permalink | Reply | 0 comments |
headers
Nigel Cunningham | 1 Apr 2008 21:34
Picon
Favicon

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

Hi.

On Tue, 2008-04-01 at 10:31 -0400, Alan Stern wrote:
> On Tue, 1 Apr 2008, Benjamin Herrenschmidt wrote:
> 
> > On Tue, 2008-04-01 at 19:15 +1100, Nigel Cunningham wrote:
> > > > + *   However, drivers may NOT assume anything about the availability of the
> > > > + *   user space at that time and it is not correct to request firmware from
> > > > + *   within  <at> prepare() (it's too late to do that).
> > > 
> > > That doesn't sound good. It would be good to be able to get drivers to
> > > request firmware early in the process.
> > 
> > Agreed. Prepare() should still allow request_firmware and full userspace
> > communication / helper usage.
> 
> Pepare() is called after userspace has been frozen.  (Of course, once 
> the freezer goes away this won't matter any more.)
> 
> There is a separate notifier chain which drivers can subscribe to;
> notifications about impending sleeps are sent out while userspace is
> still alive.  Drivers can use that for request_firmware, memory
> allocation, and other things.

Then that should be mentioned here so that drivers authors can know that
it is possible to request firmware, allocate memory and so on at a stage
when you can still rely on userspace being alive.

Regards,
(Continue reading)

Permalink | Reply | 0 comments |
headers
Rafael J. Wysocki | 1 Apr 2008 21:55
Picon
Gravatar

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

On Tuesday, 1 of April 2008, Pavel Machek wrote:
> Hi!

Hi,

> > > > > > For the reasons outlined above, the change of the suspend ordering
> > > > > > should be reverted, which is done by the patch below.
> > > > > 
> > > > > But this will break those few nvidia-based systems, no?
> > > > > 
> > > > > this may have been a good idea in -rc1 days, but we are in -rc7
> > > > > now... and the patch is slightly big.
> > > > 
> > > > It's quite obvious, though.
> > > 
> > > Yes, but breaking systems between -rc7 and final is _very_ unnice.
> > 
> > Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
> > I've posted this patch.
> > 
> > IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
> > because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
> > the "fix" and go back to the design board.  That's all we can do so late in
> > the release cycle, IMO.
> 
> Well, I agree that regression from 2.6.24 is worse, but it is
> _slightly_ worse... -rcs are really expected to improve...
> 
> ...plus it no longer looks like macbook regression is caused by _PTS
> ordering?
(Continue reading)

Permalink | Reply | 0 comments |
headers
Rafael J. Wysocki | 1 Apr 2008 22:12
Picon
Gravatar

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

On Tuesday, 1 of April 2008, Nigel Cunningham wrote:
> Hi Rafael.

Hi,

> Please excuse me, but I'm going to ask the questions you get from
> someone who hasn't followed development to date, and is thus reading the
> explanation without prior knowledge. Hopefully that will be helpful when
> you come to finalising the commit message.
> 
> On Mon, 2008-03-31 at 23:29 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw <at> sisk.pl>
> > 
> > Introduce 'struct pm_ops' and 'struct pm_ext_ops' representing
> > suspend and hibernation operations for bus types, device classes and
> > device types.
> 
> Does ..._ext_... mean extended? (external?) If 'extended' (or if not),
> does that imply that they're mutually exclusive alternatives for drivers
> to use?

'ext' means 'extended'.  The idea is that the 'extended' version will be used
by bus types / driver types that don't need to implement the _noirq callbacks.
Both the platform and PCI bus types generally allow drivers to use _noirq
callbacks, so they use 'struct pm_ext_ops', as well as their corresponding
driver types.

> > Modify the PM core to use 'struct pm_ops' and 'struct pm_ext_ops'
> > objects, if defined, instead of the ->suspend() and ->resume() or,
> > respectively, ->suspend_late() and ->resume_early() callbacks that
(Continue reading)

Permalink | Reply | 0 comments |
headers
Rafael J. Wysocki | 1 Apr 2008 22:16
Picon
Gravatar

Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 6)

On Tuesday, 1 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Tue, 2008-04-01 at 19:15 +1100, Nigel Cunningham wrote:
> > > + *   However, drivers may NOT assume anything about the availability of the
> > > + *   user space at that time and it is not correct to request firmware from
> > > + *   within  <at> prepare() (it's too late to do that).
> > 
> > That doesn't sound good. It would be good to be able to get drivers to
> > request firmware early in the process.
> 
> Agreed. Prepare() should still allow request_firmware and full userspace
> communication / helper usage.

This documents the current state which is that we have the freezer and we have
drivers that rely on it.

I can remove the comment, but then someone will invent ->prepare() relying
on the availability of the user space at this point and that won't work.

Thanks,
Rafael
Permalink | Reply | 0 comments |

Gmane