RE: suspend/resume support for driver requires an external firmware

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Oliver Neukum wrote:
> Am Freitag, 24. September 2004 08:16 schrieb Zhu, Yi:
>> Choice 3? or I missed something here?
> 
> If the user requests suspension, why can't he transfer the
> firmware before he does so? Thus the firmware would be in ram
> or part of the image read back from disk.

Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must
do something like cat /path/of/firmware > /proc/net/ipw2100/firmware?

Thanks,
-yi

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Montag, 27. September 2004 05:43 schrieb Zhu, Yi:
> Oliver Neukum wrote:
> > Am Freitag, 24. September 2004 08:16 schrieb Zhu, Yi:
> >> Choice 3? or I missed something here?
> > 
> > If the user requests suspension, why can't he transfer the
> > firmware before he does so? Thus the firmware would be in ram
> > or part of the image read back from disk.
> 
> Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must
> do something like cat /path/of/firmware > /proc/net/ipw2100/firmware?

Yes.

	Regards
		Oliver

RE: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Mon, 27 Sep 2004, Zhu, Yi wrote:

> Oliver Neukum wrote:
> > Am Freitag, 24. September 2004 08:16 schrieb Zhu, Yi:
> >> Choice 3? or I missed something here?
> >
> > If the user requests suspension, why can't he transfer the
> > firmware before he does so? Thus the firmware would be in ram
> > or part of the image read back from disk.
>
> Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must
> do something like cat /path/of/firmware > /proc/net/ipw2100/firmware?

Why not just suspend the device first, then enter the system suspend
state; then on resume, resume the device after control has transferred
back to userspace. That way, the driver can load the firmware from the
disk, and we don't have to worry about it in the kernel. Automate with a
script, and never worry about it again. :)


	Pat

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Montag, 27. September 2004 18:50 schrieb Patrick Mochel:
> 
> On Mon, 27 Sep 2004, Zhu, Yi wrote:
> 
> > Oliver Neukum wrote:
> > > Am Freitag, 24. September 2004 08:16 schrieb Zhu, Yi:
> > >> Choice 3? or I missed something here?
> > >
> > > If the user requests suspension, why can't he transfer the
> > > firmware before he does so? Thus the firmware would be in ram
> > > or part of the image read back from disk.
> >
> > Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must
> > do something like cat /path/of/firmware > /proc/net/ipw2100/firmware?
> 
> Why not just suspend the device first, then enter the system suspend
> state; then on resume, resume the device after control has transferred
> back to userspace. That way, the driver can load the firmware from the

And thus cause errors in all applications wishing to use the network
until the firmware is reloaded. It is precisely what cannot be done.
The firmware must be present on suspend. The question is, how?

	Regards
		Oliver

Re: suspend/resume support for driver requires an external firmware

[Dmitry Torokhov]

On Monday 27 September 2004 12:19 pm, Oliver Neukum wrote:
> > Why not just suspend the device first, then enter the system suspend
> > state; then on resume, resume the device after control has transferred
> > back to userspace. That way, the driver can load the firmware from the
> 
> And thus cause errors in all applications wishing to use the network
> until the firmware is reloaded. It is precisely what cannot be done.
> The firmware must be present on suspend. The question is, how?

While non-availability might be an issue for other types of hardware I think
it is ok for network cards. In many cases the interface will have to be
reconfigured at resume anyway (you move from office to home and the network
is completely different). Can't resume be handled by virtually removing/
inserting the device so firmware will be re-loaded as it was just a normal
startup?

-- 
Dmitry

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Montag, 27. September 2004 20:19 schrieb Dmitry Torokhov:
> On Monday 27 September 2004 12:19 pm, Oliver Neukum wrote:
> > > Why not just suspend the device first, then enter the system suspend
> > > state; then on resume, resume the device after control has transferred
> > > back to userspace. That way, the driver can load the firmware from the
> > 
> > And thus cause errors in all applications wishing to use the network
> > until the firmware is reloaded. It is precisely what cannot be done.
> > The firmware must be present on suspend. The question is, how?
> 
> While non-availability might be an issue for other types of hardware I think
> it is ok for network cards. In many cases the interface will have to be
> reconfigured at resume anyway (you move from office to home and the network
> is completely different). Can't resume be handled by virtually removing/
> inserting the device so firmware will be re-loaded as it was just a normal
> startup?

