[RFC PATCH 0/2] orinoco: Don't keep cached firmware around permanently

[RFC PATCH 0/2] orinoco: Don't keep cached firmware around permanently

[David Kilroy]

The recent patch to load orinoco firmware correctly on resume simply
kept the firmware in RAM for the entire time the module was loaded, and
only applied to Agere firmware.

The first patch uses the new power management notifiers to load and
release the firmware prior to suspend and after resume (for both Agere
and Symbol). I'm not an expert on where suspend/resume is headed, so I'd
appreciate any advice from linux-pm on whether this is the preferred way
to do things.

The second patch makes the spectrum_cs driver actually invoke this
functionality during resume.

David Kilroy (2):
  orinoco: Use PM notifier to cache firmware for use during resume
  orinoco: Resume spectrum_cs in the same way as orinoco_cs

 drivers/net/wireless/orinoco.c     |  140 ++++++++++++++++++++++++++++--------
 drivers/net/wireless/orinoco.h     |    8 ++-
 drivers/net/wireless/spectrum_cs.c |   21 +++++-
 3 files changed, 135 insertions(+), 34 deletions(-)

[RFC PATCH 1/2] orinoco: Use PM notifier to cache firmware for use during resume

[David Kilroy]

When preparing for either suspend or hibernation, load the necessary
firmware from userspace.

Upon error or resume, release the firmware.

Works for both Agere and Symbol firmware.

Signed-off by: David Kilroy <kilroyd@gmail.com>
---
 drivers/net/wireless/orinoco.c |  140 +++++++++++++++++++++++++++++++---------
 drivers/net/wireless/orinoco.h |    8 ++-
 2 files changed, 115 insertions(+), 33 deletions(-)

diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index 653306f..190fcec 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -84,6 +84,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/firmware.h>
+#include <linux/suspend.h>
 #include <linux/if_arp.h>
 #include <linux/wireless.h>
 #include <net/iw_handler.h>
@@ -431,9 +432,9 @@ struct fw_info {
 };
 
 const static struct fw_info orinoco_fw[] = {
-	{ "", "agere_sta_fw.bin", "agere_ap_fw.bin", 0x00390000, 1000 },
-	{ "", "prism_sta_fw.bin", "prism_ap_fw.bin", 0, 1024 },
-	{ "symbol_sp24t_prim_fw", "symbol_sp24t_sec_fw", "", 0x00003100, 512 }
+	{ NULL, "agere_sta_fw.bin", "agere_ap_fw.bin", 0x00390000, 1000 },
+	{ NULL, "prism_sta_fw.bin", "prism_ap_fw.bin", 0, 1024 },
+	{ "symbol_sp24t_prim_fw", "symbol_sp24t_sec_fw", NULL, 0x00003100, 512 }
 };
 
 /* Structure used to access fields in FW
@@ -487,18 +488,17 @@ orinoco_dl_firmware(struct orinoco_private *priv,
 	if (err)
 		goto free;
 
-	if (priv->cached_fw)
-		fw_entry = priv->cached_fw;
-	else {
+	if (!priv->cached_sta_fw) {
 		err = request_firmware(&fw_entry, firmware, priv->dev);
+
 		if (err) {
 			printk(KERN_ERR "%s: Cannot find firmware %s\n",
 			       dev->name, firmware);
 			err = -ENOENT;
 			goto free;
 		}
-		priv->cached_fw = fw_entry;
-	}
+	} else
+		fw_entry = priv->cached_sta_fw;
 
 	hdr = (const struct orinoco_fw_header *) fw_entry->data;
 
@@ -540,11 +540,10 @@ orinoco_dl_firmware(struct orinoco_private *priv,
 	       dev->name, hermes_present(hw));
 
 abort:
-	/* In case of error, assume firmware was bogus and release it */
-	if (err) {
-		priv->cached_fw = NULL;
+	/* If we requested the firmware, release it. Otherwise leave
+	 * it to the PM notifier */
+	if (!priv->cached_sta_fw)
 		release_firmware(fw_entry);
-	}
 
 free:
 	kfree(pda);
@@ -621,7 +620,7 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
 	ret = hermes_init(hw);
 
 	/* hermes_reset() should return 0 with the secondary firmware */
-	if (secondary && ret != 0)
+	if (secondary && (ret != 0))
 		return -ENODEV;
 
 	/* And this should work with any firmware */
@@ -648,34 +647,41 @@ symbol_dl_firmware(struct orinoco_private *priv,
 	int ret;
 	const struct firmware *fw_entry;
 
-	if (request_firmware(&fw_entry, fw->pri_fw,
-			     priv->dev) != 0) {
-		printk(KERN_ERR "%s: Cannot find firmware: %s\n",
-		       dev->name, fw->pri_fw);
-		return -ENOENT;
-	}
+	if (!priv->cached_pri_fw) {
+		if (request_firmware(&fw_entry, fw->pri_fw, priv->dev) != 0) {
+			printk(KERN_ERR "%s: Cannot find firmware: %s\n",
+			       dev->name, fw->pri_fw);
+			return -ENOENT;
+		}
+	} else
+		fw_entry = priv->cached_pri_fw;
 
 	/* Load primary firmware */
 	ret = symbol_dl_image(priv, fw, fw_entry->data,
 			      fw_entry->data + fw_entry->size, 0);
-	release_firmware(fw_entry);
+
+	if (!priv->cached_pri_fw)
+		release_firmware(fw_entry);
 	if (ret) {
 		printk(KERN_ERR "%s: Primary firmware download failed\n",
 		       dev->name);
 		return ret;
 	}
 
-	if (request_firmware(&fw_entry, fw->sta_fw,
-			     priv->dev) != 0) {
-		printk(KERN_ERR "%s: Cannot find firmware: %s\n",
-		       dev->name, fw->sta_fw);
-		return -ENOENT;
-	}
+	if (!priv->cached_sta_fw) {
+		if (request_firmware(&fw_entry, fw->sta_fw, priv->dev) != 0) {
+			printk(KERN_ERR "%s: Cannot find firmware: %s\n",
+			       dev->name, fw->sta_fw);
+			return -ENOENT;
+		}
+	} else
+		fw_entry = priv->cached_sta_fw;
 
 	/* Load secondary firmware */
 	ret = symbol_dl_image(priv, fw, fw_entry->data,
 			      fw_entry->data + fw_entry->size, 1);
-	release_firmware(fw_entry);
+	if (!priv->cached_sta_fw)
+		release_firmware(fw_entry);
 	if (ret) {
 		printk(KERN_ERR "%s: Secondary firmware download failed\n",
 		       dev->name);
@@ -3064,6 +3070,68 @@ irqreturn_t orinoco_interrupt(int irq, void *dev_id)
 }
 
 /********************************************************************/
+/* Power management                                                 */
+/********************************************************************/
+
+static int orinoco_pm_notifier(struct notifier_block *notifier,
+			       unsigned long pm_event,
+			       void *unused)
+{
+	struct orinoco_private *priv = container_of(notifier,
+						    struct orinoco_private,
+						    pm_notifier);
+
+	/* All we need to do is cache the firmware before suspend, and
+	 * release it when we come out */
+
+	switch (pm_event) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+	{
+		const struct firmware *fw_entry = NULL;
+		const char *pri_fw;
+		const char *sta_fw;
+
+		pri_fw = orinoco_fw[priv->firmware_type].pri_fw;
+		sta_fw = orinoco_fw[priv->firmware_type].sta_fw;
+
+		if (pri_fw) {
+			if (request_firmware(&fw_entry, pri_fw, priv->dev) == 0)
+				priv->cached_pri_fw = fw_entry;
+		}
+
+		if (sta_fw) {
+			if (request_firmware(&fw_entry, sta_fw, priv->dev) == 0)
+				priv->cached_sta_fw = fw_entry;
+		}
+
+		break;
+	}
+	case PM_POST_RESTORE:
+		/* Restore from hibernation failed. We need to clean
+		 * up in exactly the same way, so fall through. */
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+		if (priv->cached_pri_fw)
+			release_firmware(priv->cached_pri_fw);
+
+		if (priv->cached_sta_fw)
+			release_firmware(priv->cached_sta_fw);
+
+		priv->cached_pri_fw = NULL;
+		priv->cached_sta_fw = NULL;
+
+		break;
+
+	case PM_RESTORE_PREPARE:
+	default:
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+/********************************************************************/
 /* Initialization                                                   */
 /********************************************************************/
 
@@ -3543,7 +3611,12 @@ struct net_device
 	netif_carrier_off(dev);
 	priv->last_linkstatus = 0xffff;
 
-	priv->cached_fw = NULL;
+	priv->cached_pri_fw = NULL;
+	priv->cached_sta_fw = NULL;
+
+	/* Register PM notifiers */
+	priv->pm_notifier.notifier_call = orinoco_pm_notifier;
+	register_pm_notifier(&priv->pm_notifier);
 
 	return dev;
 }
@@ -3556,9 +3629,14 @@ void free_orinocodev(struct net_device *dev)
 	 * when we call tasklet_kill it will run one final time,
 	 * emptying the list */
 	tasklet_kill(&priv->rx_tasklet);
-	if (priv->cached_fw)
-		release_firmware(priv->cached_fw);
-	priv->cached_fw = NULL;
+	unregister_pm_notifier(&priv->pm_notifier);
+
+	if (priv->cached_pri_fw)
+		release_firmware(priv->cached_pri_fw);
+	if (priv->cached_sta_fw)
+		release_firmware(priv->cached_sta_fw);
+	priv->cached_pri_fw = NULL;
+	priv->cached_sta_fw = NULL;
 	priv->wpa_ie_len = 0;
 	kfree(priv->wpa_ie);
 	orinoco_mic_free(priv);
diff --git a/drivers/net/wireless/orinoco.h b/drivers/net/wireless/orinoco.h
index 8c29538..5a9685a 100644
--- a/drivers/net/wireless/orinoco.h
+++ b/drivers/net/wireless/orinoco.h
@@ -10,6 +10,7 @@
 #define DRIVER_VERSION "0.15"
 
 #include <linux/interrupt.h>
+#include <linux/suspend.h>
 #include <linux/netdevice.h>
 #include <linux/wireless.h>
 #include <net/iw_handler.h>
@@ -167,8 +168,11 @@ struct orinoco_private {
 	unsigned int tkip_cm_active:1;
 	unsigned int key_mgmt:3;
 
-	/* Cached in memory firmware to use in ->resume */
-	const struct firmware *cached_fw;
+	/* Cached in memory firmware to use during ->resume. */
+	const struct firmware *cached_pri_fw;
+	const struct firmware *cached_sta_fw;
+
+	struct notifier_block pm_notifier;
 };
 
 #ifdef ORINOCO_DEBUG
-- 
1.5.6.4

[RFC PATCH 2/2] orinoco: Resume spectrum_cs in the same way as orinoco_cs

[David Kilroy]

Retrieval of external firmware has been resolved, and should work with
the standard orinoco resume algorithm.

This fixes an issue where priv->hw_unavailable indicates the card is
ready when firmware has not been loaded.

Signed-off by: David Kilroy <kilroyd@googlemail.com>
---
I don't have a spectrum_cs card, so this patch really needs testing to
check resume works properly.
---
 drivers/net/wireless/spectrum_cs.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/spectrum_cs.c b/drivers/net/wireless/spectrum_cs.c
index 852789a..c6c6d12 100644
--- a/drivers/net/wireless/spectrum_cs.c
+++ b/drivers/net/wireless/spectrum_cs.c
@@ -450,10 +450,29 @@ spectrum_cs_resume(struct pcmcia_device *link)
 {
 	struct net_device *dev = link->priv;
 	struct orinoco_private *priv = netdev_priv(dev);
+	unsigned long flags;
+	int err;
+
+	err = orinoco_reinit_firmware(dev);
+	if (err) {
+		printk(KERN_ERR "%s: Error %d re-initializing firmware\n",
+		       dev->name, err);
+		return -EIO;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
 
 	netif_device_attach(dev);
 	priv->hw_unavailable--;
-	schedule_work(&priv->reset_work);
+
+	if (priv->open && !priv->hw_unavailable) {
+		err = __orinoco_up(dev);
+		if (err)
+			printk(KERN_ERR "%s: Error %d restarting card\n",
+			       dev->name, err);
+	}
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
 }
-- 
1.5.6.4

Re: [RFC PATCH 1/2] orinoco: Use PM notifier to cache firmware for use during resume

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

On Friday 31 October 2008, David Kilroy wrote:
> When preparing for either suspend or hibernation, load the necessary
> firmware from userspace.
> 
> Upon error or resume, release the firmware.
> 
> Works for both Agere and Symbol firmware.
> 
> Signed-off by: David Kilroy <kilroyd@gmail.com>

This is on top of my old patch; was it ever accepted anywhere? I guess
it should be rediffed against clean tree.

> @@ -621,7 +620,7 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
>  	ret = hermes_init(hw);
>  
>  	/* hermes_reset() should return 0 with the secondary firmware */
> -	if (secondary && ret != 0)
> +	if (secondary && (ret != 0))

Extra parenthesis are redundant, are not they?

>  /********************************************************************/
> +/* Power management                                                 */
> +/********************************************************************/
> +
> +static int orinoco_pm_notifier(struct notifier_block *notifier,
> +			       unsigned long pm_event,
> +			       void *unused)

It probably should be conditional on CONFIG_PM somehow? 

> diff --git a/drivers/net/wireless/orinoco.h b/drivers/net/wireless/orinoco.h
> index 8c29538..5a9685a 100644
> --- a/drivers/net/wireless/orinoco.h
> +++ b/drivers/net/wireless/orinoco.h
> @@ -10,6 +10,7 @@
>  #define DRIVER_VERSION "0.15"
>  
>  #include <linux/interrupt.h>
> +#include <linux/suspend.h>
>  #include <linux/netdevice.h>
>  #include <linux/wireless.h>
>  #include <net/iw_handler.h>
> @@ -167,8 +168,11 @@ struct orinoco_private {
>  	unsigned int tkip_cm_active:1;
>  	unsigned int key_mgmt:3;
>  
> -	/* Cached in memory firmware to use in ->resume */
> -	const struct firmware *cached_fw;
> +	/* Cached in memory firmware to use during ->resume. */
> +	const struct firmware *cached_pri_fw;
> +	const struct firmware *cached_sta_fw;

I think name is badly chosen. It could be both STA and AP firmware;
I know that AP is not implemented currently, but it does not mean
it will never be and firmware is there if required.

I will test it once I sort out issue with booting 2.6.28. Right now
it stopped booting completely.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [linux-pm] [RFC PATCH 1/2] orinoco: Use PM notifier to cache firmware for use during resume

[Rafael J. Wysocki]

On Friday, 31 of October 2008, Andrey Borzenkov wrote:
> On Friday 31 October 2008, David Kilroy wrote:
> > When preparing for either suspend or hibernation, load the necessary
> > firmware from userspace.
> > 
> > Upon error or resume, release the firmware.
> > 
> > Works for both Agere and Symbol firmware.
> > 
> > Signed-off by: David Kilroy <kilroyd@gmail.com>
> 
> This is on top of my old patch; was it ever accepted anywhere? I guess
> it should be rediffed against clean tree.
> 
> > @@ -621,7 +620,7 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
> >  	ret = hermes_init(hw);
> >  
> >  	/* hermes_reset() should return 0 with the secondary firmware */
> > -	if (secondary && ret != 0)
> > +	if (secondary && (ret != 0))
> 
> Extra parenthesis are redundant, are not they?
> 
> >  /********************************************************************/
> > +/* Power management                                                 */
> > +/********************************************************************/
> > +
> > +static int orinoco_pm_notifier(struct notifier_block *notifier,
> > +			       unsigned long pm_event,
> > +			       void *unused)
> 
> It probably should be conditional on CONFIG_PM somehow? 

CONFIG_PM_SLEEP, actually.

Thanks,
Rafael

Re: [RFC PATCH 0/2] orinoco: Don't keep cached firmware around permanently

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On Friday 31 October 2008, David Kilroy wrote:
> The recent patch to load orinoco firmware correctly on resume simply
> kept the firmware in RAM for the entire time the module was loaded, and
> only applied to Agere firmware.
> 
> The first patch uses the new power management notifiers to load and
> release the firmware prior to suspend and after resume (for both Agere
> and Symbol). I'm not an expert on where suspend/resume is headed, so I'd
> appreciate any advice from linux-pm on whether this is the preferred way
> to do things.
> 

I tested this and it works, and looking how callbacks are invoked I think
it is techically correct. But I have some concerns about general idea of
requesting firmware multiple times that are not related to PM.

Many properties for the currently running device/driver instance depend on
particular firmware type and version. Now we blindly replace firmware; how
can we be sure it is actually the same one as was at the time feature set
was detected?

Even if some sort of checksumming were impemented we still have to be
prepared to completely reinitialize card on FW mismatch.

I checked what the rest of drivers/net does and either they do not cache at
all (does not work in this case) or they cache FW im memory after initial
request.

Comments?

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC PATCH 0/2] orinoco: Don't keep cached firmware around permanently

[Dave]

Andrey Borzenkov wrote:
> On Friday 31 October 2008, David Kilroy wrote:
>> The first patch uses the new power management notifiers to load and
>> release the firmware prior to suspend and after resume (for both Agere
>> and Symbol). I'm not an expert on where suspend/resume is headed, so I'd
>> appreciate any advice from linux-pm on whether this is the preferred way
>> to do things.
> 
> I tested this and it works, and looking how callbacks are invoked I think
> it is techically correct. But I have some concerns about general idea of
> requesting firmware multiple times that are not related to PM.
> 
> Many properties for the currently running device/driver instance depend on
> particular firmware type and version. Now we blindly replace firmware; how
> can we be sure it is actually the same one as was at the time feature set
> was detected?
> 
> Even if some sort of checksumming were impemented we still have to be
> prepared to completely reinitialize card on FW mismatch.

Right. We actually have the same problem on card reset (which can happen
via private ioctl or firmware lockup). We certainly need to be able to
reinitialise the driver flags. I think it would involve something like

--> On init, resume, reset
1. fw download.
2. detect version (+checksum?).
3a. if version unchanged continue.
3b. if version different reset all driver settings.
4. record fw version.

3b requires some refactorring of initialisation. And I'm not sure how
this would interact with userspace on resume - presumably the driver
should call netif_device_detach or something.


Dave.

> I checked what the rest of drivers/net does and either they do not cache at
> all (does not work in this case) or they cache FW im memory after initial
> request.