[PATCH] iwlwifi MSI support (rev3)

[PATCH] iwlwifi MSI support (rev3)

[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.

--- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
+++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
@@ -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);

Re: [PATCH] iwlwifi MSI support (rev3)

[Jeff Garzik]

Stephen Hemminger wrote:
> 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.
> 
> --- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
> +++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
> @@ -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

Re: [PATCH] iwlwifi MSI support (rev3)

[Stephen Hemminger]

On Thu, 14 Jun 2007 14:52:42 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Stephen Hemminger wrote:
> > 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.
> > 
> > --- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
> > +++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
> > @@ -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?

Re: [PATCH] iwlwifi MSI support (rev3)

[Stephen Hemminger]

On Thu, 14 Jun 2007 14:52:42 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Stephen Hemminger wrote:
> > 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.
> > 
> > --- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
> > +++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
> > @@ -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
> 

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.

Re: [PATCH] iwlwifi MSI support (rev3)

[Jeff Garzik]

Stephen Hemminger wrote:
> On Thu, 14 Jun 2007 14:52:42 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Stephen Hemminger wrote:
>>> 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.
>>>
>>> --- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
>>> +++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
>>> @@ -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
>>
> 
> 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

Re: [PATCH] iwlwifi MSI support (rev3)

[Stephen Hemminger]

On Thu, 14 Jun 2007 19:02:54 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Stephen Hemminger wrote:
> > On Thu, 14 Jun 2007 14:52:42 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> Stephen Hemminger wrote:
> >>> 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.
> >>>
> >>> --- a/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-13 11:10:10.000000000 -0700
> >>> +++ b/drivers/net/wireless/mac80211/iwlwifi/base.c	2007-06-14 11:47:01.000000000 -0700
> >>> @@ -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
> >>
> > 
> > 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.