[PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

Hi,

When device driver unload, it may call pci_disable_msi(), if msi did not 
enabled but do msi_unmap_pirq(), then later driver reload and without
msi, then will failed in request_irq() for irq_desc[irq]->chip valie is 
no_irq_chip. So when did not enable msi during driver initializing, then
unloaded driver will not try to disable it.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 msi-xen.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- a/drivers/pci/msi-xen.c	2009-11-16 10:48:26.000000000 +0800
+++ b/drivers/pci/msi-xen.c	2009-11-16 19:27:17.000000000 +0800
@@ -670,6 +670,12 @@ void pci_disable_msi(struct pci_dev* dev
 	if (!pos)
 		return;
 
+	if (!(dev->msi_enabled)) {
+		printk(KERN_INFO "PCI: %s: Device did not eanble MSI.\n",
+		       pci_name(dev));
+		return;
+	}
+	
 	pirq = dev->irq;
 	/* Restore dev->irq to its default pin-assertion vector */
 	dev->irq = dev->irq_old;

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Konrad Rzeszutek Wilk]

On Mon, Nov 16, 2009 at 08:00:30PM +0800, Joe Jin wrote:
> Hi,
> 
> When device driver unload, it may call pci_disable_msi(), if msi did not 
> enabled but do msi_unmap_pirq(), then later driver reload and without

Where does that happen? That looks to be a driver bug as well.

> msi, then will failed in request_irq() for irq_desc[irq]->chip valie is 
> no_irq_chip. So when did not enable msi during driver initializing, then

Won't that mean it is unusable? As in, you can't allocate an IRQ
to the device when the irq_desc[irq]->chip_value==no_irq_chip?

What kernel is this for? It does not look like the 2.6.18-xen.hg?

Either way, patch looks fine (except the spelling error).

> unloaded driver will not try to disable it.
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
>  msi-xen.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/pci/msi-xen.c	2009-11-16 10:48:26.000000000 +0800
> +++ b/drivers/pci/msi-xen.c	2009-11-16 19:27:17.000000000 +0800
> @@ -670,6 +670,12 @@ void pci_disable_msi(struct pci_dev* dev
>  	if (!pos)
>  		return;
>  
> +	if (!(dev->msi_enabled)) {
> +		printk(KERN_INFO "PCI: %s: Device did not eanble MSI.\n",
                                                          ^^^^^- enable.

> +		       pci_name(dev));
> +		return;
> +	}
> +	
>  	pirq = dev->irq;
>  	/* Restore dev->irq to its default pin-assertion vector */
>  	dev->irq = dev->irq_old;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Konrad Rzeszutek Wilk]

On Mon, Nov 16, 2009 at 10:15:46AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 16, 2009 at 08:00:30PM +0800, Joe Jin wrote:
> > Hi,
> > 
> > When device driver unload, it may call pci_disable_msi(), if msi did not 
> > enabled but do msi_unmap_pirq(), then later driver reload and without
> 
> Where does that happen? That looks to be a driver bug as well.
> 
> > msi, then will failed in request_irq() for irq_desc[irq]->chip valie is 
> > no_irq_chip. So when did not enable msi during driver initializing, then
> 
> Won't that mean it is unusable? As in, you can't allocate an IRQ
> to the device when the irq_desc[irq]->chip_value==no_irq_chip?

Duh! I think I answered myself here and the answer is yes, otherwise you
would not have hit this bug :-(

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

On 2009-11-16 10:15, Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 16, 2009 at 08:00:30PM +0800, Joe Jin wrote:
> > Hi,
> > 
> > When device driver unload, it may call pci_disable_msi(), if msi did not 
> > enabled but do msi_unmap_pirq(), then later driver reload and without
> 
> Where does that happen? That looks to be a driver bug as well.

We tried to reload qla2xxx driver, unload was worked, but installed the module
again, failed with ""Failed to reserve interrupt 22/23 already in use."

Have tried no-xen kernel and it worked fine, dig the codes found when unloaded
the driver, the driver always call pci_disable_msi() even driver call 
pci_enable_msi() during driver initializing.

Compared xen pci_disable_msi() with no-xen pci_disable_msi() found it caused 
by called msi_unmap_pirq(), at the function it set irq_desc[irq]->chip to
&no_irq_chip.

Then when tried to install the driver again via request_irq(), when call
setup_irq(), the function found the chip's value is no_irq_chip then return
-ENOSYS and failed to assign the irq to driver.


> 
> > +	if (!(dev->msi_enabled)) {
> > +		printk(KERN_INFO "PCI: %s: Device did not eanble MSI.\n",
>                                                           ^^^^^- enable.

Thanks and regenerated the patch :)


Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 msi-xen.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff -r 6301ebc85480 drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Fri Oct 23 10:07:22 2009 +0100
+++ b/drivers/pci/msi-xen.c	Tue Nov 17 08:16:42 2009 +0800
@@ -673,6 +673,12 @@
 	if (!pos)
 		return;
 
+	if (!(dev->msi_enabled)) {
+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+		       pci_name(dev));
+		return;
+	}
+ 
 	pirq = dev->irq;
 	/* Restore dev->irq to its default pin-assertion vector */
 	dev->irq = msi_dev_entry->default_irq;

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jan Beulich]

>>> Joe Jin <joe.jin@oracle.com> 17.11.09 01:19 >>>
>--- a/drivers/pci/msi-xen.c	Fri Oct 23 10:07:22 2009 +0100
>+++ b/drivers/pci/msi-xen.c	Tue Nov 17 08:16:42 2009 +0800
>@@ -673,6 +673,12 @@
> 	if (!pos)
> 		return;
> 
>+	if (!(dev->msi_enabled)) {
>+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
>+		       pci_name(dev));
>+		return;
>+	}
>+ 
> 	pirq = dev->irq;
> 	/* Restore dev->irq to its default pin-assertion vector */
> 	dev->irq = msi_dev_entry->default_irq;