Can suspend/resume be handled as power off/power on?
Taking that to the logical conclusion, there is no suspend/resume.
You suspend in order to preserve the system state. The problem
of mobility is no excuse to improperly implement suspension.
You can't expect something to work suspended which wouldn't work
if the system were powered on.

	Regards
		Oliver

Re: suspend/resume support for driver requires an external firmware

[Denis Vlasenko]

On Monday 27 September 2004 21:19, Dmitry Torokhov wrote:
> On Monday 27 September 2004 12:19 pm, Oliver Neukum wrote:
> > > Why not just suspend the device first, then enter the system suspend
> > > state; then on resume, resume the device after control has transferred
> > > back to userspace. That way, the driver can load the firmware from the
> > 
> > And thus cause errors in all applications wishing to use the network
> > until the firmware is reloaded. It is precisely what cannot be done.
> > The firmware must be present on suspend. The question is, how?
> 
> While non-availability might be an issue for other types of hardware I think
> it is ok for network cards. In many cases the interface will have to be
> reconfigured at resume anyway (you move from office to home and the network
> is completely different). Can't resume be handled by virtually removing/
> inserting the device so firmware will be re-loaded as it was just a normal
> startup?

Think about situation when all filesystems are NFS-mounted.
You absolutely are not allowed to lose your network, or else hotplug
(and all fs-backed stuff in general) will die horribly.
--
vda

Re: suspend/resume support for driver requires an external firmware

[Dmitry Torokhov]

On Monday 27 September 2004 05:47 pm, Denis Vlasenko wrote:
> On Monday 27 September 2004 21:19, Dmitry Torokhov wrote:
> > On Monday 27 September 2004 12:19 pm, Oliver Neukum wrote:
> > > > Why not just suspend the device first, then enter the system suspend
> > > > state; then on resume, resume the device after control has transferred
> > > > back to userspace. That way, the driver can load the firmware from the
> > > 
> > > And thus cause errors in all applications wishing to use the network
> > > until the firmware is reloaded. It is precisely what cannot be done.
> > > The firmware must be present on suspend. The question is, how?
> > 
> > While non-availability might be an issue for other types of hardware I think
> > it is ok for network cards. In many cases the interface will have to be
> > reconfigured at resume anyway (you move from office to home and the network
> > is completely different). Can't resume be handled by virtually removing/
> > inserting the device so firmware will be re-loaded as it was just a normal
> > startup?
> 
> Think about situation when all filesystems are NFS-mounted.
> You absolutely are not allowed to lose your network, or else hotplug
> (and all fs-backed stuff in general) will die horribly.

Where do you load your firmware from so that you can bring up the network
so you can mount everything via NFS in the first place?

-- 
Dmitry

Re: suspend/resume support for driver requires an external firmware

[Denis Vlasenko]

On Tuesday 28 September 2004 02:06, Dmitry Torokhov wrote:
> On Monday 27 September 2004 05:47 pm, Denis Vlasenko wrote:
> > On Monday 27 September 2004 21:19, Dmitry Torokhov wrote:
> > > On Monday 27 September 2004 12:19 pm, Oliver Neukum wrote:
> > > > > Why not just suspend the device first, then enter the system suspend
> > > > > state; then on resume, resume the device after control has transferred
> > > > > back to userspace. That way, the driver can load the firmware from the
> > > > 
> > > > And thus cause errors in all applications wishing to use the network
> > > > until the firmware is reloaded. It is precisely what cannot be done.
> > > > The firmware must be present on suspend. The question is, how?
> > > 
> > > While non-availability might be an issue for other types of hardware I think
> > > it is ok for network cards. In many cases the interface will have to be
> > > reconfigured at resume anyway (you move from office to home and the network
> > > is completely different). Can't resume be handled by virtually removing/
> > > inserting the device so firmware will be re-loaded as it was just a normal
> > > startup?
> > 
> > Think about situation when all filesystems are NFS-mounted.
> > You absolutely are not allowed to lose your network, or else hotplug
> > (and all fs-backed stuff in general) will die horribly.
> 
> Where do you load your firmware from so that you can bring up the network
> so you can mount everything via NFS in the first place?

