On Thu, May 20, 2021 at 4:05 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> Hello!
>
> On Thursday 20 May 2021 15:47:46 Sandor Bodo-Merle wrote:
> > Hi Pali,
> >
> > thanks for catching this - i dig up the followup fixup commit we have
> > for the iproc multi MSI (it was sent to Broadcom - but unfortunately
> > we missed upstreaming it).
> >
> > Commit fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
> > failed to reserve the proper number of bits from the inner domain.
> > We need to allocate the proper amount of bits otherwise the domains for
> > multiple PCIe endpoints may overlap and freeing one of them will result
> > in freeing unrelated MSI vectors.
> >
> > Fixes: fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
> > ---
> > drivers/pci/host/pcie-iproc-msi.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git drivers/pci/host/pcie-iproc-msi.c drivers/pci/host/pcie-iproc-msi.c
> > index 708fdb1065f8..a00492dccb74 100644
> > --- drivers/pci/host/pcie-iproc-msi.c
> > +++ drivers/pci/host/pcie-iproc-msi.c
> > @@ -260,11 +260,11 @@ static int iproc_msi_irq_domain_alloc(struct
> > irq_domain *domain,
> >
> > mutex_lock(&msi->bitmap_lock);
> >
> > - /* Allocate 'nr_cpus' number of MSI vectors each time */
> > + /* Allocate 'nr_irqs' multiplied by 'nr_cpus' number of MSI
> > vectors each time */
> > hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
> > - msi->nr_cpus, 0);
> > + msi->nr_cpus * nr_irqs, 0);
>
> I'm not sure if this construction is correct. Multi-MSI interrupts needs
> to be aligned to number of requested interrupts. So if wifi driver asks
> for 32 Multi-MSI interrupts then first allocated interrupt number must
> be dividable by 32.
>
Ahh - i guess you are right. In our internal engineering we always
request 32 vectors.
IIRC the multiply by "nr_irqs" was added for iqr affinity to work correctly.
> > if (hwirq < msi->nr_msi_vecs) {
> > - bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
> > + bitmap_set(msi->bitmap, hwirq, msi->nr_cpus * nr_irqs);
>
> And another issue is that only power of 2 interrupts for Multi-MSI can
> be allocated. Otherwise one number may be allocated to more devices.
>
> But I'm not sure how number of CPUs affects it as other PCIe controller
> drivers do not use number of CPUs.
>
> Other drivers are using bitmap_find_free_region() function with
> order_base_2(nr_irqs) as argument.
>
> I hope that somebody else more skilled with MSI interrupts look at these
> constructions if are correct or needs more rework.
>
I see the point - i'll ask also in our engineering department ...
> > } else {
> > mutex_unlock(&msi->bitmap_lock);
> > return -ENOSPC;
> > @@ -292,7 +292,7 @@ static void iproc_msi_irq_domain_free(struct
> > irq_domain *domain,
> > mutex_lock(&msi->bitmap_lock);
> >
> > hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
> > - bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
> > + bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus * nr_irqs);
> >
> > mutex_unlock(&msi->bitmap_lock);
> >
> >
> > On Thu, May 20, 2021 at 2:04 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
> > >
> > > Hello!
> > >
> > > I think there is a bug in pcie-iproc-msi.c driver. It declares
> > > Multi MSI support via MSI_FLAG_MULTI_PCI_MSI flag, see:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-iproc-msi.c?h=v5.12#n174
> > >
> > > but its iproc_msi_irq_domain_alloc() function completely ignores nr_irqs
> > > argument when allocating interrupt numbers from bitmap, see:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-iproc-msi.c?h=v5.12#n246
> > >
> > > I think this this is incorrect as alloc callback should allocate nr_irqs
> > > multi interrupts as caller requested. All other drivers with Multi MSI
> > > support are doing it.
> > >
> > > Could you look at it?
