| Message ID | 1440009922-30248-1-git-send-email-gpiccoli@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2015-08-19 at 15:45 -0300, Guilherme G. Piccoli wrote: > Thanks very much for your suggestions Michael. I agree with them all, > so I'm sending the patch v2 (see below). > > About the relevant mailing lists, I already sent to the linux-pci and > already cc'ed Bjorn Helgaas - the problem is that I made a mistake and > sent 2 different emails using git send-email. I'm really sorry about > this. > > Now I'm trying to correct my mistake sending this message > simultaneously to both lists (and to a bunch of cc's) using two > Message-IDs in my reply. Hope it works... OK. In future you should send a reply like the above to my mail, and then separately send the new patch series. My preference is that the new series is not a reply to anything, though some other maintainers may disagree on that point. The other question, which I neglected to ask yesterday, is what is the symptom of the bug? ie. does the system fail to boot or otherwise crash etc.? > Changes since v2: This is changes *in* v2, or since v1. And it doesn't go here, it goes below ... > * Made commit message more clear > * Added "Fixes" line > * Improved commit reference by using 12 first chars of SHA > > >8----------8< > > Commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel > doesn't support MSI") changed the location of the code that disables > MSI/MSI-X interrupts at PCI probe time in devices that have this flag > set. It moved the code from pci_msi_init_pci_dev() to a new function > named pci_msi_setup_pci_dev(), called by pci_setup_device(). > > The pseries PCI probing code does not call pci_setup_device(), so since > the aforementioned commit the function pci_msi_setup_pci_dev() is not > called and MSI/MSI-X interrupts are left enabled, which is a bug. To fix > this, the pseries PCI probe should manually call pci_msi_setup_pci_dev(), > so this patch makes it non-static. > > Fixes: 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel > doesn't support MSI") > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- Here. Or anywhere after the first '---', which means the version commentary is discarded in the final commit. > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cefd636..520c5b6 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev) > > #define LEGACY_IO_RESOURCE (IORESOURCE_IO | IORESOURCE_PCI_FIXED) > > -static void pci_msi_setup_pci_dev(struct pci_dev *dev) > +void pci_msi_setup_pci_dev(struct pci_dev *dev) > { > /* > * Disable the MSI hardware to avoid screaming interrupts > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 8a0321a..860c751 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1202,6 +1202,7 @@ struct msix_entry { > u16 entry; /* driver uses to specify entry, OS writes */ > }; > > +void pci_msi_setup_pci_dev(struct pci_dev *dev); > > #ifdef CONFIG_PCI_MSI > int pci_msi_vec_count(struct pci_dev *dev); cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/19/2015 10:02 PM, Michael Ellerman wrote: > In future you should send a reply like the above to my mail, and then > separately send the new patch series. My preference is that the new series is > not a reply to anything, though some other maintainers may disagree on that > point. OK, sure. I can send new patch series as new messages instead of replies to the same thread. > The other question, which I neglected to ask yesterday, is what is the symptom > of the bug? ie. does the system fail to boot or otherwise crash etc.? This is briefly explained on cover-letter, but I can elaborate a bit more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when I tried the mainline kernel, the driver wasn't able to enable MSI-X capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen and the driver can use MSI-X interrupts. So, I figured that something was wrong and found the problem described on the patches. I tried the proposed solution (calling manually the function that is not reachable anymore) and it works. Regarding the bnx2x driver, below are two dmesg outputs: 1) With kernel 4.2-rc7 bnx2x 0000:01:00.0: no msix capability found 2) With kernel 4.1 bnx2x 0000:01:00.0: msix capability found bnx2x 0000:01:00.0 eth2: using MSI-X IRQs: sp 24 fp[0] 26 ... fp[7] 33 > This is changes *in* v2, or since v1. My bad, sorry. > Or anywhere after the first '---', which means the version commentary is > discarded in the final commit. I used scissors, but there's no problem in stop using it in this list. Thanks for the suggestion. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-08-20 at 16:10 -0300, Guilherme G. Piccoli wrote: > On 08/19/2015 10:02 PM, Michael Ellerman wrote: > > In future you should send a reply like the above to my mail, and then > > separately send the new patch series. My preference is that the new series is > > not a reply to anything, though some other maintainers may disagree on that > > point. > > OK, sure. I can send new patch series as new messages instead of replies > to the same thread. Thanks. > > The other question, which I neglected to ask yesterday, is what is the symptom > > of the bug? ie. does the system fail to boot or otherwise crash etc.? > > This is briefly explained on cover-letter, but I can elaborate a bit Sure, but the cover-letter is not committed, so the commit change logs need to be self describing. > more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when > I tried the mainline kernel, the driver wasn't able to enable MSI-X > capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen > and the driver can use MSI-X interrupts. > > So, I figured that something was wrong and found the problem described > on the patches. I tried the proposed solution (calling manually the > function that is not reachable anymore) and it works. > > Regarding the bnx2x driver, below are two dmesg outputs: > > 1) With kernel 4.2-rc7 > bnx2x 0000:01:00.0: no msix capability found OK. This is because the initialisation of dev->msix_cap was lost due to commit 1851617cd2da. > 2) With kernel 4.1 > bnx2x 0000:01:00.0: msix capability found > bnx2x 0000:01:00.0 eth2: using MSI-X IRQs: sp 24 fp[0] 26 ... fp[7] 33 OK. And I assume with these patches you see the above output again. > > This is changes *in* v2, or since v1. > > My bad, sorry. > > > Or anywhere after the first '---', which means the version commentary is > > discarded in the final commit. > > I used scissors, but there's no problem in stop using it in this list. Thanks, but my scripts don't grok scissors. So I prefer the commentary after the '---'. cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/24/2015 04:37 AM, Michael Ellerman wrote: >> more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when >> I tried the mainline kernel, the driver wasn't able to enable MSI-X >> capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen >> and the driver can use MSI-X interrupts. >> >> So, I figured that something was wrong and found the problem described >> on the patches. I tried the proposed solution (calling manually the >> function that is not reachable anymore) and it works. >> >> Regarding the bnx2x driver, below are two dmesg outputs: >> >> 1) With kernel 4.2-rc7 >> bnx2x 0000:01:00.0: no msix capability found > > OK. This is because the initialisation of dev->msix_cap was lost due to commit > 1851617cd2da. > >> 2) With kernel 4.1 >> bnx2x 0000:01:00.0: msix capability found >> bnx2x 0000:01:00.0 eth2: using MSI-X IRQs: sp 24 fp[0] 26 ... fp[7] 33 > > OK. And I assume with these patches you see the above output again. Exactly. With the proposed patches applied, dev->msix_cap is initialized normally, so the driver is able to do its job as usual. >>> Or anywhere after the first '---', which means the version commentary is >>> discarded in the final commit. >> >> I used scissors, but there's no problem in stop using it in this list. > > Thanks, but my scripts don't grok scissors. So I prefer the commentary after > the '---'. Thanks for the info. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cefd636..520c5b6 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev) #define LEGACY_IO_RESOURCE (IORESOURCE_IO | IORESOURCE_PCI_FIXED) -static void pci_msi_setup_pci_dev(struct pci_dev *dev) +void pci_msi_setup_pci_dev(struct pci_dev *dev) { /* * Disable the MSI hardware to avoid screaming interrupts diff --git a/include/linux/pci.h b/include/linux/pci.h index 8a0321a..860c751 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1202,6 +1202,7 @@ struct msix_entry { u16 entry; /* driver uses to specify entry, OS writes */ }; +void pci_msi_setup_pci_dev(struct pci_dev *dev); #ifdef CONFIG_PCI_MSI int pci_msi_vec_count(struct pci_dev *dev);