>From initrd. I am booting off small DOS partition which holds Linux images,
initrds, loader etc. After boot, I do not mount any local partitions.
--
vda

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Patrick Mochel wrote:
> On Tue, 28 Sep 2004, Zhu, Yi wrote:
> 
>> Do you still think the ->save_state, ->restore_state are the right
>> approach
> 
> Yes.

Fine. That's the interface I mentioned last time. Sorry if it's
confusing.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Patrick Mochel wrote:
> On Tue, 28 Sep 2004, Zhu, Yi wrote:
>> Agreed. I think now we need a clean interface that makes drivers,
>> swsusp or even the end user to work together to finally achieve the
>> goal.
> 
> Well, if you can read firmware back from the device, then
> that interface is called kmalloc() to allocate the buffer,
> and whatever driver-specific routine to copy it from the
> device into that buffer. It then stays in memory during
> suspend and can be rewritten to the device once resumed.
> 
> What's so bad about that?

Hi Patrick,

Do you still think the ->save_state, ->restore_state are the right
approach
or you want the user to provide the firmware to driver before suspend?

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Tue, 28 Sep 2004, Zhu, Yi wrote:

> Do you still think the ->save_state, ->restore_state are the right
> approach

Yes.


	Pat

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Patrick Mochel wrote:
> I presume you're not talking about doing swsusp over NFS. If
> so, there's a lot more work to be done to teach the driver
> model and power management infrastructure about the
> dependencies involved to make that a possibility.
> It's safe to say that we don't support that, and won't
> support that at least for some time.

Then let's talk about S3 (suspend to ram), I think it should be
OK with a mounted NFS root, but the firmware issue is still there.

> As far as the firmware goes, there are two choices - reload
> it from userspace once we return or save it memory during
> suspend. I assume that these devices provide some means for
> reading the firmware from them, so you can just allocate a
> buffer and read it into that during the transition.

Agreed. I think now we need a clean interface that makes
drivers, swsusp or even the end user to work together to
finally achieve the goal.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Tue, 28 Sep 2004, Zhu, Yi wrote:

> Patrick Mochel wrote:

> > As far as the firmware goes, there are two choices - reload
> > it from userspace once we return or save it memory during
> > suspend. I assume that these devices provide some means for
> > reading the firmware from them, so you can just allocate a
> > buffer and read it into that during the transition.
>
> Agreed. I think now we need a clean interface that makes
> drivers, swsusp or even the end user to work together to
> finally achieve the goal.

Well, if you can read firmware back from the device, then that interface
is called kmalloc() to allocate the buffer, and whatever driver-specific
routine to copy it from the device into that buffer. It then stays in
memory during suspend and can be rewritten to the device once resumed.

What's so bad about that?


	Pat

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Dmitry Torokhov wrote:
> Where do you load your firmware from so that you can bring up
> the network so you can mount everything via NFS in the first place?

The firmware locates together w/ the driver in the initrd which could be
either in the remote PXE server or the local diskettes. It should be
also
placed somewhere on the NFS root so that it can be picked up to
memory during suspend.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Tue, 28 Sep 2004, Zhu, Yi wrote:

> Dmitry Torokhov wrote:
> > Where do you load your firmware from so that you can bring up
> > the network so you can mount everything via NFS in the first place?
>
> The firmware locates together w/ the driver in the initrd which could be
> either in the remote PXE server or the local diskettes. It should be
> also
> placed somewhere on the NFS root so that it can be picked up to
> memory during suspend.

I presume you're not talking about doing swsusp over NFS. If so, there's
a lot more work to be done to teach the driver model and power management
infrastructure about the dependencies involved to make that a possibility.
It's safe to say that we don't support that, and won't support that at
least for some time.

As far as the firmware goes, there are two choices - reload it from
userspace once we return or save it memory during suspend. I assume that
these devices provide some means for reading the firmware from them, so
you can just allocate a buffer and read it into that during the
transition.


	Pat

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Patrick Mochel wrote:
> Why not just suspend the device first, then enter the system
> suspend state; then on resume, resume the device after
> control has transferred back to userspace. That way, the
> driver can load the firmware from the disk, and we don't have
> to worry about it in the kernel. Automate with a script, and
> never worry about it again. :)

This won't work for the mount NFS root model proposed by
Denis Vlasenko on this topic.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Oliver Neukum wrote:
>> Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must do
>> something like cat /path/of/firmware > /proc/net/ipw2100/firmware?
> 
> Yes.

I prefer it could be transparent to users.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Tue, 28 Sep 2004, Zhu, Yi wrote:

> Oliver Neukum wrote:
> >> Do you suggest before user echo 4 > /proc/acpi/sleep, [s]he must do
> >> something like cat /path/of/firmware > /proc/net/ipw2100/firmware?
> >
> > Yes.
>
> I prefer it could be transparent to users.

Then put it in a script. There are other things that need to be done,
based on user policy, during suspend and resume transitions. See the
suspend scripts that Nigel maintains for swsusp2 for examples of this. In
an ideal world (i.e. the future), users will not be echo'ing values into a
proc or sysfs file; they will be running a 'meta-script' or clicking a
button on their desktop or just closing the lid to their laptop to
suspend.

To that end, we need something like an /etc/power/ directory with scripts
that a suspend script or program runs on power state transitions. That
way policy and device-specific items can easily be dropped in there, and
transparency can be achieved that way.


	Pat

RE: suspend/resume support for driver requires an external firmware

[Li, Shaohua]

>> I somewhat think choice 2 is better. The pre-load can be invoked in
any
>> order instead of in the device tree hierarchy order. And currently
only
>> few devices need it, is it worth adding it in the device core?
>
>If order doesn't matter, the device tree order is as good as any
>other order.
>
>> In addition, the notifiers have better flexibility. We can easily add
>> more notifier types if needed, such as adding a callback between the
>> sysdev resume and regular devices resume. To tell the truth, we
actually
>> have the case. An ACPI link device must resume before regular devices
>> but must be with IRQ enabled. I'm considering add a call back between
>> sysdev and regular devices resume for the issue. I guess the MTRR
driver
>> in SMP has the same requirement. Anyway, notifier solution sounds
like
>> easier to extend, but we can't extend device core casually.
>
>If you add this stuff to anything but the device core, you have to
teach
>the drivers about the various notifier chains. Why would adding methods
>to the device core be harder than adding notifier chains? If drivers do
>not need the method they don't need to implement it. If they do need
>it, using a notifier chain isn't easier.
Adding methods to the device core requires changes the device core every
time when you add a new call back. The notifier chain method can keep
the device core stable.
Considering another case, we might want to add some call backs between
sysdevs resume and regular devices resume (the case is the above). It
might not be for a device. How can the device core call back do this? 

Thanks,
Shaohua

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Montag, 27. September 2004 08:23 schrieb Li, Shaohua:
> Adding methods to the device core requires changes the device core every
> time when you add a new call back. The notifier chain method can keep
> the device core stable.

No. It pretends that the interface is stable. If additional callbacks
are needed and provided externally, driver core is unable to do
what it is intended to.

> Considering another case, we might want to add some call backs between
> sysdevs resume and regular devices resume (the case is the above). It
> might not be for a device. How can the device core call back do this? 

The same way it resumes system devices.

	Regards
		Oliver

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Marcel Holtmann wrote:
> it consumes extra runtime memory, but why not build a simple
> firmware cache behind the request_firmware() interface. 

I think this could be another choice, but I'd rather call it as a
firmware 
suspend hook. Because when the system is running, the cache 
should be purged to save memory. swsusp (or other implementations)
is required to call the firmware hook before device_suspend. The 
firmware hook is responsible to load all the necessary firmwares into
memory before suspend so that they can be read from memory in the
next request_firmware(). And we also need a firmware resume hook to
free the memory.

But how about drivers read firmware themselves? They don't rely on
firmware_class.

> In this case it can be transparent for the driver and we won't
> have an extra workaround for suspend/resume stuff for every
> driver.

I don't think it is a workaround, drivers are supposed to know best
how to "save their states". Firmware is one example for now.

> How many firmwares do a normal system really have to
> hold in memory for suspend/resume actions?

I don't know either. ;-)

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Zhu, Yi]

Patrick Mochel wrote:
> We talked about this in Ottawa a few months ago, and I think
> this is the right approach. Note though, that I think it
> needs to be more complete:
> 
> - There needs to be restore_state() to be symmetic.
> - There needs to be the proper failure recovery
>   If save_state() or suspend() fails, every device that has had their
>   state saved needs to be restored.
> - It needs to be called for all power management requests.

Agreed.

> - The PCI implementation should call pci_save_state() in it,
> instead of in ->suspend().

Also agree. And split pci_restore_state() from ->resume to
->restore_state. But all current drivers need to be rewritten.

Thanks,
-yi

RE: suspend/resume support for driver requires an external firmware

[Li, Shaohua]

>> We talked about this in Ottawa a few months ago, and I think this is
the
>> right approach. Note though, that I think it needs to be more
complete:
>>
>> - There needs to be restore_state() to be symmetic.
>> - There needs to be the proper failure recovery
>>   If save_state() or suspend() fails, every device that has had their
>>   state saved needs to be restored.
>> - It needs to be called for all power management requests.
>> - The PCI implementation should call pci_save_state() in it, instead
of
>in
>>   ->suspend().
>>
>> It would be great if you could add these things. Otherwise, I'll add
it
>to
>> my TODO list..
>
>Additionally, for devices like the above that need either to rely on
>userland for firmware download or to allocate large amounts of memory
>for firmware backup/restore, I think we need to revive the pre-suspend
>and post-resume notifiers ... Of course, if a device that needs
userland
>to reload a firmware is on the swap patch, then we have a chicken & egg
>problem, but there is no easy solution for that one, unless the driver
>uses the pre-suspend callback to pre-load the firmware that it will
need
>for resume
I somewhat think choice 2 is better. The pre-load can be invoked in any
order instead of in the device tree hierarchy order. And currently only
few devices need it, is it worth adding it in the device core?
In addition, the notifiers have better flexibility. We can easily add
more notifier types if needed, such as adding a callback between the
sysdev resume and regular devices resume. To tell the truth, we actually
have the case. An ACPI link device must resume before regular devices
but must be with IRQ enabled. I'm considering add a call back between
sysdev and regular devices resume for the issue. I guess the MTRR driver
in SMP has the same requirement. Anyway, notifier solution sounds like
easier to extend, but we can't extend device core casually.

Thanks,
Shaohua

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Freitag, 24. September 2004 13:52 schrieb Li, Shaohua:
> I somewhat think choice 2 is better. The pre-load can be invoked in any
> order instead of in the device tree hierarchy order. And currently only
> few devices need it, is it worth adding it in the device core?

If order doesn't matter, the device tree order is as good as any
other order.

> In addition, the notifiers have better flexibility. We can easily add
> more notifier types if needed, such as adding a callback between the
> sysdev resume and regular devices resume. To tell the truth, we actually
> have the case. An ACPI link device must resume before regular devices
> but must be with IRQ enabled. I'm considering add a call back between
> sysdev and regular devices resume for the issue. I guess the MTRR driver
> in SMP has the same requirement. Anyway, notifier solution sounds like
> easier to extend, but we can't extend device core casually.

If you add this stuff to anything but the device core, you have to teach
the drivers about the various notifier chains. Why would adding methods
to the device core be harder than adding notifier chains? If drivers do
not need the method they don't need to implement it. If they do need
it, using a notifier chain isn't easier.

	Regards
		Oliver

suspend/resume support for driver requires an external firmware

[Zhu, Yi]

[Mail originally post on acpi-dev, repost here per Patrick's request.]


Hi,

I make the ipw2100 driver (http://ipw2100.sf.net) support suspend/resume
with below patch.
http://cache.gmane.org//gmane/linux/drivers/ipw2100/devel/2129-001.bin

Current state is the patch makes ipw2100 suspend/resume work in the expense
of addtional ~200k kernel runtime memory. So I'd rather look on it as a
workaround instead of a finial solution.

The problem is ipw2100 requires an external on-disk firmware support. When
the device is put to D3 state, firmware needs to be unloaded. And the
firmware should be loaded again after the device is put to D0 state. But
currently there is no suspend/resume order(dependency) for the online
devices list in the kernel (dpm_active). So the ide disk might still be in
the suspend state when ipw2100->resume is called. This causes a deadlock.
Note here the ipw2100 can be any driver that requires a firmware.


I propose 2 choices to fix the problem.

Choice 1. In 2.5 kernel, there used to be a ->save_state method in the
device PM interface. From the "not yet updated" document
(Documentation/power/pci.txt), this function can be used as "a notification
that it(the device) may be entering a sleep state in the near future". If we
take back this interface, the problem can be solved. That is, the driver
loads firmware into memory in ->save_state and frees the memory in ->resume.
The deadlock is resolved without any runtime memory wasted.

patch embeded at the end of the mail.


Choice 2. Add a notifier call chain for swsusp before prepare_suspend (and
probably for acpi_suspend as well). Each driver requires the notification
(i.e. ipw2100) registers its own callback. For people who think device PM
->save_state is an overkill since 99% devices don't require firmwares
(really today, who knows the future?), this can be considered as another
(better) solution.


Choice 3? or I missed something here?


Your comments are highly appreciated!

-- 
-----------------------------------------------------------------
Opinions expressed are those of the author and do not represent
Intel Corp.

Zhu Yi (Chuyee)

GnuPG v1.0.6 (GNU/Linux)
http://cn.geocities.com/chewie_chuyee/gpg.txt or
$ gpg --keyserver wwwkeys.pgp.net --recv-keys 71C34820
1024D/71C34820 C939 2B0B FBCE 1D51 109A  55E5 8650 DB90 71C3 4820



Signed-off-by: Zhu Yi <yi.zhu@intel.com>

 drivers/base/power/suspend.c |   18 ++++++++++++++++++
 drivers/pci/pci-driver.c     |   13 +++++++++++++
 include/linux/device.h       |    1 +
 include/linux/pci.h          |    1 +
 include/linux/pm.h           |    1 +
 kernel/power/swsusp.c        |    5 +++++
 6 files changed, 39 insertions(+)


diff -urp linux-2.6.8.1-vanilla/drivers/pci/pci-driver.c linux-2.6.8.1-swsusp/drivers/pci/pci-driver.c
--- linux-2.6.8.1-vanilla/drivers/pci/pci-driver.c	2004-09-18 20:38:57.000000000 +0800
+++ linux-2.6.8.1-swsusp/drivers/pci/pci-driver.c	2004-09-18 15:44:24.000000000 +0800
@@ -295,6 +295,18 @@ static int pci_device_remove(struct devi
 	return 0;
 }
 
+static int pci_device_save_state(struct device * dev, u32 state)
+{
+	struct pci_dev * pci_dev = to_pci_dev(dev);
+	struct pci_driver * drv = pci_dev->driver;
+	int i = 0;
+
+	if (drv && drv->save_state)
+		i = drv->save_state(pci_dev,state);
+		
+	return i;
+}
+
 static int pci_device_suspend(struct device * dev, u32 state)
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
@@ -537,6 +549,7 @@ struct bus_type pci_bus_type = {
 	.name		= "pci",
 	.match		= pci_bus_match,
 	.hotplug	= pci_hotplug,
+	.save_state	= pci_device_save_state,
 	.suspend	= pci_device_suspend,
 	.resume		= pci_device_resume,
 	.dev_attrs	= pci_dev_attrs,
diff -urp linux-2.6.8.1-vanilla/include/linux/device.h linux-2.6.8.1-swsusp/include/linux/device.h
--- linux-2.6.8.1-vanilla/include/linux/device.h	2004-09-18 20:40:32.000000000 +0800
+++ linux-2.6.8.1-swsusp/include/linux/device.h	2004-09-18 13:59:39.000000000 +0800
@@ -62,6 +62,7 @@ struct bus_type {
 	struct device * (*add)	(struct device * parent, char * bus_id);
 	int		(*hotplug) (struct device *dev, char **envp, 
 				    int num_envp, char *buffer, int buffer_size);
+	int		(*save_state)(struct device * dev, u32 state);
 	int		(*suspend)(struct device * dev, u32 state);
 	int		(*resume)(struct device * dev);
 };
diff -urp linux-2.6.8.1-vanilla/include/linux/pci.h linux-2.6.8.1-swsusp/include/linux/pci.h
--- linux-2.6.8.1-vanilla/include/linux/pci.h	2004-09-18 20:36:44.000000000 +0800
+++ linux-2.6.8.1-swsusp/include/linux/pci.h	2004-09-18 14:47:31.000000000 +0800
@@ -637,6 +637,7 @@ struct pci_driver {
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
+	int  (*save_state) (struct pci_dev *dev, u32 state);	/* Device save state */
 	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	int  (*enable_wake) (struct pci_dev *dev, u32 state, int enable);   /* Enable wake event */
diff -urp linux-2.6.8.1-vanilla/include/linux/pm.h linux-2.6.8.1-swsusp/include/linux/pm.h
--- linux-2.6.8.1-vanilla/include/linux/pm.h	2004-09-18 20:37:57.000000000 +0800
+++ linux-2.6.8.1-swsusp/include/linux/pm.h	2004-09-18 16:58:08.000000000 +0800
@@ -241,6 +241,7 @@ struct dev_pm_info {
 
 extern void device_pm_set_parent(struct device * dev, struct device * parent);
 
+extern int device_save_state(u32 state);
 extern int device_suspend(u32 state);
 extern int device_power_down(u32 state);
 extern void device_power_up(void);
diff -urp linux-2.6.8.1-vanilla/drivers/base/power/suspend.c linux-2.6.8.1-swsusp/drivers/base/power/suspend.c
--- linux-2.6.8.1-vanilla/drivers/base/power/suspend.c	2004-09-18 20:40:14.000000000 +0800
+++ linux-2.6.8.1-swsusp/drivers/base/power/suspend.c	2004-09-18 17:35:43.000000000 +0800
@@ -49,6 +49,24 @@ int suspend_device(struct device * dev, 
 	return error;
 }
 
+/**
+ *	device_save_state - Save state all devices in system.
+ *	@state:		    Power state to put each device in.
+ *
+ *	Walk the dpm_active list, call ->save_state() for each device.
+ */
+int device_save_state(u32 state)
+{
+	struct device * dev;
+	int error = 0;
+
+	list_for_each_entry(dev, &dpm_active, power.entry) {
+		if (dev->bus && dev->bus->save_state)
+			error = dev->bus->save_state(dev, state);
+	}
+
+	return error;
+}
 
 /**
  *	device_suspend - Save state and stop all devices in system.
diff -urp linux-2.6.8.1-vanilla/kernel/power/swsusp.c linux-2.6.8.1-swsusp/kernel/power/swsusp.c
--- linux-2.6.8.1-vanilla/kernel/power/swsusp.c	2004-09-18 20:35:07.000000000 +0800
+++ linux-2.6.8.1-swsusp/kernel/power/swsusp.c	2004-09-18 20:55:07.000000000 +0800
@@ -835,6 +835,11 @@ int software_suspend(void)
 	}		
 	if (pm_prepare_console())
 		printk( "%sCan't allocate a console... proceeding\n", name_suspend);
+
+	/* device save_state */
+	if (device_save_state(3))
+		return -EBUSY;
+
 	if (!prepare_suspend_processes()) {
 
 		/* At this point, all user processes and "dangerous"

Re: suspend/resume support for driver requires an external firmware

[Patrick Mochel]

On Fri, 24 Sep 2004, Zhu, Yi wrote:

> Choice 1. In 2.5 kernel, there used to be a ->save_state method in the
> device PM interface. From the "not yet updated" document
> (Documentation/power/pci.txt), this function can be used as "a notification
> that it(the device) may be entering a sleep state in the near future". If we
> take back this interface, the problem can be solved. That is, the driver
> loads firmware into memory in ->save_state and frees the memory in ->resume.
> The deadlock is resolved without any runtime memory wasted.
>
> patch embeded at the end of the mail.

We talked about this in Ottawa a few months ago, and I think this is the
right approach. Note though, that I think it needs to be more complete:

- There needs to be restore_state() to be symmetic.
- There needs to be the proper failure recovery
  If save_state() or suspend() fails, every device that has had their
  state saved needs to be restored.
- It needs to be called for all power management requests.
- The PCI implementation should call pci_save_state() in it, instead of in
  ->suspend().

It would be great if you could add these things. Otherwise, I'll add it to
my TODO list..

Thanks,


	Pat

Re: suspend/resume support for driver requires an external firmware

[Benjamin Herrenschmidt]

> We talked about this in Ottawa a few months ago, and I think this is the
> right approach. Note though, that I think it needs to be more complete:
> 
> - There needs to be restore_state() to be symmetic.
> - There needs to be the proper failure recovery
>   If save_state() or suspend() fails, every device that has had their
>   state saved needs to be restored.
> - It needs to be called for all power management requests.
> - The PCI implementation should call pci_save_state() in it, instead of in
>   ->suspend().
> 
> It would be great if you could add these things. Otherwise, I'll add it to
> my TODO list..

Additionally, for devices like the above that need either to rely on
userland for firmware download or to allocate large amounts of memory
for firmware backup/restore, I think we need to revive the pre-suspend
and post-resume notifiers ... Of course, if a device that needs userland
to reload a firmware is on the swap patch, then we have a chicken & egg
problem, but there is no easy solution for that one, unless the driver
uses the pre-suspend callback to pre-load the firmware that it will need
for resume

Ben.

Re: suspend/resume support for driver requires an external firmware

[Oliver Neukum]

Am Freitag, 24. September 2004 08:16 schrieb Zhu, Yi:
> Choice 3? or I missed something here?

If the user requests suspension, why can't he transfer the firmware
before he does so? Thus the firmware would be in ram or part of
the image read back from disk.

	Regards
		Oliver

Re: suspend/resume support for driver requires an external firmware

[Marcel Holtmann]

Hi,

> Current state is the patch makes ipw2100 suspend/resume work in the expense
> of addtional ~200k kernel runtime memory. So I'd rather look on it as a
> workaround instead of a finial solution.

it consumes extra runtime memory, but why not build a simple firmware
cache behind the request_firmware() interface. In this case it can be
transparent for the driver and we won't have an extra workaround for
suspend/resume stuff for every driver. How many firmwares do a normal
system really have to hold in memory for suspend/resume actions?

Regards

Marcel