* [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
@ 2009-11-16 12:00 Joe Jin
2009-11-16 15:15 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 16+ messages in thread
From: Joe Jin @ 2009-11-16 12:00 UTC (permalink / raw)
To: Keir Fraser, Hackel,Kurt, Jerry Yuanjiang Ou, greg.marsden
Cc: Xen-devel, 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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-16 12:00 [PATCH] Dont call msi_unmap_pirq() if did not enabled msi Joe Jin
@ 2009-11-16 15:15 ` Konrad Rzeszutek Wilk
2009-11-16 15:26 ` Konrad Rzeszutek Wilk
2009-11-17 0:19 ` Joe Jin
0 siblings, 2 replies; 16+ messages in thread
From: Konrad Rzeszutek Wilk @ 2009-11-16 15:15 UTC (permalink / raw)
To: Joe Jin
Cc: Xen-devel, Hackel, Kurt, Jerry Yuanjiang Ou, greg.marsden, Keir Fraser
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-16 15:15 ` Konrad Rzeszutek Wilk
@ 2009-11-16 15:26 ` Konrad Rzeszutek Wilk
2009-11-17 0:19 ` Joe Jin
1 sibling, 0 replies; 16+ messages in thread
From: Konrad Rzeszutek Wilk @ 2009-11-16 15:26 UTC (permalink / raw)
To: Joe Jin
Cc: Hackel, Kurt, Jerry Yuanjiang Ou, Xen-devel, Keir Fraser, greg.marsden
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 :-(
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-16 15:15 ` Konrad Rzeszutek Wilk
2009-11-16 15:26 ` Konrad Rzeszutek Wilk
@ 2009-11-17 0:19 ` Joe Jin
2009-11-17 7:59 ` Jan Beulich
1 sibling, 1 reply; 16+ messages in thread
From: Joe Jin @ 2009-11-17 0:19 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Xen-devel, Hackel, Kurt, greg.marsden, Joe Jin,
Jerry Yuanjiang Ou, Keir Fraser
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-17 0:19 ` Joe Jin
@ 2009-11-17 7:59 ` Jan Beulich
2009-11-17 10:14 ` Joe Jin
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2009-11-17 7:59 UTC (permalink / raw)
To: Joe Jin, Konrad Rzeszutek Wilk
Cc: Kurt Hackel, Jerry Yuanjiang Ou, Xen-devel, Keir Fraser, greg.marsden
>>> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-17 7:59 ` Jan Beulich
@ 2009-11-17 10:14 ` Joe Jin
2009-11-17 11:21 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Joe Jin @ 2009-11-17 10:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, greg.marsden,
Joe Jin, Jerry Yuanjiang Ou, Keir Fraser
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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-17 10:14 ` Joe Jin
@ 2009-11-17 11:21 ` Jan Beulich
2009-11-18 6:23 ` Jiang, Yunhong
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2009-11-17 11:21 UTC (permalink / raw)
To: Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, Yunhong Jiang,
Haitao Shan, greg.marsden, Jerry Yuanjiang Ou, Keir Fraser
>>> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-17 11:21 ` Jan Beulich
@ 2009-11-18 6:23 ` Jiang, Yunhong
2009-11-23 2:24 ` Joe Jin
0 siblings, 1 reply; 16+ messages in thread
From: Jiang, Yunhong @ 2009-11-18 6:23 UTC (permalink / raw)
To: Jan Beulich, Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, greg.marsden,
Shan, Haitao, Jerry, Yuanjiang Ou, Keir Fraser
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-18 6:23 ` Jiang, Yunhong
@ 2009-11-23 2:24 ` Joe Jin
2009-11-24 2:02 ` Joe Jin
0 siblings, 1 reply; 16+ messages in thread
From: Joe Jin @ 2009-11-23 2:24 UTC (permalink / raw)
To: Jiang, Yunhong
Cc: Xen-devel, Konrad Rzeszutek Wilk, Joe Jin, greg.marsden, Shan,
Haitao, Jan Beulich, Jerry Yuanjiang Ou, Keir Fraser,
Kurt Hackel
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-23 2:24 ` Joe Jin
@ 2009-11-24 2:02 ` Joe Jin
2009-11-24 6:12 ` Jiang, Yunhong
2009-11-26 9:14 ` Jan Beulich
0 siblings, 2 replies; 16+ messages in thread
From: Joe Jin @ 2009-11-24 2:02 UTC (permalink / raw)
To: Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, Jiang, Yunhong,
Shan, Haitao, Jan Beulich, Jerry Yuanjiang Ou, Keir Fraser,
greg.marsden
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-24 2:02 ` Joe Jin
@ 2009-11-24 6:12 ` Jiang, Yunhong
2009-11-24 6:42 ` Joe Jin
2009-11-26 9:14 ` Jan Beulich
1 sibling, 1 reply; 16+ messages in thread
From: Jiang, Yunhong @ 2009-11-24 6:12 UTC (permalink / raw)
To: Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, greg.marsden,
Shan, Haitao, Beulich, Jan, Jerry Yuanjiang Ou, Keir Fraser
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-24 6:12 ` Jiang, Yunhong
@ 2009-11-24 6:42 ` Joe Jin
2009-11-24 7:06 ` Jiang, Yunhong
0 siblings, 1 reply; 16+ messages in thread
From: Joe Jin @ 2009-11-24 6:42 UTC (permalink / raw)
To: Jiang, Yunhong
Cc: Xen-devel, Konrad Rzeszutek Wilk, Joe Jin, greg.marsden, Shan,
Haitao, Jan Beulich, Jerry Yuanjiang Ou, Keir Fraser,
Kurt Hackel
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-24 6:42 ` Joe Jin
@ 2009-11-24 7:06 ` Jiang, Yunhong
0 siblings, 0 replies; 16+ messages in thread
From: Jiang, Yunhong @ 2009-11-24 7:06 UTC (permalink / raw)
To: Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, greg.marsden,
Shan, Haitao, Beulich, Jan, Jerry Yuanjiang Ou, Keir Fraser
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-24 2:02 ` Joe Jin
2009-11-24 6:12 ` Jiang, Yunhong
@ 2009-11-26 9:14 ` Jan Beulich
2009-11-26 10:03 ` Jiang, Yunhong
1 sibling, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2009-11-26 9:14 UTC (permalink / raw)
To: Yunhong Jiang, Joe Jin
Cc: Xen-devel, Konrad Rzeszutek Wilk, Kurt Hackel, greg.marsden,
Haitao Shan, Jerry Yuanjiang Ou, Keir Fraser
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-26 9:14 ` Jan Beulich
@ 2009-11-26 10:03 ` Jiang, Yunhong
2009-11-26 10:14 ` Jiang, Yunhong
0 siblings, 1 reply; 16+ messages in thread
From: Jiang, Yunhong @ 2009-11-26 10:03 UTC (permalink / raw)
To: Jan Beulich, Joe Jin; +Cc: xen-devel, Keir Fraser
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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
2009-11-26 10:03 ` Jiang, Yunhong
@ 2009-11-26 10:14 ` Jiang, Yunhong
0 siblings, 0 replies; 16+ messages in thread
From: Jiang, Yunhong @ 2009-11-26 10:14 UTC (permalink / raw)
To: Jiang, Yunhong, Jan Beulich, Joe Jin; +Cc: Keir, xen-devel, Fraser
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2009-11-26 10:14 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-16 12:00 [PATCH] Dont call msi_unmap_pirq() if did not enabled msi Joe Jin
2009-11-16 15:15 ` Konrad Rzeszutek Wilk
2009-11-16 15:26 ` Konrad Rzeszutek Wilk
2009-11-17 0:19 ` Joe Jin
2009-11-17 7:59 ` Jan Beulich
2009-11-17 10:14 ` Joe Jin
2009-11-17 11:21 ` Jan Beulich
2009-11-18 6:23 ` Jiang, Yunhong
2009-11-23 2:24 ` Joe Jin
2009-11-24 2:02 ` Joe Jin
2009-11-24 6:12 ` Jiang, Yunhong
2009-11-24 6:42 ` Joe Jin
2009-11-24 7:06 ` Jiang, Yunhong
2009-11-26 9:14 ` Jan Beulich
2009-11-26 10:03 ` Jiang, Yunhong
2009-11-26 10:14 ` Jiang, Yunhong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.