| Message ID | 152097754955.241946.9551793957889760940.stgit@bhelgaas-glaptop.roam.corp.google.com |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/MSI: Don't set up INTx if MSI or MSI-X is enabled | expand |
> Change the remaining arches (cris, frv, and ia64) to skip INTx setup when > either MSI or MSI-X is enabled. My ia64 still boots with this patch applied (and doesn't have any new/different messages in the console log). So: Tested-by: Tony Luck <tony.luck@intel.com> -Tony
Should this logic go into a little helper so that everyone is kept in sync?
On 3/13/2018 10:45 PM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > If a device already has MSI or MSI-X enabled, there's no need to set up its > legacy INTx interrupt. > > 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. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > 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; > } > > /* >
On Tue, Mar 13, 2018 at 11:45 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > 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. 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) > > 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. FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > 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; > } > > /* >
On Wed, Mar 14, 2018 at 01:35:19AM -0700, Christoph Hellwig wrote: > Should this logic go into a little helper so that everyone is kept > in sync? Great point, thanks! What I'd really like is to completely get rid of most of the pcibios_enable_device() implementations. Most of them contain nothing that's arch-specific. I'll look at that some more, but will probably do that as additional steps on top of this one. That way this patch stays mostly trivial and obvious as a generalization of 16cf0ebc35dd ("x86/PCI: Do not use interrupt links for devices using MSI-X") to other affected arches. Bjorn
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@kernel.org> wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > 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@gmail.com> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > 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
On Wed, Mar 14, 2018 at 7:49 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > 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@kernel.org> wrote: >> > 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. I meant that the behaviour now is changed from check MSI only case to check MSI _or MSI-x_ case Not all code paths may survive that. >> 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. Exactly, that's why I pointed on above.
On Tue, Mar 13, 2018 at 04:45:49PM -0500, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > If a device already has MSI or MSI-X enabled, there's no need to set up its > legacy INTx interrupt. > > 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. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Applied to pci/msi with tested-by from Tony and reviewed-by Rafael and Andy. And I added a note to my to-do list about getting rid of pcibios_enable_device(). > --- > 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; > } > > /* >
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; } /*