iwlwifi MSI support (rev3)

PATCH In review Stephen Hemminger 2 participants 6 messages Jun 14, 2007 – Jun 15, 2007 Networking drivers (wireless)

Share & tools

Stephen Hemminger

author

2007-06-14 18:49 UTC #0 lore raw

PATCH 1/1 iwlwifi MSI support (rev3)

The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.

drivers/net/wireless/mac80211/iwlwifi/base.c +7 -2

@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);
+
 	err = request_irq(pdev->irq, iwl_isr, IRQF_SHARED, DRV_NAME, priv);
 	if (err) {
 		IWL_ERROR("Error allocating IRQ %d\n", pdev->irq);
-		goto out_destroy_workqueue;
+		goto out_disable_msi;
 	}
 
 	mutex_lock(&priv->mutex);

@@ -8157,7 +8160,8 @@ static int iwl_pci_probe(struct pci_dev 
  out_release_irq:
 	free_irq(pdev->irq, priv);
 
- out_destroy_workqueue:
+ out_disable_msi:
+	pci_disable_msi(pdev);
 	destroy_workqueue(priv->workqueue);
 	priv->workqueue = NULL;
 	iwl_unset_hw_setting(priv);

@@ -8225,6 +8229,7 @@ static void iwl_pci_remove(struct pci_de
 	priv->workqueue = NULL;
 
 	free_irq(pdev->irq, priv);
+	pci_disable_msi(pdev);
 	iounmap(priv->hw_base);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);

Jeff Garzik

2007-06-14 18:52 UTC #1 lore raw

drivers/net/wireless/mac80211/iwlwifi/base.c static int iwl_pci_probe(struct pci_dev · line 8121

Quoted 12 lines · Stephen Hemminger

The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.
drivers/net/wireless/mac80211/iwlwifi/base.c +2 -0
@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);

The API wants fixing, if code that fails to check return value of pci_enable_msi() is being written.

We should definitely be checking the return value there...

Jeff

Stephen Hemminger

author

2007-06-14 20:18 UTC #2 lore raw

Quoted 5 lines · On Thu, 14 Jun 2007 14:52:42 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 4 lines · Stephen Hemminger
The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.

Quoted 11 lines

drivers/net/wireless/mac80211/iwlwifi/base.c static int iwl_pci_probe(struct pci_dev · line 8121
Quoted 8 lines
drivers/net/wireless/mac80211/iwlwifi/base.c +2 -0
@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);
The API wants fixing, if code that fails to check return value of pci_enable_msi() is being written.

Why, what would be worth doing?

Stephen Hemminger

author

2007-06-14 22:48 UTC #3 lore raw

In reply to Jeff Garzik

Quoted 17 lines · On Thu, 14 Jun 2007 14:52:42 -0400 Jeff Garzik <[email protected]> wrote:

drivers/net/wireless/mac80211/iwlwifi/base.c static int iwl_pci_probe(struct pci_dev · line 8121
Quoted 12 lines · Stephen Hemminger
The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.
drivers/net/wireless/mac80211/iwlwifi/base.c +2 -0
@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);
The API wants fixing, if code that fails to check return value of pci_enable_msi() is being written.

Quoted 4 lines

We should definitely be checking the return value there...
Jeff

I don't understand. pci_enable_msi() returns error if device doesn't do MSI, that's fine. The device driver doesn't have to care or do anything different. The API is well written, it just works.

Jeff Garzik

2007-06-14 23:02 UTC #4 lore raw

Quoted 18 lines · Stephen Hemminger

Quoted 15 lines · On Thu, 14 Jun 2007 14:52:42 -0400 Jeff Garzik <[email protected]> wrote:
drivers/net/wireless/mac80211/iwlwifi/base.c static int iwl_pci_probe(struct pci_dev · line 8121
Quoted 12 lines · Stephen Hemminger
The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.
drivers/net/wireless/mac80211/iwlwifi/base.c +2 -0
@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);
The API wants fixing, if code that fails to check return value of pci_enable_msi() is being written.

Quoted 7 lines

Quoted 4 lines
We should definitely be checking the return value there...
Jeff
I don't understand. pci_enable_msi() returns error if device doesn't do MSI, that's fine.

It also returns errors for conditions other than "doesn't do MSI", and therein lies the problem with not checking return values.

Jeff

Stephen Hemminger

author

2007-06-15 00:35 UTC #5 lore raw

Quoted 22 lines · On Thu, 14 Jun 2007 19:02:54 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 21 lines · Stephen Hemminger
Quoted 18 lines · On Thu, 14 Jun 2007 14:52:42 -0400 Jeff Garzik <[email protected]> wrote:
drivers/net/wireless/mac80211/iwlwifi/base.c static int iwl_pci_probe(struct pci_dev · line 8121
Quoted 12 lines · Stephen Hemminger
The previous patch would crash on remove. pci_disable_msi has to be called after free_irq. FYI - pci_disable_msi is safe if MSI hasn't been enabled.
drivers/net/wireless/mac80211/iwlwifi/base.c +2 -0
@@ -8117,10 +8117,13 @@ static int iwl_pci_probe(struct pci_dev 
 	/* If power management is turned on, default to AC mode */
 	priv->power_mode = IWL_POWER_AC;
 	priv->user_txpower_limit = IWL_DEFAULT_TX_POWER;
+
+	pci_enable_msi(pdev);
The API wants fixing, if code that fails to check return value of pci_enable_msi() is being written.
We should definitely be checking the return value there...

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Jeff
I don't understand. pci_enable_msi() returns error if device doesn't do MSI, that's fine.
It also returns errors for conditions other than "doesn't do MSI", and therein lies the problem with not checking return values.

Errors:
Not MSI device - don't care
MSI disabled globally - don-t care
Already doing MSI-X - message already, don't care Out of memory can't setup - don't care

The error codes are a mess so it is better to just not print anything and just continue on with normal INT mode.