[WIP] p54pci: fix resume p54 cardbus

[WIP] p54pci: fix resume p54 cardbus

[Christian Lamparter]

This patch tries to fix the resume freeze, which is described in
https://bugzilla.redhat.com/show_bug.cgi?id=583623

Unlike (normal) PCI cards, cardbus cards are easily removable.
Therefore, the user might replace/remove the card while
the system is suspended. The pcmcia subsystem takes special
precautions to deal with such cases and un- and rebinds
all devices on the pci-bridge during the resume process.

But here's the catch: p54pci uses request_firmware
which blocks until the filesystem is available.
This deadlocks, because the filesystem won't
be initialized until all pci devices are ready again.
---
highly experimental and possibly unstable
---
diff --git a/drivers/net/wireless/p54/main.c b/drivers/net/wireless/p54/main.c
index 7bbd9d3..810197c 100644
--- a/drivers/net/wireless/p54/main.c
+++ b/drivers/net/wireless/p54/main.c
@@ -603,6 +603,8 @@ int p54_register_common(struct ieee80211_hw *dev, struct device *pdev)
 		return err;
 	}
 
+	priv->registered = true;
+
 #ifdef CONFIG_P54_LEDS
 	err = p54_init_leds(priv);
 	if (err)
@@ -638,6 +640,9 @@ void p54_unregister_common(struct ieee80211_hw *dev)
 {
 	struct p54_common *priv = dev->priv;
 
+	if (!priv->registered)
+		return;
+
 #ifdef CONFIG_P54_LEDS
 	p54_unregister_leds(priv);
 #endif /* CONFIG_P54_LEDS */
diff --git a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
index 43a3b2e..850e6bb 100644
--- a/drivers/net/wireless/p54/p54.h
+++ b/drivers/net/wireless/p54/p54.h
@@ -172,6 +172,7 @@ struct p54_common {
 	struct sk_buff_head tx_pending;
 	struct sk_buff_head tx_queue;
 	struct mutex conf_mutex;
+	bool registered;
 
 	/* memory management (as seen by the firmware) */
 	u32 rx_start;
diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
index c916e46..353085d 100644
--- a/drivers/net/wireless/p54/p54pci.c
+++ b/drivers/net/wireless/p54/p54pci.c
@@ -50,7 +50,6 @@ static int p54p_upload_firmware(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
 	__le32 reg;
-	int err;
 	__le32 *data;
 	u32 remains, left, device_addr;
 
@@ -75,11 +74,7 @@ static int p54p_upload_firmware(struct ieee80211_hw *dev)
 	wmb();
 
 	/* wait for the firmware to reset properly */
-	mdelay(10);
-
-	err = p54_parse_firmware(dev, priv->firmware);
-	if (err)
-		return err;
+	mdelay(50);
 
 	if (priv->common.fw_interface != FW_LM86) {
 		dev_err(&priv->pdev->dev, "wrong firmware, "
@@ -360,8 +355,12 @@ static void p54p_stop(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
 	struct p54p_ring_control *ring_control = priv->ring_control;
-	unsigned int i;
 	struct p54p_desc *desc;
+	unsigned int i;
+
+	mutex_lock(&priv->mutex);
+	if (!priv->started)
+		goto out_unlock;
 
 	P54P_WRITE(int_enable, cpu_to_le32(0));
 	P54P_READ(int_enable);
@@ -420,6 +419,10 @@ static void p54p_stop(struct ieee80211_hw *dev)
 	}
 
 	memset(ring_control, 0, sizeof(*ring_control));
+	priv->started = false;
+
+out_unlock:
+	mutex_unlock(&priv->mutex);
 }
 
 static int p54p_open(struct ieee80211_hw *dev)
@@ -427,19 +430,25 @@ static int p54p_open(struct ieee80211_hw *dev)
 	struct p54p_priv *priv = dev->priv;
 	int err;
 
+	mutex_lock(&priv->mutex);
+	if (WARN_ON(priv->started)) {
+		err = -EBUSY;
+		goto out_unlock;
+	}
+
 	init_completion(&priv->boot_comp);
 	err = request_irq(priv->pdev->irq, p54p_interrupt,
 			  IRQF_SHARED, "p54pci", dev);
 	if (err) {
 		dev_err(&priv->pdev->dev, "failed to register IRQ handler\n");
-		return err;
+		goto out_unlock;
 	}
 
 	memset(priv->ring_control, 0, sizeof(*priv->ring_control));
 	err = p54p_upload_firmware(dev);
 	if (err) {
 		free_irq(priv->pdev->irq, dev);
-		return err;
+		goto out_unlock;
 	}
 	priv->rx_idx_data = priv->tx_idx_data = 0;
 	priv->rx_idx_mgmt = priv->tx_idx_mgmt = 0;
@@ -464,10 +473,10 @@ static int p54p_open(struct ieee80211_hw *dev)
 	P54P_READ(dev_int);
 
 	if (!wait_for_completion_interruptible_timeout(&priv->boot_comp, HZ)) {
-		printk(KERN_ERR "%s: Cannot boot firmware!\n",
-		       wiphy_name(dev->wiphy));
+		dev_err(&priv->pdev->dev, "Cannot boot firmware!");
 		p54p_stop(dev);
-		return -ETIMEDOUT;
+		err = -ETIMEDOUT;
+		goto out_unlock;
 	}
 
 	P54P_WRITE(int_enable, cpu_to_le32(ISL38XX_INT_IDENT_UPDATE));
@@ -480,7 +489,78 @@ static int p54p_open(struct ieee80211_hw *dev)
 	wmb();
 	udelay(10);
 
-	return 0;
+	priv->started = true;
+
+out_unlock:
+	mutex_unlock(&priv->mutex);
+	return err;
+}
+
+const static char *p54p_fws[] = { "isl3886pci", "isl3886" };
+
+static void p54p_fw_callback(const struct firmware *fw, void *context);
+static int p54p_request_fw(struct p54p_priv *priv)
+{
+
+	if (ARRAY_SIZE(p54p_fws) <= priv->fw_idx)
+		return -ENOENT;
+
+	return request_firmware_nowait(THIS_MODULE, 1, p54p_fws[priv->fw_idx],
+				       &priv->pdev->dev, GFP_KERNEL, priv,
+				       p54p_fw_callback);
+}
+
+static void p54p_fw_callback(const struct firmware *fw, void *context)
+{
+	struct p54p_priv *priv = context;
+	struct ieee80211_hw *dev = pci_get_drvdata(priv->pdev);
+	int err;
+
+	if (!fw) {
+		dev_err(&priv->pdev->dev, "Cannot find firmware (\"%s\")",
+			p54p_fws[priv->fw_idx]);
+		priv->fw_idx++;
+
+		err = p54p_request_fw(priv);
+		if (err)
+			goto err_out;
+
+		return;
+	}
+
+	priv->firmware = fw;
+
+	err = p54_parse_firmware(dev, priv->firmware);
+	if (err) {
+		dev_err(&priv->pdev->dev, "Failed to parse firmware");
+		goto err_out;
+	}
+
+	err = p54p_open(dev);
+	if (err)
+		goto err_out;
+	err = p54_read_eeprom(dev);
+	p54p_stop(dev);
+	if (err)
+		goto err_out;
+
+	err = p54_register_common(dev, &priv->pdev->dev);
+	if (err)
+		goto err_out;
+
+	return;
+
+err_out:
+	device_release_driver(&priv->pdev->dev);
+}
+
+static void p54p_init_dev(struct pci_dev *pdev)
+{
+	pci_set_master(pdev);
+	pci_try_set_mwi(pdev);
+
+	pci_write_config_byte(pdev, 0x40, 0);
+	pci_write_config_byte(pdev, 0x41, 0);
 }
 
 static int __devinit p54p_probe(struct pci_dev *pdev,
@@ -518,11 +598,7 @@ static int __devinit p54p_probe(struct pci_dev *pdev,
 		goto err_free_reg;
 	}
 
-	pci_set_master(pdev);
-	pci_try_set_mwi(pdev);
-
-	pci_write_config_byte(pdev, 0x40, 0);
-	pci_write_config_byte(pdev, 0x41, 0);
+	p54p_init_dev(pdev);
 
 	dev = p54_init_common(sizeof(*priv));
 	if (!dev) {
@@ -556,34 +632,16 @@ static int __devinit p54p_probe(struct pci_dev *pdev,
 	priv->common.tx = p54p_tx;
 
 	spin_lock_init(&priv->lock);
+	mutex_init(&priv->mutex);
 	tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev);
 
-	err = request_firmware(&priv->firmware, "isl3886pci",
-			       &priv->pdev->dev);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot find firmware (isl3886pci)\n");
-		err = request_firmware(&priv->firmware, "isl3886",
-				       &priv->pdev->dev);
-		if (err)
-			goto err_free_common;
-	}
-
-	err = p54p_open(dev);
-	if (err)
-		goto err_free_common;
-	err = p54_read_eeprom(dev);
-	p54p_stop(dev);
-	if (err)
-		goto err_free_common;
-
-	err = p54_register_common(dev, &pdev->dev);
+	err = p54p_request_fw(priv);
 	if (err)
 		goto err_free_common;
 
 	return 0;
 
  err_free_common:
-	release_firmware(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
 
@@ -611,6 +669,8 @@ static void __devexit p54p_remove(struct pci_dev *pdev)
 
 	p54_unregister_common(dev);
 	priv = dev->priv;
+	p54p_stop(dev);
+	mutex_destroy(&priv->mutex);
 	release_firmware(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
@@ -624,32 +684,48 @@ static void __devexit p54p_remove(struct pci_dev *pdev)
 static int p54p_suspend(struct pci_dev *pdev, pm_message_t state)
 {
 	struct ieee80211_hw *dev = pci_get_drvdata(pdev);
-	struct p54p_priv *priv = dev->priv;
-
-	if (priv->common.mode != NL80211_IFTYPE_UNSPECIFIED) {
-		ieee80211_stop_queues(dev);
-		p54p_stop(dev);
-	}
 
+	p54p_stop(dev);
 	pci_save_state(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-	return 0;
+	pci_disable_device(pdev);
+	return pci_set_power_state(pdev, pci_choose_state(pdev, state));
 }
 
 static int p54p_resume(struct pci_dev *pdev)
 {
 	struct ieee80211_hw *dev = pci_get_drvdata(pdev);
 	struct p54p_priv *priv = dev->priv;
+	int err;
+
+	err = pci_set_power_state(pdev, PCI_D0);
+	if (err) {
+		dev_err(&pdev->dev, "failed to power-up device");
+		return err;
+	}
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
+	err = pci_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to reenable device");
+		return err;
+	}
+
+	err = pci_restore_state(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to restore device state");
+		return err;
+	}
+
+	p54p_init_dev(pdev);
 
 	if (priv->common.mode != NL80211_IFTYPE_UNSPECIFIED) {
-		p54p_open(dev);
-		ieee80211_wake_queues(dev);
+		err = p54p_open(dev);
+		if (err) {
+			dev_err(&pdev->dev, "failed to bring up link");
+			return err;
+		}
 	}
 
-	return 0;
+	return err;
 }
 #endif /* CONFIG_PM */
 
diff --git a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
index 9fd822f..d4a9bdc 100644
--- a/drivers/net/wireless/p54/p54pci.h
+++ b/drivers/net/wireless/p54/p54pci.h
@@ -95,7 +95,10 @@ struct p54p_priv {
 	struct pci_dev *pdev;
 	struct p54p_csr __iomem *map;
 	struct tasklet_struct tasklet;
+	unsigned int fw_idx;
 	const struct firmware *firmware;
+	bool started;
+	struct mutex mutex;
 	spinlock_t lock;
 	struct p54p_ring_control *ring_control;
 	dma_addr_t ring_control_dma;

Re: [WIP] p54pci: fix resume p54 cardbus

[Hans de Goede]

Hi Christian,

Thanks for working on this and thanks for CC-ing me, but I don't
understand why you're making the firmware loading asynchronous...

On 04/25/2010 03:25 PM, Christian Lamparter wrote:
> This patch tries to fix the resume freeze, which is described in
> https://bugzilla.redhat.com/show_bug.cgi?id=583623
>
> Unlike (normal) PCI cards, cardbus cards are easily removable.
> Therefore, the user might replace/remove the card while
> the system is suspended. The pcmcia subsystem takes special
> precautions to deal with such cases and un- and rebinds
> all devices on the pci-bridge during the resume process.
>
> But here's the catch: p54pci uses request_firmware
> which blocks until the filesystem is available.
> This deadlocks, because the filesystem won't
> be initialized until all pci devices are ready again.

p54pci uses request_firmware only from its probe function,
which does not get called during a suspend resume AFAIK.

And if the card was inserted during a suspend / resume. I would
not expect its driver to get loaded until the resume has completed
and udev runs again.

Hmm, I think while typing this message I just understood what you're
trying to fix. The problem could occur when the driver is already loaded
(for some reason), but not yet bound to the device as the card got
inserted during suspend. Does the probe function get called during
resume then ?

This would seem like a driver core bug to me. It should not start
binding drivers to "new" devices, until the resume is completed IMHO.

Note that the problem which I'm mostly seeing with suspend resume,
is that the card fails shortly after resume. It seems to be come up
and I can send / receive some packets and then it fails. The changes
you're making to resume where you're actually calling pci enable
on the device, and re-doing some pci config space settings might help
here (I'm currently unloading the driver before suspend and reloading
it after resume and then things work fine).

The one actual lockup I experienced in combination with suspend / resume
was before the other p54pci bug we've working on together was fixed,
as the driver was unstable at that time in general, so I'm not overly
worried about that one lockup.

Regards,

Hans




> ---
> highly experimental and possibly unstable
> ---
> diff --git a/drivers/net/wireless/p54/main.c b/drivers/net/wireless/p54/main.c
> index 7bbd9d3..810197c 100644
> --- a/drivers/net/wireless/p54/main.c
> +++ b/drivers/net/wireless/p54/main.c
> @@ -603,6 +603,8 @@ int p54_register_common(struct ieee80211_hw *dev, struct device *pdev)
>   		return err;
>   	}
>
> +	priv->registered = true;
> +
>   #ifdef CONFIG_P54_LEDS
>   	err = p54_init_leds(priv);
>   	if (err)
> @@ -638,6 +640,9 @@ void p54_unregister_common(struct ieee80211_hw *dev)
>   {
>   	struct p54_common *priv = dev->priv;
>
> +	if (!priv->registered)
> +		return;
> +
>   #ifdef CONFIG_P54_LEDS
>   	p54_unregister_leds(priv);
>   #endif /* CONFIG_P54_LEDS */
> diff --git a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
> index 43a3b2e..850e6bb 100644
> --- a/drivers/net/wireless/p54/p54.h
> +++ b/drivers/net/wireless/p54/p54.h
> @@ -172,6 +172,7 @@ struct p54_common {
>   	struct sk_buff_head tx_pending;
>   	struct sk_buff_head tx_queue;
>   	struct mutex conf_mutex;
> +	bool registered;
>
>   	/* memory management (as seen by the firmware) */
>   	u32 rx_start;
> diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
> index c916e46..353085d 100644
> --- a/drivers/net/wireless/p54/p54pci.c
> +++ b/drivers/net/wireless/p54/p54pci.c
> @@ -50,7 +50,6 @@ static int p54p_upload_firmware(struct ieee80211_hw *dev)
>   {
>   	struct p54p_priv *priv = dev->priv;
>   	__le32 reg;
> -	int err;
>   	__le32 *data;
>   	u32 remains, left, device_addr;
>
> @@ -75,11 +74,7 @@ static int p54p_upload_firmware(struct ieee80211_hw *dev)
>   	wmb();
>
>   	/* wait for the firmware to reset properly */
> -	mdelay(10);
> -
> -	err = p54_parse_firmware(dev, priv->firmware);
> -	if (err)
> -		return err;
> +	mdelay(50);
>
>   	if (priv->common.fw_interface != FW_LM86) {
>   		dev_err(&priv->pdev->dev, "wrong firmware, "
> @@ -360,8 +355,12 @@ static void p54p_stop(struct ieee80211_hw *dev)
>   {
>   	struct p54p_priv *priv = dev->priv;
>   	struct p54p_ring_control *ring_control = priv->ring_control;
> -	unsigned int i;
>   	struct p54p_desc *desc;
> +	unsigned int i;
> +
> +	mutex_lock(&priv->mutex);
> +	if (!priv->started)
> +		goto out_unlock;
>
>   	P54P_WRITE(int_enable, cpu_to_le32(0));
>   	P54P_READ(int_enable);
> @@ -420,6 +419,10 @@ static void p54p_stop(struct ieee80211_hw *dev)
>   	}
>
>   	memset(ring_control, 0, sizeof(*ring_control));
> +	priv->started = false;
> +
> +out_unlock:
> +	mutex_unlock(&priv->mutex);
>   }
>
>   static int p54p_open(struct ieee80211_hw *dev)
> @@ -427,19 +430,25 @@ static int p54p_open(struct ieee80211_hw *dev)
>   	struct p54p_priv *priv = dev->priv;
>   	int err;
>
> +	mutex_lock(&priv->mutex);
> +	if (WARN_ON(priv->started)) {
> +		err = -EBUSY;
> +		goto out_unlock;
> +	}
> +
>   	init_completion(&priv->boot_comp);
>   	err = request_irq(priv->pdev->irq, p54p_interrupt,
>   			  IRQF_SHARED, "p54pci", dev);
>   	if (err) {
>   		dev_err(&priv->pdev->dev, "failed to register IRQ handler\n");
> -		return err;
> +		goto out_unlock;
>   	}
>
>   	memset(priv->ring_control, 0, sizeof(*priv->ring_control));
>   	err = p54p_upload_firmware(dev);
>   	if (err) {
>   		free_irq(priv->pdev->irq, dev);
> -		return err;
> +		goto out_unlock;
>   	}
>   	priv->rx_idx_data = priv->tx_idx_data = 0;
>   	priv->rx_idx_mgmt = priv->tx_idx_mgmt = 0;
> @@ -464,10 +473,10 @@ static int p54p_open(struct ieee80211_hw *dev)
>   	P54P_READ(dev_int);
>
>   	if (!wait_for_completion_interruptible_timeout(&priv->boot_comp, HZ)) {
> -		printk(KERN_ERR "%s: Cannot boot firmware!\n",
> -		       wiphy_name(dev->wiphy));
> +		dev_err(&priv->pdev->dev, "Cannot boot firmware!");
>   		p54p_stop(dev);
> -		return -ETIMEDOUT;
> +		err = -ETIMEDOUT;
> +		goto out_unlock;
>   	}
>
>   	P54P_WRITE(int_enable, cpu_to_le32(ISL38XX_INT_IDENT_UPDATE));
> @@ -480,7 +489,78 @@ static int p54p_open(struct ieee80211_hw *dev)
>   	wmb();
>   	udelay(10);
>
> -	return 0;
> +	priv->started = true;
> +
> +out_unlock:
> +	mutex_unlock(&priv->mutex);
> +	return err;
> +}
> +
> +const static char *p54p_fws[] = { "isl3886pci", "isl3886" };
> +
> +static void p54p_fw_callback(const struct firmware *fw, void *context);
> +static int p54p_request_fw(struct p54p_priv *priv)
> +{
> +
> +	if (ARRAY_SIZE(p54p_fws)<= priv->fw_idx)
> +		return -ENOENT;
> +
> +	return request_firmware_nowait(THIS_MODULE, 1, p54p_fws[priv->fw_idx],
> +				&priv->pdev->dev, GFP_KERNEL, priv,
> +				       p54p_fw_callback);
> +}
> +
> +static void p54p_fw_callback(const struct firmware *fw, void *context)
> +{
> +	struct p54p_priv *priv = context;
> +	struct ieee80211_hw *dev = pci_get_drvdata(priv->pdev);
> +	int err;
> +
> +	if (!fw) {
> +		dev_err(&priv->pdev->dev, "Cannot find firmware (\"%s\")",
> +			p54p_fws[priv->fw_idx]);
> +		priv->fw_idx++;
> +
> +		err = p54p_request_fw(priv);
> +		if (err)
> +			goto err_out;
> +
> +		return;
> +	}
> +
> +	priv->firmware = fw;
> +
> +	err = p54_parse_firmware(dev, priv->firmware);
> +	if (err) {
> +		dev_err(&priv->pdev->dev, "Failed to parse firmware");
> +		goto err_out;
> +	}
> +
> +	err = p54p_open(dev);
> +	if (err)
> +		goto err_out;
> +	err = p54_read_eeprom(dev);
> +	p54p_stop(dev);
> +	if (err)
> +		goto err_out;
> +
> +	err = p54_register_common(dev,&priv->pdev->dev);
> +	if (err)
> +		goto err_out;
> +
> +	return;
> +
> +err_out:
> +	device_release_driver(&priv->pdev->dev);
> +}
> +
> +static void p54p_init_dev(struct pci_dev *pdev)
> +{
> +	pci_set_master(pdev);
> +	pci_try_set_mwi(pdev);
> +
> +	pci_write_config_byte(pdev, 0x40, 0);
> +	pci_write_config_byte(pdev, 0x41, 0);
>   }
>
>   static int __devinit p54p_probe(struct pci_dev *pdev,
> @@ -518,11 +598,7 @@ static int __devinit p54p_probe(struct pci_dev *pdev,
>   		goto err_free_reg;
>   	}
>
> -	pci_set_master(pdev);
> -	pci_try_set_mwi(pdev);
> -
> -	pci_write_config_byte(pdev, 0x40, 0);
> -	pci_write_config_byte(pdev, 0x41, 0);
> +	p54p_init_dev(pdev);
>
>   	dev = p54_init_common(sizeof(*priv));
>   	if (!dev) {
> @@ -556,34 +632,16 @@ static int __devinit p54p_probe(struct pci_dev *pdev,
>   	priv->common.tx = p54p_tx;
>
>   	spin_lock_init(&priv->lock);
> +	mutex_init(&priv->mutex);
>   	tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev);
>
> -	err = request_firmware(&priv->firmware, "isl3886pci",
> -			&priv->pdev->dev);
> -	if (err) {
> -		dev_err(&pdev->dev, "Cannot find firmware (isl3886pci)\n");
> -		err = request_firmware(&priv->firmware, "isl3886",
> -				&priv->pdev->dev);
> -		if (err)
> -			goto err_free_common;
> -	}
> -
> -	err = p54p_open(dev);
> -	if (err)
> -		goto err_free_common;
> -	err = p54_read_eeprom(dev);
> -	p54p_stop(dev);
> -	if (err)
> -		goto err_free_common;
> -
> -	err = p54_register_common(dev,&pdev->dev);
> +	err = p54p_request_fw(priv);
>   	if (err)
>   		goto err_free_common;
>
>   	return 0;
>
>    err_free_common:
> -	release_firmware(priv->firmware);
>   	pci_free_consistent(pdev, sizeof(*priv->ring_control),
>   			    priv->ring_control, priv->ring_control_dma);
>
> @@ -611,6 +669,8 @@ static void __devexit p54p_remove(struct pci_dev *pdev)
>
>   	p54_unregister_common(dev);
>   	priv = dev->priv;
> +	p54p_stop(dev);
> +	mutex_destroy(&priv->mutex);
>   	release_firmware(priv->firmware);
>   	pci_free_consistent(pdev, sizeof(*priv->ring_control),
>   			    priv->ring_control, priv->ring_control_dma);
> @@ -624,32 +684,48 @@ static void __devexit p54p_remove(struct pci_dev *pdev)
>   static int p54p_suspend(struct pci_dev *pdev, pm_message_t state)
>   {
>   	struct ieee80211_hw *dev = pci_get_drvdata(pdev);
> -	struct p54p_priv *priv = dev->priv;
> -
> -	if (priv->common.mode != NL80211_IFTYPE_UNSPECIFIED) {
> -		ieee80211_stop_queues(dev);
> -		p54p_stop(dev);
> -	}
>
> +	p54p_stop(dev);
>   	pci_save_state(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -	return 0;
> +	pci_disable_device(pdev);
> +	return pci_set_power_state(pdev, pci_choose_state(pdev, state));
>   }
>
>   static int p54p_resume(struct pci_dev *pdev)
>   {
>   	struct ieee80211_hw *dev = pci_get_drvdata(pdev);
>   	struct p54p_priv *priv = dev->priv;
> +	int err;
> +
> +	err = pci_set_power_state(pdev, PCI_D0);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to power-up device");
> +		return err;
> +	}
>
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> +	err = pci_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to reenable device");
> +		return err;
> +	}
> +
> +	err = pci_restore_state(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to restore device state");
> +		return err;
> +	}
> +
> +	p54p_init_dev(pdev);
>
>   	if (priv->common.mode != NL80211_IFTYPE_UNSPECIFIED) {
> -		p54p_open(dev);
> -		ieee80211_wake_queues(dev);
> +		err = p54p_open(dev);
> +		if (err) {
> +			dev_err(&pdev->dev, "failed to bring up link");
> +			return err;
> +		}
>   	}
>
> -	return 0;
> +	return err;
>   }
>   #endif /* CONFIG_PM */
>
> diff --git a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
> index 9fd822f..d4a9bdc 100644
> --- a/drivers/net/wireless/p54/p54pci.h
> +++ b/drivers/net/wireless/p54/p54pci.h
> @@ -95,7 +95,10 @@ struct p54p_priv {
>   	struct pci_dev *pdev;
>   	struct p54p_csr __iomem *map;
>   	struct tasklet_struct tasklet;
> +	unsigned int fw_idx;
>   	const struct firmware *firmware;
> +	bool started;
> +	struct mutex mutex;
>   	spinlock_t lock;
>   	struct p54p_ring_control *ring_control;
>   	dma_addr_t ring_control_dma;

Re: [WIP] p54pci: fix resume p54 cardbus

[Christian Lamparter]

On Monday 26 April 2010 11:38:06 Hans de Goede wrote: 
> On 04/25/2010 03:25 PM, Christian Lamparter wrote:
> > This patch tries to fix the resume freeze, which is described in
> > https://bugzilla.redhat.com/show_bug.cgi?id=583623
> >
> > Unlike (normal) PCI cards, cardbus cards are easily removable.
> > Therefore, the user might replace/remove the card while
> > the system is suspended. The pcmcia subsystem takes special
> > precautions to deal with such cases and un- and rebinds
> > all devices on the pci-bridge during the resume process.
> >
> > But here's the catch: p54pci uses request_firmware
> > which blocks until the filesystem is available.
> > This deadlocks, because the filesystem won't
> > be initialized until all pci devices are ready again.
> 
> p54pci uses request_firmware only from its probe function,
> which does not get called during a suspend resume AFAIK.
> 
> And if the card was inserted during a suspend / resume. I would
> not expect its driver to get loaded until the resume has completed
> and udev runs again.

no, all 2.6.34-rcX-wl kernels - I tried - refused to resume if
a p54pci was present in the cardslot. The culprit was found to be:

commit 88b060d6c03fcb9e4d2018b4349954c4242a5c7f               
Author: Dominik Brodowski <linux@dominikbrodowski.net>        
Date:   Sat Jan 2 14:14:23 2010 +0100                         

    pcmcia: improve check for same card in slot after resume
                                                            
    During a suspend/resume cycle, an user may change the card in the
    PCMCIA/CardBus slot. The pcmcia_core can at least look at the    
    socket state to check whether it is the same.                    
                                                                     
    For PCMCIA devices, move the detection and handling of such a    
    change to ds.c.                                                  
                                                                     
--> For CardBus devices, the PCI hotplug interface doesn't offer a "rescan"
    facility which also _removes_ devices no longer to be found behind a   
    bridge. Therefore, remove and re-add all devices unconditionally.  

> Hmm, I think while typing this message I just understood what you're
> trying to fix. The problem could occur when the driver is already loaded
> (for some reason), but not yet bound to the device as the card got
> inserted during suspend. Does the probe function get called during
> resume then ?

yes, due to "pcmcia: improve check for same card in slot after resume"
the p54p_probe function is now always called during resume. And as
you know this deadlocks, unless the firmware is included into the
kernel.

> This would seem like a driver core bug to me. It should not start
> binding drivers to "new" devices, until the resume is completed IMHO.
Well, the commit author does have a point.

what if I replace my Netgear WG511 with different Netgear WG511
(or even SMC W2835V2) while the system is suspendend?
All cards are fullmac p54pci, but they are not the same HW.

On the other hand, this patch breaks basically any cardbus
device driver which needs firmware and doesn't use the asynchronus
request interface. Also, (in case of mac80211) the unbind/bind
procedure resets mac80211/driver/etc.. to their uninitialized states
(and changes a few things, e.g.: phyX ids and friends). 
This could very well confuse the userspace, because after the
resume all configurations are gone...

> Note that the problem which I'm mostly seeing with suspend resume,
> is that the card fails shortly after resume. It seems to be come up
> and I can send / receive some packets and then it fails. The changes
> you're making to resume where you're actually calling pci enable
> on the device, and re-doing some pci config space settings might help
> here (I'm currently unloading the driver before suspend and reloading
> it after resume and then things work fine).
hm, can't say much about that... But, I have a (mini)pci system,
which hopefully won't unbind/rebind the devices upon resume.

> The one actual lockup I experienced in combination with suspend / resume
> was before the other p54pci bug we've working on together was fixed,
> as the driver was unstable at that time in general, so I'm not overly
> worried about that one lockup.
Ok, understood.

Regards,
   Chr

Re: [WIP] p54pci: fix resume p54 cardbus

[Hans de Goede]

Hi,

On 04/26/2010 02:12 PM, Christian Lamparter wrote:
> On Monday 26 April 2010 11:38:06 Hans de Goede wrote:
>> On 04/25/2010 03:25 PM, Christian Lamparter wrote:
>>> This patch tries to fix the resume freeze, which is described in
>>> https://bugzilla.redhat.com/show_bug.cgi?id=583623
>>>
>>> Unlike (normal) PCI cards, cardbus cards are easily removable.
>>> Therefore, the user might replace/remove the card while
>>> the system is suspended. The pcmcia subsystem takes special
>>> precautions to deal with such cases and un- and rebinds
>>> all devices on the pci-bridge during the resume process.
>>>
>>> But here's the catch: p54pci uses request_firmware
>>> which blocks until the filesystem is available.
>>> This deadlocks, because the filesystem won't
>>> be initialized until all pci devices are ready again.
>>
>> p54pci uses request_firmware only from its probe function,
>> which does not get called during a suspend resume AFAIK.
>>
>> And if the card was inserted during a suspend / resume. I would
>> not expect its driver to get loaded until the resume has completed
>> and udev runs again.
>
> no, all 2.6.34-rcX-wl kernels - I tried - refused to resume if
> a p54pci was present in the cardslot. The culprit was found to be:
>
> commit 88b060d6c03fcb9e4d2018b4349954c4242a5c7f
> Author: Dominik Brodowski<linux@dominikbrodowski.net>
> Date:   Sat Jan 2 14:14:23 2010 +0100
>
>      pcmcia: improve check for same card in slot after resume
>
>      During a suspend/resume cycle, an user may change the card in the
>      PCMCIA/CardBus slot. The pcmcia_core can at least look at the
>      socket state to check whether it is the same.
>
>      For PCMCIA devices, move the detection and handling of such a
>      change to ds.c.
>
> -->  For CardBus devices, the PCI hotplug interface doesn't offer a "rescan"
>      facility which also _removes_ devices no longer to be found behind a
>      bridge. Therefore, remove and re-add all devices unconditionally.
>
>> Hmm, I think while typing this message I just understood what you're
>> trying to fix. The problem could occur when the driver is already loaded
>> (for some reason), but not yet bound to the device as the card got
>> inserted during suspend. Does the probe function get called during
>> resume then ?
>
> yes, due to "pcmcia: improve check for same card in slot after resume"
> the p54p_probe function is now always called during resume. And as
> you know this deadlocks, unless the firmware is included into the
> kernel.
>
>> This would seem like a driver core bug to me. It should not start
>> binding drivers to "new" devices, until the resume is completed IMHO.
> Well, the commit author does have a point.
>
> what if I replace my Netgear WG511 with different Netgear WG511
> (or even SMC W2835V2) while the system is suspendend?
> All cards are fullmac p54pci, but they are not the same HW.
>

Unbinding / rebinding the driver sounds like a really big hammer
to me though. Maybe drivers need a new re-scan hook or some such.

> On the other hand, this patch breaks basically any cardbus
> device driver which needs firmware and doesn't use the asynchronus
> request interface. Also, (in case of mac80211) the unbind/bind
> procedure resets mac80211/driver/etc.. to their uninitialized states
> (and changes a few things, e.g.: phyX ids and friends).
> This could very well confuse the userspace, because after the
> resume all configurations are gone...
>

Yeah, this sounds like a case of the cure being worse then the disease
(as we say in the Netherlands).

>> Note that the problem which I'm mostly seeing with suspend resume,
>> is that the card fails shortly after resume. It seems to be come up
>> and I can send / receive some packets and then it fails. The changes
>> you're making to resume where you're actually calling pci enable
>> on the device, and re-doing some pci config space settings might help
>> here (I'm currently unloading the driver before suspend and reloading
>> it after resume and then things work fine).
> hm, can't say much about that... But, I have a (mini)pci system,
> which hopefully won't unbind/rebind the devices upon resume.
>

I've taking a look at this on my to do list, and some parts of your WIP
patch look like a good candidate for initial testing. I must say this is
rather low on my todo though as the remove module before suspend / reinsert
after resume fix works and my to do list is rather full.

Regards,

Hans