But shouldn't this happen before the CONFIG_XEN_PCIDEV_FRONTEND
conditional block? This one also calls evtchn_map_pirq(..., 0), i.e. would
also result in the storing of no_irq_chip.

Jan

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

On 2009-11-17 07:59, Jan Beulich wrote:
> >>> Joe Jin <joe.jin@oracle.com> 17.11.09 01:19 >>>
> >--- a/drivers/pci/msi-xen.c	Fri Oct 23 10:07:22 2009 +0100
> >+++ b/drivers/pci/msi-xen.c	Tue Nov 17 08:16:42 2009 +0800
> >@@ -673,6 +673,12 @@
> > 	if (!pos)
> > 		return;
> > 
> >+	if (!(dev->msi_enabled)) {
> >+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
> >+		       pci_name(dev));
> >+		return;
> >+	}
> >+ 
> > 	pirq = dev->irq;
> > 	/* Restore dev->irq to its default pin-assertion vector */
> > 	dev->irq = msi_dev_entry->default_irq;
> 
> But shouldn't this happen before the CONFIG_XEN_PCIDEV_FRONTEND
> conditional block? This one also calls evtchn_map_pirq(..., 0), i.e. would
> also result in the storing of no_irq_chip.

However when irq_desc[irq]->chip set to no_irq_chip, if any device try to request
the @irq will failed and return -ENOSYS via request_irq()->setup_irq().

According to codes, only when CONFIG_XEN_PCIDEV_FRONTEND and !is_initial_xendomain(),
it will called evtchn_map_pirq(), meant only guest OS may call it, Dom0 will not.
But during pci_enable_msi(), it never set the flag(dev->msi_enabled), I'm not sure if
Guest OS will set it if enabled msi, any suggestion?

Thanks,
Joe


> 
> Jan
> 
> 

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jan Beulich]

>>> Joe Jin <joe.jin@oracle.com> 17.11.09 11:14 >>>
>On 2009-11-17 07:59, Jan Beulich wrote:
>> >>> Joe Jin <joe.jin@oracle.com> 17.11.09 01:19 >>>
>> >--- a/drivers/pci/msi-xen.c	Fri Oct 23 10:07:22 2009 +0100
>> >+++ b/drivers/pci/msi-xen.c	Tue Nov 17 08:16:42 2009 +0800
>> >@@ -673,6 +673,12 @@
>> > 	if (!pos)
>> > 		return;
>> > 
>> >+	if (!(dev->msi_enabled)) {
>> >+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
>> >+		       pci_name(dev));
>> >+		return;
>> >+	}
>> >+ 
>> > 	pirq = dev->irq;
>> > 	/* Restore dev->irq to its default pin-assertion vector */
>> > 	dev->irq = msi_dev_entry->default_irq;
>> 
>> But shouldn't this happen before the CONFIG_XEN_PCIDEV_FRONTEND
>> conditional block? This one also calls evtchn_map_pirq(..., 0), i.e. would
>> also result in the storing of no_irq_chip.
>
>However when irq_desc[irq]->chip set to no_irq_chip, if any device try to request
>the @irq will failed and return -ENOSYS via request_irq()->setup_irq().
>
>According to codes, only when CONFIG_XEN_PCIDEV_FRONTEND and !is_initial_xendomain(),
>it will called evtchn_map_pirq(), meant only guest OS may call it, Dom0 will not.
>But during pci_enable_msi(), it never set the flag(dev->msi_enabled), I'm not sure if
>Guest OS will set it if enabled msi, any suggestion?

Hmm, indeed - I'm not sure then. Clarification from the Intel guys having
originally written this code would be very desirable here; adding them to
Cc.

Jan

RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jiang, Yunhong]

Yes, we need keep the msi disable/enable information for frontend also, now it is only kept in backend side. It is a issue introduced by us from beginning.

I will cook a patch after I get an environment to test.

--jyh

Jan Beulich wrote:
>>>> Joe Jin <joe.jin@oracle.com> 17.11.09 11:14 >>>
>> On 2009-11-17 07:59, Jan Beulich wrote:
>>>>>> Joe Jin <joe.jin@oracle.com> 17.11.09 01:19 >>>
>>>> --- a/drivers/pci/msi-xen.c	Fri Oct 23 10:07:22 2009 +0100
>>>> +++ b/drivers/pci/msi-xen.c	Tue Nov 17 08:16:42 2009 +0800 @@
>>>> 	-673,6 +673,12 @@ if (!pos)
>>>> 		return;
>>>> 
>>>> +	if (!(dev->msi_enabled)) {
>>>> +		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n", +		 
>>>> pci_name(dev)); +		return;
>>>> +	}
>>>> +
>>>> 	pirq = dev->irq;
>>>> 	/* Restore dev->irq to its default pin-assertion vector */
>>>> 	dev->irq = msi_dev_entry->default_irq;
>>> 
>>> But shouldn't this happen before the CONFIG_XEN_PCIDEV_FRONTEND
>>> conditional block? This one also calls evtchn_map_pirq(..., 0),
>>> i.e. would also result in the storing of no_irq_chip.
>> 
>> However when irq_desc[irq]->chip set to no_irq_chip, if any device
>> try to request the @irq will failed and return -ENOSYS via
>> request_irq()->setup_irq(). 
>> 
>> According to codes, only when CONFIG_XEN_PCIDEV_FRONTEND and
>> !is_initial_xendomain(), it will called evtchn_map_pirq(), meant
>> only guest OS may call it, Dom0 will not. But during
>> pci_enable_msi(), it never set the 
> flag(dev->msi_enabled), I'm not sure if
>> Guest OS will set it if enabled msi, any suggestion?
> 
> Hmm, indeed - I'm not sure then. Clarification from the Intel guys
> having originally written this code would be very desirable here;
> adding them to Cc.
> 
> Jan

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

On 2009-11-18 14:23, Jiang, Yunhong wrote:
> Yes, we need keep the msi disable/enable information for frontend also, now it is only kept in backend side. It is a issue introduced by us from beginning.

Thanks for your information, patch may like below?


diff -r c5c40e80bd7d drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
+++ b/drivers/pci/msi-xen.c	Mon Nov 23 10:21:17 2009 +0800
@@ -618,6 +618,7 @@
 			return ret;
 
 		dev->irq = evtchn_map_pirq(-1, dev->irq);
+		dev->msi_enabled = 1;
 		msi_dev_entry->default_irq = temp;
 
 		return ret;
@@ -662,6 +663,11 @@
 
 #ifdef CONFIG_XEN_PCIDEV_FRONTEND
 	if (!is_initial_xendomain()) {
+		if (!(dev->msi_enabled)) {
+			printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+			       pci_name(dev));
+			return;
+		}
 		evtchn_map_pirq(dev->irq, 0);
 		pci_frontend_disable_msi(dev);
 		dev->irq = msi_dev_entry->default_irq;
@@ -673,6 +679,12 @@
 	if (!pos)
 		return;
 
+	if (!(dev->msi_enabled)) {
+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+		       pci_name(dev));
+		return;
+	}
+ 
 	pirq = dev->irq;
 	/* Restore dev->irq to its default pin-assertion vector */
 	dev->irq = msi_dev_entry->default_irq;

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
here are the patch, please review and comment:

When device driver unload, it may call pci_disable_msi(), if msi did not
enabled but do msi_unmap_pirq(), then later driver reload and without
msi, then will failed in request_irq() for irq_desc[irq]->chip valie is
no_irq_chip. So when did not enable msi during driver initializing, then
unloaded driver will not try to disable it.

How to reproduce it:
  At the server with QLogic 25xx, try to reload qla2xxx will hit it.


Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 msi-xen.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)


diff -r c5c40e80bd7d drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
+++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800
@@ -618,6 +618,7 @@
 			return ret;
 
 		dev->irq = evtchn_map_pirq(-1, dev->irq);
+		dev->msi_enabled = 1;
 		msi_dev_entry->default_irq = temp;
 
 		return ret;
@@ -662,9 +663,15 @@
 
 #ifdef CONFIG_XEN_PCIDEV_FRONTEND
 	if (!is_initial_xendomain()) {
+		if (!(dev->msi_enabled)) {
+			printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+			       pci_name(dev));
+			return;
+		}
 		evtchn_map_pirq(dev->irq, 0);
 		pci_frontend_disable_msi(dev);
 		dev->irq = msi_dev_entry->default_irq;
+		dev->msi_enabled = 0;
 		return;
 	}
 #endif
@@ -673,6 +680,12 @@
 	if (!pos)
 		return;
 
+	if (!(dev->msi_enabled)) {
+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+		       pci_name(dev));
+		return;
+	}
+ 
 	pirq = dev->irq;
 	/* Restore dev->irq to its default pin-assertion vector */
 	dev->irq = msi_dev_entry->default_irq;

RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jiang, Yunhong]

Joe, thanks for your patch very much. 
A small question is, can we check if (!(dev->msi_enabled))  only once in pci_disable_msi()? 

--jyh

Joe Jin wrote:
> Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
> here are the patch, please review and comment:
> 
> When device driver unload, it may call pci_disable_msi(), if
> msi did not
> enabled but do msi_unmap_pirq(), then later driver reload and without
> msi, then will failed in request_irq() for irq_desc[irq]->chip valie
> is no_irq_chip. So when did not enable msi during driver
> initializing, then
> unloaded driver will not try to disable it.
> 
> How to reproduce it:
>  At the server with QLogic 25xx, try to reload qla2xxx will hit it.
> 
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
> msi-xen.c |   13 +++++++++++++
> 1 file changed, 13 insertions(+)
> 
> 
> diff -r c5c40e80bd7d drivers/pci/msi-xen.c
> --- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
> +++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800 @@ -618,6
> 			+618,7 @@ return ret;
> 
> 		dev->irq = evtchn_map_pirq(-1, dev->irq);
> +		dev->msi_enabled = 1;
> 		msi_dev_entry->default_irq = temp;
> 
> 		return ret;
> @@ -662,9 +663,15 @@
> 
> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
> 	if (!is_initial_xendomain()) {
> +		if (!(dev->msi_enabled)) {
> +			printk(KERN_INFO "PCI: %s: Device did
> not enabled MSI.\n",
> +			       pci_name(dev));
> +			return;
> +		}
> 		evtchn_map_pirq(dev->irq, 0);
> 		pci_frontend_disable_msi(dev);
> 		dev->irq = msi_dev_entry->default_irq;
> +		dev->msi_enabled = 0;
> 		return;
> 	}
> #endif
> @@ -673,6 +680,12 @@
> 	if (!pos)
> 		return;
> 
> +	if (!(dev->msi_enabled)) {
> +		printk(KERN_INFO "PCI: %s: Device did not
> enabled MSI.\n",
> +		       pci_name(dev));
> +		return;
> +	}
> +
> 	pirq = dev->irq;
> 	/* Restore dev->irq to its default pin-assertion vector */
> 	dev->irq = msi_dev_entry->default_irq;

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Joe Jin]

On 2009-11-24 14:12, Jiang, Yunhong wrote:
> Joe, thanks for your patch very much. 
> A small question is, can we check if (!(dev->msi_enabled))  only once in pci_disable_msi()? 

Check once is OK, twice check made codes easy to understand(compare
pci_enable_msi()), and will not impact program's flow :)

Thanks,
Joe

> 
> --jyh
> 
> Joe Jin wrote:
> > Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
> > here are the patch, please review and comment:
> > 
> > When device driver unload, it may call pci_disable_msi(), if
> > msi did not
> > enabled but do msi_unmap_pirq(), then later driver reload and without
> > msi, then will failed in request_irq() for irq_desc[irq]->chip valie
> > is no_irq_chip. So when did not enable msi during driver
> > initializing, then
> > unloaded driver will not try to disable it.
> > 
> > How to reproduce it:
> >  At the server with QLogic 25xx, try to reload qla2xxx will hit it.
> > 
> > 
> > Signed-off-by: Joe Jin <joe.jin@oracle.com>
> > ---
> > msi-xen.c |   13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> > 
> > 
> > diff -r c5c40e80bd7d drivers/pci/msi-xen.c
> > --- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
> > +++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800 @@ -618,6
> > 			+618,7 @@ return ret;
> > 
> > 		dev->irq = evtchn_map_pirq(-1, dev->irq);
> > +		dev->msi_enabled = 1;
> > 		msi_dev_entry->default_irq = temp;
> > 
> > 		return ret;
> > @@ -662,9 +663,15 @@
> > 
> > #ifdef CONFIG_XEN_PCIDEV_FRONTEND
> > 	if (!is_initial_xendomain()) {
> > +		if (!(dev->msi_enabled)) {
> > +			printk(KERN_INFO "PCI: %s: Device did
> > not enabled MSI.\n",
> > +			       pci_name(dev));
> > +			return;
> > +		}
> > 		evtchn_map_pirq(dev->irq, 0);
> > 		pci_frontend_disable_msi(dev);
> > 		dev->irq = msi_dev_entry->default_irq;
> > +		dev->msi_enabled = 0;
> > 		return;
> > 	}
> > #endif
> > @@ -673,6 +680,12 @@
> > 	if (!pos)
> > 		return;
> > 
> > +	if (!(dev->msi_enabled)) {
> > +		printk(KERN_INFO "PCI: %s: Device did not
> > enabled MSI.\n",
> > +		       pci_name(dev));
> > +		return;
> > +	}
> > +
> > 	pirq = dev->irq;
> > 	/* Restore dev->irq to its default pin-assertion vector */
> > 	dev->irq = msi_dev_entry->default_irq;

RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jiang, Yunhong]

Got it. Thanks.

--jyh

Joe Jin wrote:
> On 2009-11-24 14:12, Jiang, Yunhong wrote:
>> Joe, thanks for your patch very much.
>> A small question is, can we check if (!(dev->msi_enabled))
> only once in pci_disable_msi()?
> 
> Check once is OK, twice check made codes easy to understand(compare
> pci_enable_msi()), and will not impact program's flow :)
> 
> Thanks,
> Joe
> 
>> 
>> --jyh
>> 
>> Joe Jin wrote:
>>> Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
>>> here are the patch, please review and comment:
>>> 
>>> When device driver unload, it may call pci_disable_msi(), if
>>> msi did not
>>> enabled but do msi_unmap_pirq(), then later driver reload and
>>> without msi, then will failed in request_irq() for
>>> irq_desc[irq]->chip valie is no_irq_chip. So when did not enable
>>> msi during driver initializing, then unloaded driver will not try
>>> to disable it. 
>>> 
>>> How to reproduce it:
>>>  At the server with QLogic 25xx, try to reload qla2xxx will hit it.
>>> 
>>> 
>>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>>> ---
>>> msi-xen.c |   13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>> 
>>> 
>>> diff -r c5c40e80bd7d drivers/pci/msi-xen.c
>>> --- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
>>> +++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800 @@
>>> -618,6 			+618,7 @@ return ret; 
>>> 
>>> 		dev->irq = evtchn_map_pirq(-1, dev->irq);
>>> +		dev->msi_enabled = 1;
>>> 		msi_dev_entry->default_irq = temp;
>>> 
>>> 		return ret;
>>> @@ -662,9 +663,15 @@
>>> 
>>> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
>>> 	if (!is_initial_xendomain()) {
>>> +		if (!(dev->msi_enabled)) {
>>> +			printk(KERN_INFO "PCI: %s: Device did
>>> not enabled MSI.\n",
>>> +			       pci_name(dev));
>>> +			return;
>>> +		}
>>> 		evtchn_map_pirq(dev->irq, 0);
>>> 		pci_frontend_disable_msi(dev);
>>> 		dev->irq = msi_dev_entry->default_irq;
>>> +		dev->msi_enabled = 0;
>>> 		return;
>>> 	}
>>> #endif
>>> @@ -673,6 +680,12 @@
>>> 	if (!pos)
>>> 		return;
>>> 
>>> +	if (!(dev->msi_enabled)) {
>>> +		printk(KERN_INFO "PCI: %s: Device did not
>>> enabled MSI.\n",
>>> +		       pci_name(dev));
>>> +		return;
>>> +	}
>>> +
>>> 	pirq = dev->irq;
>>> 	/* Restore dev->irq to its default pin-assertion vector */
>>> 	dev->irq = msi_dev_entry->default_irq;

Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jan Beulich]

Wouldn't we need the same also for MSI-X?

Jan

>>> Joe Jin <joe.jin@oracle.com> 24.11.09 03:02 >>>
Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
here are the patch, please review and comment:

When device driver unload, it may call pci_disable_msi(), if msi did not
enabled but do msi_unmap_pirq(), then later driver reload and without
msi, then will failed in request_irq() for irq_desc[irq]->chip valie is
no_irq_chip. So when did not enable msi during driver initializing, then
unloaded driver will not try to disable it.

How to reproduce it:
  At the server with QLogic 25xx, try to reload qla2xxx will hit it.


Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 msi-xen.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)


diff -r c5c40e80bd7d drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
+++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800
@@ -618,6 +618,7 @@
 			return ret;
 
 		dev->irq = evtchn_map_pirq(-1, dev->irq);
+		dev->msi_enabled = 1;
 		msi_dev_entry->default_irq = temp;
 
 		return ret;
@@ -662,9 +663,15 @@
 
 #ifdef CONFIG_XEN_PCIDEV_FRONTEND
 	if (!is_initial_xendomain()) {
+		if (!(dev->msi_enabled)) {
+			printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+			       pci_name(dev));
+			return;
+		}
 		evtchn_map_pirq(dev->irq, 0);
 		pci_frontend_disable_msi(dev);
 		dev->irq = msi_dev_entry->default_irq;
+		dev->msi_enabled = 0;
 		return;
 	}
 #endif
@@ -673,6 +680,12 @@
 	if (!pos)
 		return;
 
+	if (!(dev->msi_enabled)) {
+		printk(KERN_INFO "PCI: %s: Device did not enabled MSI.\n",
+		       pci_name(dev));
+		return;
+	}
+ 
 	pirq = dev->irq;
 	/* Restore dev->irq to its default pin-assertion vector */
 	dev->irq = msi_dev_entry->default_irq;

RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jiang, Yunhong]

I also noticed following code  have no lock protection in the list_for_each_entry_safe(pirq_entry, tmp, &msi_dev_entry->pirq_list_head, list) and needs a fix.

-- jyh

> void pci_disable_msix(struct pci_dev* dev)
> {
>     int pos;
>     u16 control;
> 
>     if (!pci_msi_enable)
>         return;
>     if (!dev)
>         return;
> 
> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
>     if (!is_initial_xendomain()) {
>         struct msi_dev_list *msi_dev_entry;
>         struct msi_pirq_entry *pirq_entry, *tmp;
> 
>         pci_frontend_disable_msix(dev);
> 
>         msi_dev_entry = get_msi_dev_pirq_list(dev);
>         list_for_each_entry_safe(pirq_entry, tmp,
>                                  &msi_dev_entry->pirq_list_head,
>             list) { evtchn_map_pirq(pirq_entry->pirq, 0);
>             list_del(&pirq_entry->list);
>             kfree(pirq_entry);
>         }
> 
>         dev->irq = msi_dev_entry->default_irq;
>         return;
>     }
> #endif


Jan Beulich wrote:
> Wouldn't we need the same also for MSI-X?
> 
> Jan
> 
>>>> Joe Jin <joe.jin@oracle.com> 24.11.09 03:02 >>>
> Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
> here are the patch, please review and comment:
> 
> When device driver unload, it may call pci_disable_msi(), if
> msi did not
> enabled but do msi_unmap_pirq(), then later driver reload and without
> msi, then will failed in request_irq() for irq_desc[irq]->chip valie
> is no_irq_chip. So when did not enable msi during driver
> initializing, then
> unloaded driver will not try to disable it.
> 
> How to reproduce it:
>  At the server with QLogic 25xx, try to reload qla2xxx will hit it.
> 
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
> msi-xen.c |   13 +++++++++++++
> 1 file changed, 13 insertions(+)
> 
> 
> diff -r c5c40e80bd7d drivers/pci/msi-xen.c
> --- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
> +++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800 @@ -618,6
> 			+618,7 @@ return ret;
> 
> 		dev->irq = evtchn_map_pirq(-1, dev->irq);
> +		dev->msi_enabled = 1;
> 		msi_dev_entry->default_irq = temp;
> 
> 		return ret;
> @@ -662,9 +663,15 @@
> 
> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
> 	if (!is_initial_xendomain()) {
> +		if (!(dev->msi_enabled)) {
> +			printk(KERN_INFO "PCI: %s: Device did
> not enabled MSI.\n",
> +			       pci_name(dev));
> +			return;
> +		}
> 		evtchn_map_pirq(dev->irq, 0);
> 		pci_frontend_disable_msi(dev);
> 		dev->irq = msi_dev_entry->default_irq;
> +		dev->msi_enabled = 0;
> 		return;
> 	}
> #endif
> @@ -673,6 +680,12 @@
> 	if (!pos)
> 		return;
> 
> +	if (!(dev->msi_enabled)) {
> +		printk(KERN_INFO "PCI: %s: Device did not
> enabled MSI.\n",
> +		       pci_name(dev));
> +		return;
> +	}
> +
> 	pirq = dev->irq;
> 	/* Restore dev->irq to its default pin-assertion vector */
> 	dev->irq = msi_dev_entry->default_irq;

RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi

[Jiang, Yunhong]

Seems this list handling need a clean-up, sometimes it is protected, sometimes seems not. I will check it again because I didn't touch the this code for a long time.

--jyh

xen-devel-bounces@lists.xensource.com wrote:
> I also noticed following code  have no lock protection in the
> list_for_each_entry_safe(pirq_entry, tmp,
> &msi_dev_entry->pirq_list_head, list) and needs a fix.
> 
> -- jyh
> 
>> void pci_disable_msix(struct pci_dev* dev)
>> {
>>     int pos;
>>     u16 control;
>> 
>>     if (!pci_msi_enable)
>>         return;
>>     if (!dev)
>>         return;
>> 
>> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
>>     if (!is_initial_xendomain()) {
>>         struct msi_dev_list *msi_dev_entry;
>>         struct msi_pirq_entry *pirq_entry, *tmp;
>> 
>>         pci_frontend_disable_msix(dev);
>> 
>>         msi_dev_entry = get_msi_dev_pirq_list(dev);
>>         list_for_each_entry_safe(pirq_entry, tmp,
>>                                  &msi_dev_entry->pirq_list_head,
>>             list) { evtchn_map_pirq(pirq_entry->pirq, 0);
>>             list_del(&pirq_entry->list);
>>             kfree(pirq_entry);
>>         }
>> 
>>         dev->irq = msi_dev_entry->default_irq;
>>         return;
>>     }
>> #endif
> 
> 
> Jan Beulich wrote:
>> Wouldn't we need the same also for MSI-X?
>> 
>> Jan
>> 
>>>>> Joe Jin <joe.jin@oracle.com> 24.11.09 03:02 >>>
>> Sorry I lost to set @dev->msi_enabled to false in pci_disable_msi,
>> here are the patch, please review and comment:
>> 
>> When device driver unload, it may call pci_disable_msi(), if
>> msi did not
>> enabled but do msi_unmap_pirq(), then later driver reload and without
>> msi, then will failed in request_irq() for irq_desc[irq]->chip valie
>> is no_irq_chip. So when did not enable msi during driver
>> initializing, then unloaded driver will not try to disable it.
>> 
>> How to reproduce it:
>>  At the server with QLogic 25xx, try to reload qla2xxx will hit it.
>> 
>> 
>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>> ---
>> msi-xen.c |   13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>> 
>> 
>> diff -r c5c40e80bd7d drivers/pci/msi-xen.c
>> --- a/drivers/pci/msi-xen.c	Fri Nov 13 22:01:54 2009 +0000
>> +++ b/drivers/pci/msi-xen.c	Tue Nov 24 09:56:52 2009 +0800 @@ -618,6
>> +618,7 @@ return ret; 
>> 
>> 		dev->irq = evtchn_map_pirq(-1, dev->irq);
>> +		dev->msi_enabled = 1;
>> 		msi_dev_entry->default_irq = temp;
>> 
>> 		return ret;
>> @@ -662,9 +663,15 @@
>> 
>> #ifdef CONFIG_XEN_PCIDEV_FRONTEND
>> 	if (!is_initial_xendomain()) {
>> +		if (!(dev->msi_enabled)) {
>> +			printk(KERN_INFO "PCI: %s: Device did
>> not enabled MSI.\n",
>> +			       pci_name(dev));
>> +			return;
>> +		}
>> 		evtchn_map_pirq(dev->irq, 0);
>> 		pci_frontend_disable_msi(dev);
>> 		dev->irq = msi_dev_entry->default_irq;
>> +		dev->msi_enabled = 0;
>> 		return;
>> 	}
>> #endif
>> @@ -673,6 +680,12 @@
>> 	if (!pos)
>> 		return;
>> 
>> +	if (!(dev->msi_enabled)) {
>> +		printk(KERN_INFO "PCI: %s: Device did not
>> enabled MSI.\n",
>> +		       pci_name(dev));
>> +		return;
>> +	}
>> +
>> 	pirq = dev->irq;
>> 	/* Restore dev->irq to its default pin-assertion vector */
>> 	dev->irq = msi_dev_entry->default_irq;
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel