Joe Jin
2009-Nov-16 12:00 UTC
[Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2009-Nov-16 15:15 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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 withoutWhere 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, thenWon''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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2009-Nov-16 15:26 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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 :-( _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2009-Nov-17 00:19 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Nov-17 07:59 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2009-Nov-17 10:14 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Nov-17 11:21 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2009-Nov-18 06:23 UTC
RE: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2009-Nov-23 02:24 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2009-Nov-24 02:02 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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
Jiang, Yunhong
2009-Nov-24 06:12 UTC
RE: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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;_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2009-Nov-24 06:42 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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;_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2009-Nov-24 07:06 UTC
RE: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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;_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Nov-26 09:14 UTC
Re: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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
Jiang, Yunhong
2009-Nov-26 10:03 UTC
RE: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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; > } > #endifJan 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
Jiang, Yunhong
2009-Nov-26 10:14 UTC
RE: [Xen-devel] [PATCH] Dont call msi_unmap_pirq() if did not enabled msi
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel