* [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers @ 2017-10-11 10:52 Dongdong Liu 2017-10-11 10:52 ` [PATCH V4 1/2] " Dongdong Liu ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Dongdong Liu @ 2017-10-11 10:52 UTC (permalink / raw) To: helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm, Dongdong Liu This patchset is to optimize the interrupt vector allocation strategy in pcie_port_enable_irq_vec() and add #defines for the AER and DPC Interrupt Message Number masks. v3->v4: - Optimize the interrupt vector allocation strategy in pcie_port_enable_irq_vec(). - add #defines for the AER and DPC Interrupt Message Number masks. v2->v3: - Use a 12-character SHA1 commit id. - Rebase on v4.14-rc4. v1->v2: - Fix comments on PATCH v1. - Simplify implementation. Dongdong Liu (2): PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++--------------------------- include/uapi/linux/pci_regs.h | 2 ++ 2 files changed, 18 insertions(+), 33 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-11 10:52 [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers Dongdong Liu @ 2017-10-11 10:52 ` Dongdong Liu 2017-10-11 11:30 ` Bjorn Helgaas 2017-10-17 20:25 ` Bjorn Helgaas 2017-10-11 10:52 ` [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks Dongdong Liu 2017-10-16 12:09 ` Dongdong Liu 2 siblings, 2 replies; 15+ messages in thread From: Dongdong Liu @ 2017-10-11 10:52 UTC (permalink / raw) To: helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm, Dongdong Liu After removing and adding back the PCI root port device, we see the PCIe port service drivers request irq failed. pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 aer: probe of 0000:00:00.0:pcie002 failed with error -22 pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 dpc: probe of 0000:00:00.0:pcie010 failed with error -22 The current code basically does this: - allocate 32 vectors - figure out vector used by PME and hotplug - figure out vector used by AER - figure out vector used by DPC - free the 32 vectors we allocated - allocate only as many vectors as we need but it is broken as calling pci_free_irq_vectors() invalidates the IRQ numbers returned before by pci_irq_vectors(); The hardware works: - PME and hotplug use the Interrupt Message Number from the PCIe Capability register. - AER uses the AER Interrupt Message Number from the AER Root Error Status register. - DPC uses the DPC Interrupt Message Number from the DPC Capability register. - FRS (not supported by Linux yet) uses the FRS Interrupt Message Number from the FRS Queuing Capability register. - That's a total of 4 possible MSI/MSI-X vectors used for PME, hotplug, AER, DPC, and FRS, so there's no point in trying to allocate more than 4 vectors (we currently start with 32). - All these Interrupt Message Numbers are read-only to software but are updated by hardware when software writes the Multiple Message Enable field of the MSI Message Control register. Thanks for pointing this out; I didn't realize this before. - Therefore, we must read the Interrupt Message Numbers *after* we write Multiple Message Enable. Since we can use at most 4 vectors, this patch is to optimize the interrupt vector allocation strategy in pcie_port_enable_irq_vec(). First figure out how many vectors we need,allocate them, and then fill in the slots in the irqs[] table. Cc: <stable@vger.kernel.org> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") Suggested-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> --- drivers/pci/pcie/portdrv_core.c | 45 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 313a21d..9692379 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -56,14 +56,16 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) { int nr_entries, entry, nvec = 0; - /* - * Allocate as many entries as the port wants, so that we can check - * which of them will be useful. Moreover, if nr_entries is correctly - * equal to the number of entries this port actually uses, we'll happily - * go through without any tricks. - */ - nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSI_ENTRIES, - PCI_IRQ_MSIX | PCI_IRQ_MSI); + if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) + nvec++; + if (mask & PCIE_PORT_SERVICE_AER) + nvec++; + if (mask & PCIE_PORT_SERVICE_DPC) + nvec++; + + /* Allocate as many vectors as the we need */ + nr_entries = pci_alloc_irq_vectors(dev, 1, nvec, + PCI_IRQ_MSIX | PCI_IRQ_MSI); if (nr_entries < 0) return nr_entries; @@ -90,10 +92,10 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) if (entry >= nr_entries) goto out_free_irqs; - irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pci_irq_vector(dev, entry); - irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pci_irq_vector(dev, entry); - - nvec = max(nvec, entry + 1); + if (mask & PCIE_PORT_SERVICE_PME) + irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pci_irq_vector(dev, entry); + if (mask & PCIE_PORT_SERVICE_HP) + irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pci_irq_vector(dev, entry); } if (mask & PCIE_PORT_SERVICE_AER) { @@ -120,7 +122,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pci_irq_vector(dev, entry); - nvec = max(nvec, entry + 1); } if (mask & PCIE_PORT_SERVICE_DPC) { @@ -146,24 +147,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) goto out_free_irqs; irqs[PCIE_PORT_SERVICE_DPC_SHIFT] = pci_irq_vector(dev, entry); - - nvec = max(nvec, entry + 1); - } - - /* - * If nvec is equal to the allocated number of entries, we can just use - * what we have. Otherwise, the port has some extra entries not for the - * services we know and we need to work around that. - */ - if (nvec != nr_entries) { - /* Drop the temporary MSI-X setup */ - pci_free_irq_vectors(dev); - - /* Now allocate the MSI-X vectors for real */ - nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec, - PCI_IRQ_MSIX | PCI_IRQ_MSI); - if (nr_entries < 0) - return nr_entries; } return 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-11 10:52 ` [PATCH V4 1/2] " Dongdong Liu @ 2017-10-11 11:30 ` Bjorn Helgaas 2017-10-12 2:40 ` Dongdong Liu 2017-10-17 20:25 ` Bjorn Helgaas 1 sibling, 1 reply; 15+ messages in thread From: Bjorn Helgaas @ 2017-10-11 11:30 UTC (permalink / raw) To: Dongdong Liu Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: > After removing and adding back the PCI root port device, > we see the PCIe port service drivers request irq failed. > pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > aer: probe of 0000:00:00.0:pcie002 failed with error -22 > pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- > PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > > The current code basically does this: > > - allocate 32 vectors > - figure out vector used by PME and hotplug > - figure out vector used by AER > - figure out vector used by DPC > - free the 32 vectors we allocated > - allocate only as many vectors as we need > > but it is broken as calling pci_free_irq_vectors() > invalidates the IRQ numbers returned before by pci_irq_vectors(); > > The hardware works: > - PME and hotplug use the Interrupt Message Number from the PCIe > Capability register. > > - AER uses the AER Interrupt Message Number from the AER Root Error > Status register. > > - DPC uses the DPC Interrupt Message Number from the DPC Capability > register. > > - FRS (not supported by Linux yet) uses the FRS Interrupt Message > Number from the FRS Queuing Capability register. > > - That's a total of 4 possible MSI/MSI-X vectors used for PME, > hotplug, AER, DPC, and FRS, so there's no point in trying to > allocate more than 4 vectors (we currently start with 32). You pointed out that there actually may be some benefit to allocating more than 4 vectors because the hardware may not distribute the Interrupt Message Numbers across the allocated vectors, e.g., you have a system with HP/PME - message number 0 AER - message number 2 DPC - message number 6 In that case, if we allocate 8 vectors, there's probably no sharing (other than HP/PME), but if we allocate 4 vectors, the hardware might put DPC on message number 0 or 2. Out of curiosity, what does happen on this system when we only allocate 4 vectors? Does DPC end up sharing a vector with PME or AER? Or is it smart enough to put DPC on message number 1 or 3? Bjorn ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-11 11:30 ` Bjorn Helgaas @ 2017-10-12 2:40 ` Dongdong Liu 0 siblings, 0 replies; 15+ messages in thread From: Dongdong Liu @ 2017-10-12 2:40 UTC (permalink / raw) To: Bjorn Helgaas Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm 在 2017/10/11 19:30, Bjorn Helgaas 写道: > On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: >> After removing and adding back the PCI root port device, >> we see the PCIe port service drivers request irq failed. >> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 >> aer: probe of 0000:00:00.0:pcie002 failed with error -22 >> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- >> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ >> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller >> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) >> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 >> dpc: probe of 0000:00:00.0:pcie010 failed with error -22 >> >> The current code basically does this: >> >> - allocate 32 vectors >> - figure out vector used by PME and hotplug >> - figure out vector used by AER >> - figure out vector used by DPC >> - free the 32 vectors we allocated >> - allocate only as many vectors as we need >> >> but it is broken as calling pci_free_irq_vectors() >> invalidates the IRQ numbers returned before by pci_irq_vectors(); >> >> The hardware works: >> - PME and hotplug use the Interrupt Message Number from the PCIe >> Capability register. >> >> - AER uses the AER Interrupt Message Number from the AER Root Error >> Status register. >> >> - DPC uses the DPC Interrupt Message Number from the DPC Capability >> register. >> >> - FRS (not supported by Linux yet) uses the FRS Interrupt Message >> Number from the FRS Queuing Capability register. >> >> - That's a total of 4 possible MSI/MSI-X vectors used for PME, >> hotplug, AER, DPC, and FRS, so there's no point in trying to >> allocate more than 4 vectors (we currently start with 32). > > You pointed out that there actually may be some benefit to allocating > more than 4 vectors because the hardware may not distribute the > Interrupt Message Numbers across the allocated vectors, e.g., you have > a system with > > HP/PME - message number 0 > AER - message number 2 > DPC - message number 6 > > In that case, if we allocate 8 vectors, there's probably no sharing > (other than HP/PME), but if we allocate 4 vectors, the hardware might > put DPC on message number 0 or 2. Yes, test on HiSilicon hip08 platform, the HW puts DPC on message number 2. > > Out of curiosity, what does happen on this system when we only > allocate 4 vectors? Does DPC end up sharing a vector with PME or AER? > Or is it smart enough to put DPC on message number 1 or 3? Test on HiSilicon hip08 platform, DPC shares a vector withs AER. [ 139.106191] pcieport 0000:00:08.0: PME entry 0 [ 139.112511] pcieport 0000:00:08.0: AER entry 2 [ 139.117890] pcieport 0000:00:08.0: DPC entry 2 19: 1 ITS-MSI 131074 Edge aerdrv, pcie-dpc Thanks, Dongdong > > Bjorn > > . > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-11 10:52 ` [PATCH V4 1/2] " Dongdong Liu 2017-10-11 11:30 ` Bjorn Helgaas @ 2017-10-17 20:25 ` Bjorn Helgaas 2017-10-18 10:50 ` Dongdong Liu 1 sibling, 1 reply; 15+ messages in thread From: Bjorn Helgaas @ 2017-10-17 20:25 UTC (permalink / raw) To: Dongdong Liu Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: > After removing and adding back the PCI root port device, > we see the PCIe port service drivers request irq failed. > pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > aer: probe of 0000:00:00.0:pcie002 failed with error -22 > pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- > PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > > The current code basically does this: > > - allocate 32 vectors > - figure out vector used by PME and hotplug > - figure out vector used by AER > - figure out vector used by DPC > - free the 32 vectors we allocated > - allocate only as many vectors as we need > > but it is broken as calling pci_free_irq_vectors() > invalidates the IRQ numbers returned before by pci_irq_vectors(); > > The hardware works: > - PME and hotplug use the Interrupt Message Number from the PCIe > Capability register. > > - AER uses the AER Interrupt Message Number from the AER Root Error > Status register. > > - DPC uses the DPC Interrupt Message Number from the DPC Capability > register. > > - FRS (not supported by Linux yet) uses the FRS Interrupt Message > Number from the FRS Queuing Capability register. > > - That's a total of 4 possible MSI/MSI-X vectors used for PME, > hotplug, AER, DPC, and FRS, so there's no point in trying to > allocate more than 4 vectors (we currently start with 32). > > - All these Interrupt Message Numbers are read-only to software but > are updated by hardware when software writes the Multiple Message > Enable field of the MSI Message Control register. Thanks for > pointing this out; I didn't realize this before. > > - Therefore, we must read the Interrupt Message Numbers *after* we > write Multiple Message Enable. > > Since we can use at most 4 vectors, this patch is to optimize the > interrupt vector allocation strategy in pcie_port_enable_irq_vec(). > First figure out how many vectors we need,allocate them, and then fill > in the slots in the irqs[] table. > > Cc: <stable@vger.kernel.org> > Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> Thanks a lot for all your work on this. This was a confusing issue with code to match. I tried to simplify the changelog as below; please correct anything I messed up. A few hopefully last questions: - Your subject suggested that this was a bug for both MSI and MSI-X, but I think it only affected MSI, because the spec says MSI-X Interrupt Message Numbers must remain constant. What do you think? - I think the problem was probably exposed by a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only used a single MSI vector, and we didn't do the "allocate vectors, see which ones were used, reallocate only the required number of vectors" dance. It looks like we only allocated a single vector, so all the Interrupt Message Numbers should have been zero. Or did you actually bisect it to 3674cc49da9a? Bjorn commit e402a536cf7122cda6b3bef420de36c22d2907e6 Author: Dongdong Liu <liudongdong3@huawei.com> Date: Wed Oct 11 18:52:57 2017 +0800 PCI/portdrv: Fix MSI multiple interrupt setup When removing and adding back a PCIe Root Port device that uses MSI (not MSI-X), the port service driver IRQ request fails: pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 aer: probe of 0000:00:00.0:pcie002 failed with error -22 pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 dpc: probe of 0000:00:00.0:pcie010 failed with error -22 The previous code basically did this: - Write MSI Multiple Message Enable to enable 32 vectors - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC - Rewrite MSI Multiple Message Enable to enable only as many vectors as we think we need - Assume the Interrupt Message Numbers are still valid The problem is that when we change the Multiple Message Enable field to what we think is the minimal number of vectors required, the hardware may change which vectors it uses. See the "Interrupt Message Number" discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2 (DPC). Read the Interrupt Message Numbers *after* we write the Multiple Message Enable field so we use the correct IRQ vectors. Note that we can't predict how hardware will select Interrupt Message Numbers, so even if we allocate as many vectors as there are interrupt sources, the hardware may still select Message Numbers that result in sharing a vector. E.g., on HiSilicon hip08, if we have 3 sources (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message Numbers 0 for PME/hotplug and 2 for AER and DPC. That leaves vectors 1 and 3 unused, while AER and DPC share vector 2. Even though MSI-X doesn't have the issue with Interrupt Message Numbers changing, this patch may result in more IRQ sharing because we no longer read those Message Numbers and use them to determine how many MSI-X vectors to request. Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X") Suggested-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> [bhelgaas: changelog] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: stable@vger.kernel.org # v4.13+ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-17 20:25 ` Bjorn Helgaas @ 2017-10-18 10:50 ` Dongdong Liu 2017-10-18 22:33 ` Bjorn Helgaas 0 siblings, 1 reply; 15+ messages in thread From: Dongdong Liu @ 2017-10-18 10:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm Hi Bjorn 在 2017/10/18 4:25, Bjorn Helgaas 写道: > On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: >> After removing and adding back the PCI root port device, >> we see the PCIe port service drivers request irq failed. >> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 >> aer: probe of 0000:00:00.0:pcie002 failed with error -22 >> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- >> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ >> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller >> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) >> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 >> dpc: probe of 0000:00:00.0:pcie010 failed with error -22 >> >> The current code basically does this: >> >> - allocate 32 vectors >> - figure out vector used by PME and hotplug >> - figure out vector used by AER >> - figure out vector used by DPC >> - free the 32 vectors we allocated >> - allocate only as many vectors as we need >> >> but it is broken as calling pci_free_irq_vectors() >> invalidates the IRQ numbers returned before by pci_irq_vectors(); >> >> The hardware works: >> - PME and hotplug use the Interrupt Message Number from the PCIe >> Capability register. >> >> - AER uses the AER Interrupt Message Number from the AER Root Error >> Status register. >> >> - DPC uses the DPC Interrupt Message Number from the DPC Capability >> register. >> >> - FRS (not supported by Linux yet) uses the FRS Interrupt Message >> Number from the FRS Queuing Capability register. >> >> - That's a total of 4 possible MSI/MSI-X vectors used for PME, >> hotplug, AER, DPC, and FRS, so there's no point in trying to >> allocate more than 4 vectors (we currently start with 32). >> >> - All these Interrupt Message Numbers are read-only to software but >> are updated by hardware when software writes the Multiple Message >> Enable field of the MSI Message Control register. Thanks for >> pointing this out; I didn't realize this before. >> >> - Therefore, we must read the Interrupt Message Numbers *after* we >> write Multiple Message Enable. >> >> Since we can use at most 4 vectors, this patch is to optimize the >> interrupt vector allocation strategy in pcie_port_enable_irq_vec(). >> First figure out how many vectors we need,allocate them, and then fill >> in the slots in the irqs[] table. >> >> Cc: <stable@vger.kernel.org> >> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") >> Suggested-by: Bjorn Helgaas <bhelgaas@google.com> >> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > Thanks a lot for all your work on this. This was a confusing issue > with code to match. I tried to simplify the changelog as below; > please correct anything I messed up. > > A few hopefully last questions: > > - Your subject suggested that this was a bug for both MSI and MSI-X, > but I think it only affected MSI, because the spec says MSI-X > Interrupt Message Numbers must remain constant. What do you > think? Yes, for a given MSI-X implementation, the entry must remain constant. But in fact, the bug PCIe port service drivers request irq failed does not relate with this. As we said in PATCH V2, The current code does: pci_alloc_irq_vectors(dev, ...) irqs[x] = pci_irq_vector(dev, entry) pci_free_irq_vectors(dev) pci_alloc_irq_vectors(dev, ...) The problem is that the second pci_alloc_irq_vectors() call gets different vectors than the first one. For MSI-X, It maybe also have such bug. > > - I think the problem was probably exposed by a1d5f18cafe6 > ("PCI/portdrv: Support multiple interrupts for MSI as well as > MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only > used a single MSI vector, and we didn't do the "allocate vectors, > see which ones were used, reallocate only the required number of > vectors" dance. Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a single MSI vector(not get the irq and free and alloc irq agian as MSI-X). But for MSI-X, I think it maybe still have a bug. > > It looks like we only allocated a single vector, so all the > Interrupt Message Numbers should have been zero. Or did you > actually bisect it to 3674cc49da9a? I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X. a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X 3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors(). > > Bjorn > > > commit e402a536cf7122cda6b3bef420de36c22d2907e6 > Author: Dongdong Liu <liudongdong3@huawei.com> > Date: Wed Oct 11 18:52:57 2017 +0800 > > PCI/portdrv: Fix MSI multiple interrupt setup > > When removing and adding back a PCIe Root Port device that uses MSI (not > MSI-X), the port service driver IRQ request fails: > > pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > aer: probe of 0000:00:00.0:pcie002 failed with error -22 > pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > > The previous code basically did this: > > - Write MSI Multiple Message Enable to enable 32 vectors > - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC > - Rewrite MSI Multiple Message Enable to enable only as many vectors as > we think we need > - Assume the Interrupt Message Numbers are still valid > > The problem is that when we change the Multiple Message Enable field to > what we think is the minimal number of vectors required, the hardware may > change which vectors it uses. See the "Interrupt Message Number" > discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2 > (DPC). > > Read the Interrupt Message Numbers *after* we write the Multiple Message > Enable field so we use the correct IRQ vectors. > > Note that we can't predict how hardware will select Interrupt Message > Numbers, so even if we allocate as many vectors as there are interrupt > sources, the hardware may still select Message Numbers that result in > sharing a vector. E.g., on HiSilicon hip08, if we have 3 sources > (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message > Numbers 0 for PME/hotplug and 2 for AER and DPC. That leaves vectors 1 > and 3 unused, while AER and DPC share vector 2. > > Even though MSI-X doesn't have the issue with Interrupt Message Numbers > changing, this patch may result in more IRQ sharing because we no longer > read those Message Numbers and use them to determine how many MSI-X vectors > to request. > > Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X") > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > [bhelgaas: changelog] > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: stable@vger.kernel.org # v4.13+ > The commit message looks good to me. I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as it still has a bug for MSI-X. Thanks, Dongdong > . > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-18 10:50 ` Dongdong Liu @ 2017-10-18 22:33 ` Bjorn Helgaas 2017-10-19 2:50 ` Dongdong Liu 0 siblings, 1 reply; 15+ messages in thread From: Bjorn Helgaas @ 2017-10-18 22:33 UTC (permalink / raw) To: Dongdong Liu Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote: > Hi Bjorn > 在 2017/10/18 4:25, Bjorn Helgaas 写道: > >On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: > >>After removing and adding back the PCI root port device, > >>we see the PCIe port service drivers request irq failed. > >>pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > >>aer: probe of 0000:00:00.0:pcie002 failed with error -22 > >>pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- > >>PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > >>pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > >>pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > >>dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > >>dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > >> > >>The current code basically does this: > >> > >> - allocate 32 vectors > >> - figure out vector used by PME and hotplug > >> - figure out vector used by AER > >> - figure out vector used by DPC > >> - free the 32 vectors we allocated > >> - allocate only as many vectors as we need > >> > >>but it is broken as calling pci_free_irq_vectors() > >>invalidates the IRQ numbers returned before by pci_irq_vectors(); > >> > >>The hardware works: > >> - PME and hotplug use the Interrupt Message Number from the PCIe > >> Capability register. > >> > >> - AER uses the AER Interrupt Message Number from the AER Root Error > >> Status register. > >> > >> - DPC uses the DPC Interrupt Message Number from the DPC Capability > >> register. > >> > >> - FRS (not supported by Linux yet) uses the FRS Interrupt Message > >> Number from the FRS Queuing Capability register. > >> > >> - That's a total of 4 possible MSI/MSI-X vectors used for PME, > >> hotplug, AER, DPC, and FRS, so there's no point in trying to > >> allocate more than 4 vectors (we currently start with 32). > >> > >> - All these Interrupt Message Numbers are read-only to software but > >> are updated by hardware when software writes the Multiple Message > >> Enable field of the MSI Message Control register. Thanks for > >> pointing this out; I didn't realize this before. > >> > >> - Therefore, we must read the Interrupt Message Numbers *after* we > >> write Multiple Message Enable. > >> > >>Since we can use at most 4 vectors, this patch is to optimize the > >>interrupt vector allocation strategy in pcie_port_enable_irq_vec(). > >>First figure out how many vectors we need,allocate them, and then fill > >>in the slots in the irqs[] table. > >> > >>Cc: <stable@vger.kernel.org> > >>Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") > >>Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > >>Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > >>Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > > >Thanks a lot for all your work on this. This was a confusing issue > >with code to match. I tried to simplify the changelog as below; > >please correct anything I messed up. > > > >A few hopefully last questions: > > > > - Your subject suggested that this was a bug for both MSI and MSI-X, > > but I think it only affected MSI, because the spec says MSI-X > > Interrupt Message Numbers must remain constant. What do you > > think? > > Yes, for a given MSI-X implementation, the entry must remain constant. > But in fact, the bug PCIe port service drivers request irq failed does > not relate with this. As we said in PATCH V2, > The current code does: > > pci_alloc_irq_vectors(dev, ...) > irqs[x] = pci_irq_vector(dev, entry) > pci_free_irq_vectors(dev) > pci_alloc_irq_vectors(dev, ...) > > The problem is that the second pci_alloc_irq_vectors() call gets different > vectors than the first one. For MSI-X, It maybe also have such bug. OK, I'm not sure I understand yet how this works with MSI-X, but I suspect the current patch still breaks MSI-X. Let me see if I can make sense of what happens when the root port supports MSI-X: - The MSI-X Table Size (in Message Control) is fixed; this field is read-only in hardware. - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP, AER, DPC, etc., are fixed, contain an MSI-X Table index, and must be less than 32. - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2). There's no reason to assume the Interrupt Message Numbers (which can be 0-31) will be in that range. Here's the flow through this path for MSI-X with the current patch applied, with all services enabled. You can see that in msix_setup_entries(), we'll allocate 3 entries and they will use indexes 0-2 in the MSI-X table. The Interrupt Message Numbers index that table, and they can be in the range 0-31. pcie_init_service_irqs pcie_port_enable_irq_vec nvec = 3 # PME/HP, AER, DPC pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) __pci_enable_msix_range(entries==NULL, 1, 3) __pci_enable_msix(entries==NULL, nvec=3) msix_capability_init(entries==NULL, nvec=3) base = msix_map_region(dev, msix_table_size(control)) msix_setup_entries(base, entries==NULL, nvec=3) for (i = 0; i < nvec; i++) entry = alloc_msi_entry() entry->msi_attrib.entry_nr = i # <-- MSI-X table index entry->mask_base = base # MSI-X table base msix_program_entries for_each_pci_msi_entry() writel(PCI_MSIX_ENTRY_CTRL_MASKBIT, pci_msix_desc_addr(entry) + PCI_MSIX_ENTRY_VECTOR_CTRL) nr_entries = 3 # returned from pci_alloc_irq_vectors() # read message numbers: entry = <PME/HP, AER, or DPC message number> # assume entry == 4 if (entry > nr_entries) goto out_free_irqs # we should take this error path I would expect pcie_port_enable_irq_vec() to fail on many root ports with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt Message Number greater than 3. The previous code (before this patch) first allocates 32 MSI-X vectors, then reads all the Interrupt Message Numbers, then allocates just enough MSI-X vectors so all the message numbers are valid table indexes. In the current patch, we don't know the message numbers until after we've already enabled MSI-X with some number of vectors. So for MSI, we have the constraint that we have to enable MSI before reading the Message Numbers. For MSI-X, we can't know how many vectors to request until after we read the Message Numbers. I wonder if we need to split these two paths? > > - I think the problem was probably exposed by a1d5f18cafe6 > > ("PCI/portdrv: Support multiple interrupts for MSI as well as > > MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only > > used a single MSI vector, and we didn't do the "allocate vectors, > > see which ones were used, reallocate only the required number of > > vectors" dance. > > Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a > single MSI vector(not get the irq and free and alloc irq agian as MSI-X). > But for MSI-X, I think it maybe still have a bug. > > > > It looks like we only allocated a single vector, so all the > > Interrupt Message Numbers should have been zero. Or did you > > actually bisect it to 3674cc49da9a? > > I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X. > a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X > 3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors(). > > > > >Bjorn > > > > > >commit e402a536cf7122cda6b3bef420de36c22d2907e6 > >Author: Dongdong Liu <liudongdong3@huawei.com> > >Date: Wed Oct 11 18:52:57 2017 +0800 > > > > PCI/portdrv: Fix MSI multiple interrupt setup > > > > When removing and adding back a PCIe Root Port device that uses MSI (not > > MSI-X), the port service driver IRQ request fails: > > > > pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > > aer: probe of 0000:00:00.0:pcie002 failed with error -22 > > pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > > pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > > pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > > dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > > dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > > > > The previous code basically did this: > > > > - Write MSI Multiple Message Enable to enable 32 vectors > > - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC > > - Rewrite MSI Multiple Message Enable to enable only as many vectors as > > we think we need > > - Assume the Interrupt Message Numbers are still valid > > > > The problem is that when we change the Multiple Message Enable field to > > what we think is the minimal number of vectors required, the hardware may > > change which vectors it uses. See the "Interrupt Message Number" > > discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2 > > (DPC). > > > > Read the Interrupt Message Numbers *after* we write the Multiple Message > > Enable field so we use the correct IRQ vectors. > > > > Note that we can't predict how hardware will select Interrupt Message > > Numbers, so even if we allocate as many vectors as there are interrupt > > sources, the hardware may still select Message Numbers that result in > > sharing a vector. E.g., on HiSilicon hip08, if we have 3 sources > > (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message > > Numbers 0 for PME/hotplug and 2 for AER and DPC. That leaves vectors 1 > > and 3 unused, while AER and DPC share vector 2. > > > > Even though MSI-X doesn't have the issue with Interrupt Message Numbers > > changing, this patch may result in more IRQ sharing because we no longer > > read those Message Numbers and use them to determine how many MSI-X vectors > > to request. > > > > Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X") > > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > [bhelgaas: changelog] > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > Cc: stable@vger.kernel.org # v4.13+ > > > > The commit message looks good to me. > I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as > it still has a bug for MSI-X. > > Thanks, > Dongdong > >. > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-18 22:33 ` Bjorn Helgaas @ 2017-10-19 2:50 ` Dongdong Liu 2017-10-19 22:47 ` Bjorn Helgaas 0 siblings, 1 reply; 15+ messages in thread From: Dongdong Liu @ 2017-10-19 2:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm Hi Bjorn 在 2017/10/19 6:33, Bjorn Helgaas 写道: > On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote: >> Hi Bjorn >> 在 2017/10/18 4:25, Bjorn Helgaas 写道: >>> On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: >>>> After removing and adding back the PCI root port device, >>>> we see the PCIe port service drivers request irq failed. >>>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 >>>> aer: probe of 0000:00:00.0:pcie002 failed with error -22 >>>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- >>>> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ >>>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller >>>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) >>>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 >>>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22 >>>> >>>> The current code basically does this: >>>> >>>> - allocate 32 vectors >>>> - figure out vector used by PME and hotplug >>>> - figure out vector used by AER >>>> - figure out vector used by DPC >>>> - free the 32 vectors we allocated >>>> - allocate only as many vectors as we need >>>> >>>> but it is broken as calling pci_free_irq_vectors() >>>> invalidates the IRQ numbers returned before by pci_irq_vectors(); >>>> >>>> The hardware works: >>>> - PME and hotplug use the Interrupt Message Number from the PCIe >>>> Capability register. >>>> >>>> - AER uses the AER Interrupt Message Number from the AER Root Error >>>> Status register. >>>> >>>> - DPC uses the DPC Interrupt Message Number from the DPC Capability >>>> register. >>>> >>>> - FRS (not supported by Linux yet) uses the FRS Interrupt Message >>>> Number from the FRS Queuing Capability register. >>>> >>>> - That's a total of 4 possible MSI/MSI-X vectors used for PME, >>>> hotplug, AER, DPC, and FRS, so there's no point in trying to >>>> allocate more than 4 vectors (we currently start with 32). >>>> >>>> - All these Interrupt Message Numbers are read-only to software but >>>> are updated by hardware when software writes the Multiple Message >>>> Enable field of the MSI Message Control register. Thanks for >>>> pointing this out; I didn't realize this before. >>>> >>>> - Therefore, we must read the Interrupt Message Numbers *after* we >>>> write Multiple Message Enable. >>>> >>>> Since we can use at most 4 vectors, this patch is to optimize the >>>> interrupt vector allocation strategy in pcie_port_enable_irq_vec(). >>>> First figure out how many vectors we need,allocate them, and then fill >>>> in the slots in the irqs[] table. >>>> >>>> Cc: <stable@vger.kernel.org> >>>> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") >>>> Suggested-by: Bjorn Helgaas <bhelgaas@google.com> >>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> >>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >>> >>> Thanks a lot for all your work on this. This was a confusing issue >>> with code to match. I tried to simplify the changelog as below; >>> please correct anything I messed up. >>> >>> A few hopefully last questions: >>> >>> - Your subject suggested that this was a bug for both MSI and MSI-X, >>> but I think it only affected MSI, because the spec says MSI-X >>> Interrupt Message Numbers must remain constant. What do you >>> think? >> >> Yes, for a given MSI-X implementation, the entry must remain constant. >> But in fact, the bug PCIe port service drivers request irq failed does >> not relate with this. As we said in PATCH V2, >> The current code does: >> >> pci_alloc_irq_vectors(dev, ...) >> irqs[x] = pci_irq_vector(dev, entry) >> pci_free_irq_vectors(dev) >> pci_alloc_irq_vectors(dev, ...) >> >> The problem is that the second pci_alloc_irq_vectors() call gets different >> vectors than the first one. For MSI-X, It maybe also have such bug. > > OK, I'm not sure I understand yet how this works with MSI-X, but I > suspect the current patch still breaks MSI-X. Let me see if I can > make sense of what happens when the root port supports MSI-X: > > - The MSI-X Table Size (in Message Control) is fixed; this field is > read-only in hardware. > > - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP, > AER, DPC, etc., are fixed, contain an MSI-X Table index, and must > be less than 32. > > - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2). > There's no reason to assume the Interrupt Message Numbers (which > can be 0-31) will be in that range. > > Here's the flow through this path for MSI-X with the current patch > applied, with all services enabled. You can see that in > msix_setup_entries(), we'll allocate 3 entries and they will use > indexes 0-2 in the MSI-X table. The Interrupt Message Numbers index > that table, and they can be in the range 0-31. > > pcie_init_service_irqs > pcie_port_enable_irq_vec > nvec = 3 # PME/HP, AER, DPC > pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) > pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) > __pci_enable_msix_range(entries==NULL, 1, 3) > __pci_enable_msix(entries==NULL, nvec=3) > msix_capability_init(entries==NULL, nvec=3) > base = msix_map_region(dev, msix_table_size(control)) > msix_setup_entries(base, entries==NULL, nvec=3) > for (i = 0; i < nvec; i++) > entry = alloc_msi_entry() > entry->msi_attrib.entry_nr = i # <-- MSI-X table index > entry->mask_base = base # MSI-X table base > msix_program_entries > for_each_pci_msi_entry() > writel(PCI_MSIX_ENTRY_CTRL_MASKBIT, > pci_msix_desc_addr(entry) + > PCI_MSIX_ENTRY_VECTOR_CTRL) > > nr_entries = 3 # returned from pci_alloc_irq_vectors() > # read message numbers: > entry = <PME/HP, AER, or DPC message number> > # assume entry == 4 > if (entry > nr_entries) > goto out_free_irqs # we should take this error path Yes, the above analysis is correct. > > I would expect pcie_port_enable_irq_vec() to fail on many root ports > with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt > Message Number greater than 3. > > The previous code (before this patch) first allocates 32 MSI-X > vectors, then reads all the Interrupt Message Numbers, then allocates > just enough MSI-X vectors so all the message numbers are valid table > indexes. > > In the current patch, we don't know the message numbers until after > we've already enabled MSI-X with some number of vectors. > > So for MSI, we have the constraint that we have to enable MSI before > reading the Message Numbers. For MSI-X, we can't know how many > vectors to request until after we read the Message Numbers. I wonder > if we need to split these two paths? If we worry about current patch will break MSI-X. How about [PATCH V3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers. https://patchwork.kernel.org/patch/9992827/ I still think patch v3 will be ok for MSI/MSI-X. ---------------------------------------------- The patch v3 below contains this sequence: pci_alloc_irq_vectors(32) ... write Multiple Message Enable <-- Interrupt Message Numbers change idx[x] = PME/hotplug Interrupt Message Number idx[y] = AER Interrupt Message Number idx[z] = DPC Interrupt Message Number pci_free_irq_vectors() pci_alloc_irq_vectors() ... write Multiple Message Enable <-- Interrupt Message Numbers change for (i = 0; ...) irqs[i] = pci_irq_vector(idx[i]) This incorrectly assumes the Interrupt Message Numbers remain the same after we write Multiple Message Enable. -------------------------------------------- We just worry about Interrupt Message Numbers maybe are not the same. pci_alloc_irq_vectors(32) write Multiple Message Enable=5(32 vectors) <-- Interrupt Message Numbers change idx[x] = PME/hotplug Interrupt Message Number, assume 0 idx[y] = AER Interrupt Message Number, assume 2 idx[z] = DPC Interrupt Message Number, assume 6 pci_free_irq_vectors() pci_alloc_irq_vectors(7) ... write Multiple Message Enable=3(8 vectors) for (i = 0; ...) irqs[i] = pci_irq_vector(idx[i]) As Multiple Message Enable is 8, Hardware must remain the same as before (HiSilicon hip08 design as this).Test on HiSilicon hip08 platform, it works ok. Hardware is required to update this field so that it is correct if the number of ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ MSI Messages assigned to the Function changes when software writes to the Multiple Message Enable field in the MSI Message Control register The PCIe spec is not very clear about this make us confused. We worry about the hardwares are not consistent. Split these two paths means msi/msi-x uses different algorithm. Currently, it maybe a good choice. Thanks, Dongdong > >>> - I think the problem was probably exposed by a1d5f18cafe6 >>> ("PCI/portdrv: Support multiple interrupts for MSI as well as >>> MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only >>> used a single MSI vector, and we didn't do the "allocate vectors, >>> see which ones were used, reallocate only the required number of >>> vectors" dance. >> >> Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a >> single MSI vector(not get the irq and free and alloc irq agian as MSI-X). >> But for MSI-X, I think it maybe still have a bug. >>> >>> It looks like we only allocated a single vector, so all the >>> Interrupt Message Numbers should have been zero. Or did you >>> actually bisect it to 3674cc49da9a? >> >> I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X. >> a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X >> 3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors(). >> >>> >>> Bjorn >>> >>> >>> commit e402a536cf7122cda6b3bef420de36c22d2907e6 >>> Author: Dongdong Liu <liudongdong3@huawei.com> >>> Date: Wed Oct 11 18:52:57 2017 +0800 >>> >>> PCI/portdrv: Fix MSI multiple interrupt setup >>> >>> When removing and adding back a PCIe Root Port device that uses MSI (not >>> MSI-X), the port service driver IRQ request fails: >>> >>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 >>> aer: probe of 0000:00:00.0:pcie002 failed with error -22 >>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ >>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller >>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) >>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 >>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22 >>> >>> The previous code basically did this: >>> >>> - Write MSI Multiple Message Enable to enable 32 vectors >>> - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC >>> - Rewrite MSI Multiple Message Enable to enable only as many vectors as >>> we think we need >>> - Assume the Interrupt Message Numbers are still valid >>> >>> The problem is that when we change the Multiple Message Enable field to >>> what we think is the minimal number of vectors required, the hardware may >>> change which vectors it uses. See the "Interrupt Message Number" >>> discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2 >>> (DPC). >>> >>> Read the Interrupt Message Numbers *after* we write the Multiple Message >>> Enable field so we use the correct IRQ vectors. >>> >>> Note that we can't predict how hardware will select Interrupt Message >>> Numbers, so even if we allocate as many vectors as there are interrupt >>> sources, the hardware may still select Message Numbers that result in >>> sharing a vector. E.g., on HiSilicon hip08, if we have 3 sources >>> (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message >>> Numbers 0 for PME/hotplug and 2 for AER and DPC. That leaves vectors 1 >>> and 3 unused, while AER and DPC share vector 2. >>> >>> Even though MSI-X doesn't have the issue with Interrupt Message Numbers >>> changing, this patch may result in more IRQ sharing because we no longer >>> read those Message Numbers and use them to determine how many MSI-X vectors >>> to request. >>> >>> Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X") >>> Suggested-by: Bjorn Helgaas <bhelgaas@google.com> >>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> >>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >>> [bhelgaas: changelog] >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: stable@vger.kernel.org # v4.13+ >>> >> >> The commit message looks good to me. >> I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as >> it still has a bug for MSI-X. >> >> Thanks, >> Dongdong >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-19 2:50 ` Dongdong Liu @ 2017-10-19 22:47 ` Bjorn Helgaas 0 siblings, 0 replies; 15+ messages in thread From: Bjorn Helgaas @ 2017-10-19 22:47 UTC (permalink / raw) To: Dongdong Liu Cc: hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm On Thu, Oct 19, 2017 at 10:50:57AM +0800, Dongdong Liu wrote: > Hi Bjorn > > 在 2017/10/19 6:33, Bjorn Helgaas 写道: > >On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote: > >>Hi Bjorn > >>在 2017/10/18 4:25, Bjorn Helgaas 写道: > >>>On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote: > >>>>After removing and adding back the PCI root port device, > >>>>we see the PCIe port service drivers request irq failed. > >>>>pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > >>>>aer: probe of 0000:00:00.0:pcie002 failed with error -22 > >>>>pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- > >>>>PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > >>>>pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > >>>>pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > >>>>dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > >>>>dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > >>>> > >>>>The current code basically does this: > >>>> > >>>> - allocate 32 vectors > >>>> - figure out vector used by PME and hotplug > >>>> - figure out vector used by AER > >>>> - figure out vector used by DPC > >>>> - free the 32 vectors we allocated > >>>> - allocate only as many vectors as we need > >>>> > >>>>but it is broken as calling pci_free_irq_vectors() > >>>>invalidates the IRQ numbers returned before by pci_irq_vectors(); > >>>> > >>>>The hardware works: > >>>> - PME and hotplug use the Interrupt Message Number from the PCIe > >>>> Capability register. > >>>> > >>>> - AER uses the AER Interrupt Message Number from the AER Root Error > >>>> Status register. > >>>> > >>>> - DPC uses the DPC Interrupt Message Number from the DPC Capability > >>>> register. > >>>> > >>>> - FRS (not supported by Linux yet) uses the FRS Interrupt Message > >>>> Number from the FRS Queuing Capability register. > >>>> > >>>> - That's a total of 4 possible MSI/MSI-X vectors used for PME, > >>>> hotplug, AER, DPC, and FRS, so there's no point in trying to > >>>> allocate more than 4 vectors (we currently start with 32). > >>>> > >>>> - All these Interrupt Message Numbers are read-only to software but > >>>> are updated by hardware when software writes the Multiple Message > >>>> Enable field of the MSI Message Control register. Thanks for > >>>> pointing this out; I didn't realize this before. > >>>> > >>>> - Therefore, we must read the Interrupt Message Numbers *after* we > >>>> write Multiple Message Enable. > >>>> > >>>>Since we can use at most 4 vectors, this patch is to optimize the > >>>>interrupt vector allocation strategy in pcie_port_enable_irq_vec(). > >>>>First figure out how many vectors we need,allocate them, and then fill > >>>>in the slots in the irqs[] table. > >>>> > >>>>Cc: <stable@vger.kernel.org> > >>>>Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") > >>>>Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > >>>>Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > >>>>Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > >>> > >>>Thanks a lot for all your work on this. This was a confusing issue > >>>with code to match. I tried to simplify the changelog as below; > >>>please correct anything I messed up. > >>> > >>>A few hopefully last questions: > >>> > >>> - Your subject suggested that this was a bug for both MSI and MSI-X, > >>> but I think it only affected MSI, because the spec says MSI-X > >>> Interrupt Message Numbers must remain constant. What do you > >>> think? > >> > >>Yes, for a given MSI-X implementation, the entry must remain constant. > >>But in fact, the bug PCIe port service drivers request irq failed does > >>not relate with this. As we said in PATCH V2, > >>The current code does: > >> > >> pci_alloc_irq_vectors(dev, ...) > >> irqs[x] = pci_irq_vector(dev, entry) > >> pci_free_irq_vectors(dev) > >> pci_alloc_irq_vectors(dev, ...) > >> > >>The problem is that the second pci_alloc_irq_vectors() call gets different > >>vectors than the first one. For MSI-X, It maybe also have such bug. > > > >OK, I'm not sure I understand yet how this works with MSI-X, but I > >suspect the current patch still breaks MSI-X. Let me see if I can > >make sense of what happens when the root port supports MSI-X: > > > > - The MSI-X Table Size (in Message Control) is fixed; this field is > > read-only in hardware. > > > > - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP, > > AER, DPC, etc., are fixed, contain an MSI-X Table index, and must > > be less than 32. > > > > - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2). > > There's no reason to assume the Interrupt Message Numbers (which > > can be 0-31) will be in that range. > > > >Here's the flow through this path for MSI-X with the current patch > >applied, with all services enabled. You can see that in > >msix_setup_entries(), we'll allocate 3 entries and they will use > >indexes 0-2 in the MSI-X table. The Interrupt Message Numbers index > >that table, and they can be in the range 0-31. > > > > pcie_init_service_irqs > > pcie_port_enable_irq_vec > > nvec = 3 # PME/HP, AER, DPC > > pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) > > pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI) > > __pci_enable_msix_range(entries==NULL, 1, 3) > > __pci_enable_msix(entries==NULL, nvec=3) > > msix_capability_init(entries==NULL, nvec=3) > > base = msix_map_region(dev, msix_table_size(control)) > > msix_setup_entries(base, entries==NULL, nvec=3) > > for (i = 0; i < nvec; i++) > > entry = alloc_msi_entry() > > entry->msi_attrib.entry_nr = i # <-- MSI-X table index > > entry->mask_base = base # MSI-X table base > > msix_program_entries > > for_each_pci_msi_entry() > > writel(PCI_MSIX_ENTRY_CTRL_MASKBIT, > > pci_msix_desc_addr(entry) + > > PCI_MSIX_ENTRY_VECTOR_CTRL) > > > > nr_entries = 3 # returned from pci_alloc_irq_vectors() > > # read message numbers: > > entry = <PME/HP, AER, or DPC message number> > > # assume entry == 4 > > if (entry > nr_entries) > > goto out_free_irqs # we should take this error path > > Yes, the above analysis is correct. > > > >I would expect pcie_port_enable_irq_vec() to fail on many root ports > >with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt > >Message Number greater than 3. > > > >The previous code (before this patch) first allocates 32 MSI-X > >vectors, then reads all the Interrupt Message Numbers, then allocates > >just enough MSI-X vectors so all the message numbers are valid table > >indexes. > > > >In the current patch, we don't know the message numbers until after > >we've already enabled MSI-X with some number of vectors. > > > >So for MSI, we have the constraint that we have to enable MSI before > >reading the Message Numbers. For MSI-X, we can't know how many > >vectors to request until after we read the Message Numbers. I wonder > >if we need to split these two paths? > > If we worry about current patch will break MSI-X. > How about [PATCH V3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers. > https://patchwork.kernel.org/patch/9992827/ > > I still think patch v3 will be ok for MSI/MSI-X. > ---------------------------------------------- > The patch v3 below contains this sequence: > > pci_alloc_irq_vectors(32) > ... > write Multiple Message Enable <-- Interrupt Message Numbers change > idx[x] = PME/hotplug Interrupt Message Number > idx[y] = AER Interrupt Message Number > idx[z] = DPC Interrupt Message Number > pci_free_irq_vectors() > pci_alloc_irq_vectors() > ... > write Multiple Message Enable <-- Interrupt Message Numbers change > for (i = 0; ...) > irqs[i] = pci_irq_vector(idx[i]) > > This incorrectly assumes the Interrupt Message Numbers remain the same > after we write Multiple Message Enable. > -------------------------------------------- > We just worry about Interrupt Message Numbers maybe are not the same. > pci_alloc_irq_vectors(32) > write Multiple Message Enable=5(32 vectors) <-- Interrupt Message Numbers change > idx[x] = PME/hotplug Interrupt Message Number, assume 0 > idx[y] = AER Interrupt Message Number, assume 2 > idx[z] = DPC Interrupt Message Number, assume 6 > pci_free_irq_vectors() > pci_alloc_irq_vectors(7) > ... > write Multiple Message Enable=3(8 vectors) > for (i = 0; ...) > irqs[i] = pci_irq_vector(idx[i]) > > As Multiple Message Enable is 8, Hardware must remain the same as before > (HiSilicon hip08 design as this).Test on HiSilicon hip08 platform, it works ok. This isn't strictly required by the spec, since we do change Multiple Message Enable, and hardware is allowed to change the message numbers when we do that. But I agree it is quite unlikely that the message numbers will change since we're allocating enough vectors to accommodate all of them. I'll post a v5 series that is basically a refinement of your v3. > Hardware is required to update this field so that it is correct if the number of > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > MSI Messages assigned to the Function changes when software writes to the Multiple > Message Enable field in the MSI Message Control register > > The PCIe spec is not very clear about this make us confused. We worry about the > hardwares are not consistent. > > Split these two paths means msi/msi-x uses different algorithm. > Currently, it maybe a good choice. > > Thanks, > Dongdong > > > >>> - I think the problem was probably exposed by a1d5f18cafe6 > >>> ("PCI/portdrv: Support multiple interrupts for MSI as well as > >>> MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only > >>> used a single MSI vector, and we didn't do the "allocate vectors, > >>> see which ones were used, reallocate only the required number of > >>> vectors" dance. > >> > >>Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a > >>single MSI vector(not get the irq and free and alloc irq agian as MSI-X). > >>But for MSI-X, I think it maybe still have a bug. > >>> > >>> It looks like we only allocated a single vector, so all the > >>> Interrupt Message Numbers should have been zero. Or did you > >>> actually bisect it to 3674cc49da9a? > >> > >>I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X. > >>a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X > >>3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors(). > >> > >>> > >>>Bjorn > >>> > >>> > >>>commit e402a536cf7122cda6b3bef420de36c22d2907e6 > >>>Author: Dongdong Liu <liudongdong3@huawei.com> > >>>Date: Wed Oct 11 18:52:57 2017 +0800 > >>> > >>> PCI/portdrv: Fix MSI multiple interrupt setup > >>> > >>> When removing and adding back a PCIe Root Port device that uses MSI (not > >>> MSI-X), the port service driver IRQ request fails: > >>> > >>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22 > >>> aer: probe of 0000:00:00.0:pcie002 failed with error -22 > >>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+ > >>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller > >>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1) > >>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22 > >>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22 > >>> > >>> The previous code basically did this: > >>> > >>> - Write MSI Multiple Message Enable to enable 32 vectors > >>> - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC > >>> - Rewrite MSI Multiple Message Enable to enable only as many vectors as > >>> we think we need > >>> - Assume the Interrupt Message Numbers are still valid > >>> > >>> The problem is that when we change the Multiple Message Enable field to > >>> what we think is the minimal number of vectors required, the hardware may > >>> change which vectors it uses. See the "Interrupt Message Number" > >>> discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2 > >>> (DPC). > >>> > >>> Read the Interrupt Message Numbers *after* we write the Multiple Message > >>> Enable field so we use the correct IRQ vectors. > >>> > >>> Note that we can't predict how hardware will select Interrupt Message > >>> Numbers, so even if we allocate as many vectors as there are interrupt > >>> sources, the hardware may still select Message Numbers that result in > >>> sharing a vector. E.g., on HiSilicon hip08, if we have 3 sources > >>> (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message > >>> Numbers 0 for PME/hotplug and 2 for AER and DPC. That leaves vectors 1 > >>> and 3 unused, while AER and DPC share vector 2. > >>> > >>> Even though MSI-X doesn't have the issue with Interrupt Message Numbers > >>> changing, this patch may result in more IRQ sharing because we no longer > >>> read those Message Numbers and use them to determine how many MSI-X vectors > >>> to request. > >>> > >>> Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X") > >>> Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > >>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > >>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > >>> [bhelgaas: changelog] > >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > >>> Cc: stable@vger.kernel.org # v4.13+ > >>> > >> > >>The commit message looks good to me. > >>I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as > >>it still has a bug for MSI-X. > >> > >>Thanks, > >>Dongdong > >>>. > >>> > >> > > > >. > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks 2017-10-11 10:52 [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers Dongdong Liu 2017-10-11 10:52 ` [PATCH V4 1/2] " Dongdong Liu @ 2017-10-11 10:52 ` Dongdong Liu 2017-10-11 13:01 ` David Laight 2017-10-16 12:09 ` Dongdong Liu 2 siblings, 1 reply; 15+ messages in thread From: Dongdong Liu @ 2017-10-11 10:52 UTC (permalink / raw) To: helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm, Dongdong Liu In the AER case, the mask isn't strictly necessary because there are no higher-order bits above the Interrupt Message Number, but using a #define will make it possible to grep for it. Suggested-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> --- drivers/pci/pcie/portdrv_core.c | 4 ++-- include/uapi/linux/pci_regs.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 9692379..2b48297 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) */ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); - entry = reg32 >> 27; + entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27; if (entry >= nr_entries) goto out_free_irqs; @@ -142,7 +142,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) */ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC); pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP, ®16); - entry = reg16 & 0x1f; + entry = reg16 & PCI_EXP_DPC_IRQ; if (entry >= nr_entries) goto out_free_irqs; diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index f8d5804..756e704 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -746,6 +746,7 @@ #define PCI_ERR_ROOT_FIRST_FATAL 0x00000010 /* First UNC is Fatal */ #define PCI_ERR_ROOT_NONFATAL_RCV 0x00000020 /* Non-Fatal Received */ #define PCI_ERR_ROOT_FATAL_RCV 0x00000040 /* Fatal Received */ +#define PCI_ERR_ROOT_AER_IRQ 0xf8000000 /* Advanced Error Interrupt Message Number */ #define PCI_ERR_ROOT_ERR_SRC 52 /* Error Source Identification */ /* Virtual Channel */ @@ -960,6 +961,7 @@ /* Downstream Port Containment */ #define PCI_EXP_DPC_CAP 4 /* DPC Capability */ +#define PCI_EXP_DPC_IRQ 0x1f /* DPC Interrupt message number */ #define PCI_EXP_DPC_CAP_RP_EXT 0x20 /* Root Port Extensions for DPC */ #define PCI_EXP_DPC_CAP_POISONED_TLP 0x40 /* Poisoned TLP Egress Blocking Supported */ #define PCI_EXP_DPC_CAP_SW_TRIGGER 0x80 /* Software Triggering Supported */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks 2017-10-11 10:52 ` [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks Dongdong Liu @ 2017-10-11 13:01 ` David Laight 0 siblings, 0 replies; 15+ messages in thread From: David Laight @ 2017-10-11 13:01 UTC (permalink / raw) To: 'Dongdong Liu', helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm From: Dongdong Liu > Sent: 11 October 2017 11:53 > In the AER case, the mask isn't strictly necessary because there are no > higher-order bits above the Interrupt Message Number, but using a #define > will make it possible to grep for it. > > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > --- > drivers/pci/pcie/portdrv_core.c | 4 ++-- > include/uapi/linux/pci_regs.h | 2 ++ > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 9692379..2b48297 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) > */ > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); > - entry = reg32 >> 27; > + entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27; You've still got the hard coded 27 - which is almost more important than the mask. Perhaps define things so this is: entry = PCI_ERR_ROOT_AER_IRQ(reg32); Think I'd shift then mask with 0x1f. David ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks @ 2017-10-11 13:01 ` David Laight 0 siblings, 0 replies; 15+ messages in thread From: David Laight @ 2017-10-11 13:01 UTC (permalink / raw) To: 'Dongdong Liu', helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm From: Dongdong Liu > Sent: 11 October 2017 11:53 > In the AER case, the mask isn't strictly necessary because there are no > higher-order bits above the Interrupt Message Number, but using a #define > will make it possible to grep for it. >=20 > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > --- > drivers/pci/pcie/portdrv_core.c | 4 ++-- > include/uapi/linux/pci_regs.h | 2 ++ > 2 files changed, 4 insertions(+), 2 deletions(-) >=20 > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_c= ore.c > index 9692379..2b48297 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *d= ev, int *irqs, int mask) > */ > pos =3D pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); > - entry =3D reg32 >> 27; > + entry =3D (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27; You've still got the hard coded 27 - which is almost more important than th= e mask. Perhaps define things so this is: entry =3D PCI_ERR_ROOT_AER_IRQ(reg32); Think I'd shift then mask with 0x1f. David ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks 2017-10-11 13:01 ` David Laight (?) @ 2017-10-11 13:34 ` Bjorn Helgaas -1 siblings, 0 replies; 15+ messages in thread From: Bjorn Helgaas @ 2017-10-11 13:34 UTC (permalink / raw) To: David Laight Cc: 'Dongdong Liu', hch, linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm On Wed, Oct 11, 2017 at 01:01:26PM +0000, David Laight wrote: > From: Dongdong Liu > > Sent: 11 October 2017 11:53 > > In the AER case, the mask isn't strictly necessary because there are no > > higher-order bits above the Interrupt Message Number, but using a #define > > will make it possible to grep for it. > > > > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > > --- > > drivers/pci/pcie/portdrv_core.c | 4 ++-- > > include/uapi/linux/pci_regs.h | 2 ++ > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index 9692379..2b48297 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) > > */ > > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); > > - entry = reg32 >> 27; > > + entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27; > > You've still got the hard coded 27 - which is almost more important than the mask. > Perhaps define things so this is: > entry = PCI_ERR_ROOT_AER_IRQ(reg32); > > Think I'd shift then mask with 0x1f. Personally, I prefer to have the mask unshifted so it's easy to see where the fields are in the register and to compare them with the spec, e.g., #define PCI_EXP_DEVCAP_PAYLOAD 0x00000007 /* Max_Payload_Size */ #define PCI_EXP_DEVCAP_PHANTOM 0x00000018 /* Phantom functions */ #define PCI_EXP_DEVCAP_EXT_TAG 0x00000020 /* Extended tags */ Should there be a #define for the shift? Maybe. It doesn't bother me unless it's used in many places. Here I think there'd only be one use, so having it hard-coded along with the symbolic mask is enough for me. Sometimes too many #defines clutter up the include file. Just my personal preferences. Bjorn ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers 2017-10-11 10:52 [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers Dongdong Liu @ 2017-10-16 12:09 ` Dongdong Liu 2017-10-11 10:52 ` [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks Dongdong Liu 2017-10-16 12:09 ` Dongdong Liu 2 siblings, 0 replies; 15+ messages in thread From: Dongdong Liu @ 2017-10-16 12:09 UTC (permalink / raw) To: helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm Hi Bjorn Is this patchset ok ? or do I still need to work v5 without optimizing the interrupt vector allocation strategy(just use PATCH v3 and add #defines for the AER and DPC Interrupt Message Number masks.) Thanks, Dongdong 锟斤拷 2017/10/11 18:52, Dongdong Liu 写锟斤拷: > This patchset is to optimize the interrupt vector allocation strategy > in pcie_port_enable_irq_vec() and add #defines for the AER and DPC > Interrupt Message Number masks. > > v3->v4: > - Optimize the interrupt vector allocation strategy in > pcie_port_enable_irq_vec(). > - add #defines for the AER and DPC Interrupt Message Number masks. > v2->v3: > - Use a 12-character SHA1 commit id. > - Rebase on v4.14-rc4. > v1->v2: > - Fix comments on PATCH v1. > - Simplify implementation. > > Dongdong Liu (2): > PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers > PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number > masks > > drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++--------------------------- > include/uapi/linux/pci_regs.h | 2 ++ > 2 files changed, 18 insertions(+), 33 deletions(-) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers @ 2017-10-16 12:09 ` Dongdong Liu 0 siblings, 0 replies; 15+ messages in thread From: Dongdong Liu @ 2017-10-16 12:09 UTC (permalink / raw) To: helgaas, hch Cc: linux-pci, stable, gabriele.paoloni, charles.chenxin, linuxarm Hi Bjorn Is this patchset ok ? or do I still need to work v5 without optimizing the interrupt vector allocation strategy(just use PATCH v3 and add #defines for the AER and DPC Interrupt Message Number masks.) Thanks, Dongdong 在 2017/10/11 18:52, Dongdong Liu 写道: > This patchset is to optimize the interrupt vector allocation strategy > in pcie_port_enable_irq_vec() and add #defines for the AER and DPC > Interrupt Message Number masks. > > v3->v4: > - Optimize the interrupt vector allocation strategy in > pcie_port_enable_irq_vec(). > - add #defines for the AER and DPC Interrupt Message Number masks. > v2->v3: > - Use a 12-character SHA1 commit id. > - Rebase on v4.14-rc4. > v1->v2: > - Fix comments on PATCH v1. > - Simplify implementation. > > Dongdong Liu (2): > PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers > PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number > masks > > drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++--------------------------- > include/uapi/linux/pci_regs.h | 2 ++ > 2 files changed, 18 insertions(+), 33 deletions(-) > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-10-19 22:47 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-11 10:52 [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers Dongdong Liu 2017-10-11 10:52 ` [PATCH V4 1/2] " Dongdong Liu 2017-10-11 11:30 ` Bjorn Helgaas 2017-10-12 2:40 ` Dongdong Liu 2017-10-17 20:25 ` Bjorn Helgaas 2017-10-18 10:50 ` Dongdong Liu 2017-10-18 22:33 ` Bjorn Helgaas 2017-10-19 2:50 ` Dongdong Liu 2017-10-19 22:47 ` Bjorn Helgaas 2017-10-11 10:52 ` [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks Dongdong Liu 2017-10-11 13:01 ` David Laight 2017-10-11 13:01 ` David Laight 2017-10-11 13:34 ` Bjorn Helgaas 2017-10-16 12:09 ` [PATCH V4 0/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers Dongdong Liu 2017-10-16 12:09 ` Dongdong Liu
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.