Re: [PATCH] PCI/MSI: Don't set up INTx if MSI or MSI-X is enabled

[Bjorn Helgaas]

On Wed, Mar 14, 2018 at 06:31:38PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 11:45 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> 
> Agree with Christoph's comment.
> 
> > If a device already has MSI or MSI-X enabled, there's no need to set up its
> > legacy INTx interrupt.
> 
> Just point to the actual behaviour of this.

By "point to the actual behaviour", do you mean adding something to
the changelog along the lines of the following?

  If MSI or MSI-X is enabled, the device uses that.  It uses INTx only
  if both MSI and MSI-X are disabled (see PCIe r4.0, sec 7.7.1.2).

I did add that because I think that spec reference is useful.  If you
have something else in mind, maybe an example would help me
understand.

> In some cases code in question has to distinguish between MSI and
> MSI-x.  So, this or similar changes has to be done with keeping
> above in mind.
> 
> (Existing example is Thunderbolt driver)

Sorry, I didn't get your point here.  Certainly some code needs to
distinguish between MSI and MSI-X, but I don't think that's the case
here.  I'm not proposing to change Thunderbolt; I do see that it uses
dev->msix_enabled (but not dev->msi_enabled), and it doesn't look like
using pci_dev_msi_enabled() there would be appropriate.

> > bba6f6fc68e7 ("[PATCH] MSI-X: fix resume crash") changed the cris, frv,
> > x86, and ia64 arches to skip INTx setup when MSI is enabled.
> >
> > 16cf0ebc35dd ("x86/PCI: Do not use interrupt links for devices using
> > MSI-X") extended that by changing x86 to also skip INTx setup when MSI-X is
> > enabled.
> >
> > Change the remaining arches (cris, frv, and ia64) to skip INTx setup when
> > either MSI or MSI-X is enabled.
> >
> > Also update mn10300 (which didn't exist at the time of bba6f6fc68e7) to
> > follow the same pattern.
> 
> Perhaps no need to change the architectures that are about to be
> removed completely from the kernel.

Yeah, I figured there was a danger of that, but I haven't kept up on
exactly which are on the chopping block, so I figured it'd be trivial
to drop anything that turns out to be unnecessary.

> FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > ---
> >  arch/cris/arch-v32/drivers/pci/bios.c |    2 +-
> >  arch/frv/mb93090-mb00/pci-vdk.c       |    2 +-
> >  arch/ia64/pci/pci.c                   |    4 ++--
> >  arch/mn10300/unit-asb2305/pci.c       |    8 +++++---
> >  4 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/cris/arch-v32/drivers/pci/bios.c b/arch/cris/arch-v32/drivers/pci/bios.c
> > index 6b9e6cfaa29e..c2bed0cc060b 100644
> > --- a/arch/cris/arch-v32/drivers/pci/bios.c
> > +++ b/arch/cris/arch-v32/drivers/pci/bios.c
> > @@ -68,7 +68,7 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >         if ((err = pcibios_enable_resources(dev, mask)) < 0)
> >                 return err;
> >
> > -       if (!dev->msi_enabled)
> > +       if (!pci_dev_msi_enabled(dev))
> >                 pcibios_enable_irq(dev);
> >         return 0;
> >  }
> > diff --git a/arch/frv/mb93090-mb00/pci-vdk.c b/arch/frv/mb93090-mb00/pci-vdk.c
> > index f211839e2cae..4a55d1b82d21 100644
> > --- a/arch/frv/mb93090-mb00/pci-vdk.c
> > +++ b/arch/frv/mb93090-mb00/pci-vdk.c
> > @@ -413,7 +413,7 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >
> >         if ((err = pci_enable_resources(dev, mask)) < 0)
> >                 return err;
> > -       if (!dev->msi_enabled)
> > +       if (!pci_dev_msi_enabled(dev))
> >                 pcibios_enable_irq(dev);
> >         return 0;
> >  }
> > diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> > index f5ec736100ee..7ccc64d5fe3e 100644
> > --- a/arch/ia64/pci/pci.c
> > +++ b/arch/ia64/pci/pci.c
> > @@ -398,7 +398,7 @@ pcibios_enable_device (struct pci_dev *dev, int mask)
> >         if (ret < 0)
> >                 return ret;
> >
> > -       if (!dev->msi_enabled)
> > +       if (!pci_dev_msi_enabled(dev))
> >                 return acpi_pci_irq_enable(dev);
> >         return 0;
> >  }
> > @@ -407,7 +407,7 @@ void
> >  pcibios_disable_device (struct pci_dev *dev)
> >  {
> >         BUG_ON(atomic_read(&dev->enable_cnt));
> > -       if (!dev->msi_enabled)
> > +       if (!pci_dev_msi_enabled(dev))
> >                 acpi_pci_irq_disable(dev);
> >  }
> >
> > diff --git a/arch/mn10300/unit-asb2305/pci.c b/arch/mn10300/unit-asb2305/pci.c
> > index 3dfe2d31c67b..4d36ea517679 100644
> > --- a/arch/mn10300/unit-asb2305/pci.c
> > +++ b/arch/mn10300/unit-asb2305/pci.c
> > @@ -399,10 +399,12 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >  {
> >         int err;
> >
> > -       err = pci_enable_resources(dev, mask);
> > -       if (err == 0)
> > +       if ((err = pci_enable_resources(dev, mask)) < 0)
> > +               return err;
> > +
> > +       if (!pci_dev_msi_enabled(dev))
> >                 pcibios_enable_irq(dev);
> > -       return err;
> > +       return 0;
> >  }
> >
> >  /*
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko