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@xxxxxxxxxxxxxxx>
> >>>>Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
> >>>>Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>>>Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> >>>>Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> >>>
> >>>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@xxxxxxxxxx>
> >>>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@xxxxxxxxxx>
> >>> Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> >>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> >>> [bhelgaas: changelog]
> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>> Cc: stable@xxxxxxxxxxxxxxx # 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
> >>>.
> >>>
> >>
> >
> >.
> >
>
