suspend/resume support for driver requires an external firmware

suspend/resume support for driver requires an external firmware

[Zhu, Yi]

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.


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.

3 patches are attached for this idea.
device-save_state.patch adds back the interface ->save_state for the devices
	and implements pci bus save_state method.
swsusp-save_state.patch makes swsusp walks all devices ->save_state method
	before calling ->suspend method.
ipw2100-save_state.patch implements ipw2100 ->save_state method. 
They are proved working.


Choice 2. Add a notifier call chain for swsusp before prepare_suspend. 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



-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Zhu, Yi]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 37 bytes --]

Thu, 23 Sep 2004 15:46:11 +0800 (CST)

[-- Attachment #2: device-save_state --]
[-- Type: TEXT/PLAIN, Size: 3122 bytes --]

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);

[-- Attachment #3: swsusp-save_state --]
[-- Type: TEXT/PLAIN, Size: 1514 bytes --]

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"

[-- Attachment #4: ipw2100-save_state --]
[-- Type: TEXT/PLAIN, Size: 4520 bytes --]

diff -urp ipw2100-0.54/ipw2100_main.c ipw2100-0.54-swsusp/ipw2100_main.c
--- ipw2100-0.54/ipw2100_main.c	2004-09-15 13:11:28.000000000 +0800
+++ ipw2100-0.54-swsusp/ipw2100_main.c	2004-09-19 20:13:27.000000000 +0800
@@ -233,6 +233,8 @@ static int mode = 0;
 static int channel = 1;
 static int associate = 1;
 static int disable = 0;
+static struct ipw2100_fw ipw2100_firmware;
+static int keep_firmware = 0;
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
 
@@ -750,8 +752,6 @@ static int ipw2100_download_firmware(str
 	int err;
 
 	/* Fetch the firmware and microcode */
-	struct ipw2100_fw fw;
-	
 	if (priv->fatal_error) {
 		printk(KERN_ERR "%s: ipw2100_download_firmware called after "
 		       "fatal error %d.  Interface must be brought down.\n",
@@ -759,14 +759,14 @@ static int ipw2100_download_firmware(str
 		return -EINVAL;
 	}
 
-	err = ipw2100_get_firmware(priv, &fw);
+	err = ipw2100_get_firmware(priv, &ipw2100_firmware);
 	if (err) {
 		printk(KERN_ERR "%s: ipw2100_get_firmware failed: %d\n",
 		       priv->ndev->name, err);
 		priv->fatal_error = IPW2100_ERR_FW_LOAD;
 		goto fail;
 	}
-	priv->firmware_version = fw.version;
+	priv->firmware_version = ipw2100_firmware.version;
 
 	/* s/w reset and clock stabilization */
 	err = sw_reset_and_clock(priv);
@@ -792,7 +792,7 @@ static int ipw2100_download_firmware(str
 	write_register(priv->ndev, IPW_REG_RESET_REG, 0);
 
 	/* load microcode */
-	err = ipw2100_ucode_download(priv, &fw);
+	err = ipw2100_ucode_download(priv, &ipw2100_firmware);
 	if (err) {
 		printk(KERN_ERR "%s: Error loading microcode: %d\n",
 		       priv->ndev->name, err);
@@ -813,7 +813,7 @@ static int ipw2100_download_firmware(str
 	}
 
 	/* load f/w */
-	err = ipw2100_fw_download(priv, &fw);
+	err = ipw2100_fw_download(priv, &ipw2100_firmware);
 	if (err) {
 		printk(KERN_ERR "%s: Error loading firmware: %d\n",
 		       priv->ndev->name, err);
@@ -821,7 +821,8 @@ static int ipw2100_download_firmware(str
 	}
 	
 	/* free any storage allocated for firmware image */
-	ipw2100_release_firmware(priv, &fw);
+	if (!keep_firmware)
+		ipw2100_release_firmware(priv, &ipw2100_firmware);
 
 	/* zero out Domain 1 area indirectly (Si requirement) */
 	for (address = IPW_HOST_FW_SHARED_AREA0; 
@@ -843,7 +844,7 @@ static int ipw2100_download_firmware(str
 	return 0;
 
  fail:
-	ipw2100_release_firmware(priv, &fw);
+	ipw2100_release_firmware(priv, &ipw2100_firmware);
 	return err;
 }
 
@@ -6670,6 +6671,30 @@ static void __devexit ipw2100_pci_remove
 
 
 #ifdef CONFIG_PM
+static int ipw2100_save_state(struct pci_dev *pdev, u32 state)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	struct ipw2100_priv *priv = ipw2100_netdev(dev);
+	int err;
+
+	/* We're going to suspend in the near future, load firmware image
+	 * into memory so that we don't need to touch disk during resume.
+	 */
+
+	/* Mark the firmware cannot be released */
+	keep_firmware = 1;
+
+	err = ipw2100_get_firmware(priv, &ipw2100_firmware);
+	if (err) {
+		printk(KERN_ERR "%s: ipw2100_get_firmware failed: %d\n",
+		       priv->ndev->name, err);
+		priv->fatal_error = IPW2100_ERR_FW_LOAD;
+		return err;
+	}
+
+	return 0;
+}
+
 static int ipw2100_suspend(struct pci_dev *pdev, u32 state)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
@@ -6717,9 +6742,12 @@ static int ipw2100_resume(struct pci_dev
 	 * the queue of needed */
 	netif_device_attach(dev);
 
+	/* Mark the firmware can be released */
+	keep_firmware = 0;
+	
 	/* Bring the device back up */
 	ipw2100_up(priv, 0);
-	
+
 	return 0;
 }
 #endif
@@ -6785,6 +6813,7 @@ static struct pci_driver ipw2100_pci_dri
 	.probe = ipw2100_pci_init_one,
 	.remove = __devexit_p(ipw2100_pci_remove_one),
 #ifdef CONFIG_PM
+	.save_state = ipw2100_save_state,
 	.suspend = ipw2100_suspend,
 	.resume = ipw2100_resume,
 #endif
@@ -8489,6 +8518,12 @@ int ipw2100_get_firmware(struct ipw2100_
 {
 	char *fw_name;
 
+	/* Firmware might already be loaded by ipw2100_save_state. */
+	if (ipw2100_firmware.version) {
+		IPW_DEBUG_FW("Firmware is already loaded into memory\n");
+		return 0;
+	}
+
 #ifdef CONFIG_IPW2100_LEGACY_FW_LOAD
 	int err = 0;
 
@@ -8567,6 +8602,7 @@ int ipw2100_get_firmware(struct ipw2100_
 void ipw2100_release_firmware(struct ipw2100_priv *priv,
 			      struct ipw2100_fw *fw)
 {
+	fw->version = 0;
 #ifdef CONFIG_IPW2100_LEGACY_FW_LOAD
 
 	ipw2100_fw_free(fw);

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

[Pavel Machek]

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.
> 
> 
> 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.
> 
> 3 patches are attached for this idea.
> device-save_state.patch adds back the interface ->save_state for the devices
> 	and implements pci bus save_state method.
> swsusp-save_state.patch makes swsusp walks all devices ->save_state method
> 	before calling ->suspend method.
> ipw2100-save_state.patch implements ipw2100 ->save_state method. 
> They are proved working.

Are you sure you'll be able to allocate 200KB of memory at runtime?
Well, you probably can using vmalloc or such.

Yes, this looks like reasonable way to go. I guess I'd survive
notifier chain, too, because saving state can be done at any order for
todays devices, but this looks better with the driver model.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Pavel Machek]

Hi!

Docs is needed for the patch. What happens if you save_state and then
suspend fails for whatever reason?
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Dmitry Torokhov]

Hi,

Apologies for breaking threading...

Zhu, Yi wrote:
> 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.
> 
> 
> 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
> 
> Choice 2. Add a notifier call chain for swsusp before prepare_suspend.
> 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?
> 

What about splitting resume in 2 parts - the ->resume method
would just schedule_work() real resume for the card out of
line where it can safely wait for drive to be available without
blocking other devices from resuming. This way you do not need
to pre-load firmware on suspend.

-- 
Dmitry



-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Pavel Machek]

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.
> > 
> > 
> > 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
> > 
> > Choice 2. Add a notifier call chain for swsusp before prepare_suspend.
> > 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?
> > 
> 
> What about splitting resume in 2 parts - the ->resume method
> would just schedule_work() real resume for the card out of
> line where it can safely wait for drive to be available without
> blocking other devices from resuming. This way you do not need
> to pre-load firmware on suspend.

That way, userspace could see wifi card in half-suspended state. Not
good.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Zhu, Yi]

Pavel Machek wrote:
> Are you sure you'll be able to allocate 200KB of memory at runtime?
> Well, you probably can using vmalloc or such.

If malloc fails in ->save_state, the system cannot go to suspend.
The same as ->suspend fails.

> Yes, this looks like reasonable way to go. I guess I'd
> survive notifier chain, too, because saving state can be done
> at any order for todays devices, but this looks better with
> the driver model.

Thanks for your feedback.

-yi


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

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

[Zhu, Yi]

Pavel Machek wrote:
> Hi!
> 
> Docs is needed for the patch. What happens if you save_state
> and then suspend fails for whatever reason?
> 								Pavel

Yes, good point. ->resume is supposed to roll back whatever
->save_state and ->suspend did both in a success and a failure
case.

Thanks,
-yi


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php