| Message ID | 20220321183446.1108325-1-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 63cd736f449445edcd7f0bcc7d84453e9beec0aa |
| Headers | show |
| Series | PCI: Avoid broken MSI on SB600 USB devices | expand |
On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > setting PCI_COMMAND_INTX_DISABLE, MSI doesn't work either. The PCI/PCIe > specs do not require software to set PCI_COMMAND_INTX_DISABLE when enabling > MSI, but Linux has done that for many years. > > Mick reported that 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI > devices") broke these devices. Prior to 306c54d0edb6, they used INTx. > Starting with 306c54d0edb6, they use MSI, and and the fact that Linux sets > PCI_COMMAND_INTX_DISABLE means both INTx and MSI are disabled on these > devices. > > Avoid this SB600 defect by disabling MSI so we use INTx as before. > > Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215690 > Link: https://lore.kernel.org/all/PxIByDyBRcsbpcmVhGSNDFAoUcMmb78ctXCkw6fbpx25TGlCHvA6SJjjFkNr1FfQZMntYPTNyvEnblxzAZ8a6jP9ddLpKeCN6Chi_2FuexU=@protonmail.com/ > BugLink: https://lore.kernel.org/all/20200702143045.23429-1-andriy.shevchenko@linux.intel.com/ > Link: https://lore.kernel.org/r/20220314101448.90074-1-andriy.shevchenko@linux.intel.com > Reported-by: Mick Lorain <micklorain@protonmail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Unless there's objection, I plan to include this in the v5.18 pull request in the next few days. It was in the 20220322 linux-next tree: https://lore.kernel.org/linux-next/20220322203829.2bb0166c@canb.auug.org.au/ > --- > drivers/pci/quirks.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index d2dd6a6cda60..5f46fed01e6c 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -1811,6 +1811,18 @@ static void quirk_alder_ioapic(struct pci_dev *pdev) > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EESSC, quirk_alder_ioapic); > #endif > > +static void quirk_no_msi(struct pci_dev *dev) > +{ > + pci_info(dev, "avoiding MSI to work around a hardware defect\n"); > + dev->no_msi = 1; > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4386, quirk_no_msi); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4387, quirk_no_msi); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4388, quirk_no_msi); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4389, quirk_no_msi); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x438a, quirk_no_msi); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x438b, quirk_no_msi); > + > static void quirk_pcie_mch(struct pci_dev *pdev) > { > pdev->no_msi = 1; > -- > 2.25.1 >
On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > setting PCI_COMMAND_INTX_DISABLE, > > MSI doesn't work either. I think this is not correct. > > The PCI/PCIe > > specs do not require software to set PCI_COMMAND_INTX_DISABLE when enabling > > MSI, but Linux has done that for many years. > > > > Mick reported that 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI > > devices") broke these devices. Prior to 306c54d0edb6, they used INTx. > > Starting with 306c54d0edb6, they use MSI, and and the fact that Linux sets > > PCI_COMMAND_INTX_DISABLE means both INTx and MSI are disabled on these > > devices. > > Avoid this SB600 defect by disabling MSI so we use INTx as before. And this is kinda too conservative approach. > > Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices") > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215690 > > Link: https://lore.kernel.org/all/PxIByDyBRcsbpcmVhGSNDFAoUcMmb78ctXCkw6fbpx25TGlCHvA6SJjjFkNr1FfQZMntYPTNyvEnblxzAZ8a6jP9ddLpKeCN6Chi_2FuexU=@protonmail.com/ > > BugLink: https://lore.kernel.org/all/20200702143045.23429-1-andriy.shevchenko@linux.intel.com/ > > Link: https://lore.kernel.org/r/20220314101448.90074-1-andriy.shevchenko@linux.intel.com > > Reported-by: Mick Lorain <micklorain@protonmail.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> And it hasn't been tested by the reporter. > Unless there's objection, I plan to include this in the v5.18 pull > request in the next few days. You are the maintainer here and it's your choice. I'm not going to stop you.
On Wed, Mar 23, 2022 at 10:03:38AM +0200, Andy Shevchenko wrote: > On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > > setting PCI_COMMAND_INTX_DISABLE, > > > > MSI doesn't work either. > > I think this is not correct. I'd like to make it correct. What would make this better? I was trying to say the same as your original commit log: ATI PCIe-USB adapter advertises MSI, but it doesn't work if INTx is disabled. Bjorn
On Wed, Mar 23, 2022 at 1:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Mar 23, 2022 at 10:03:38AM +0200, Andy Shevchenko wrote: > > On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > > > setting PCI_COMMAND_INTX_DISABLE, > > > > > > MSI doesn't work either. > > > > I think this is not correct. > > I'd like to make it correct. What would make this better? MSI with the quirk (not in this patch) is working and has been tested. That said, the part that I commented on is confusing and states the opposite. I would change the patch if you ask me how to improve it, > I was > trying to say the same as your original commit log: > > ATI PCIe-USB adapter advertises MSI, but it doesn't work if INTx is > disabled. I'm not a native speaker, maybe I was wrong in formulating that MSI enabling needs a quirk.
On Wed, 2022-03-23 at 10:03 +0200, Andy Shevchenko wrote: > On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > > setting PCI_COMMAND_INTX_DISABLE, > > > > MSI doesn't work either. > > I think this is not correct. I think it was perfectly correct until you added a couple of newlines in the middle of the sentence, then took it out of context. :) "If INTX is disabled, MSI doesn't work either". But really, in that case surely the solution is *not* to disable INTX for this device. Then MSI will work, right?
On Wed, Mar 23, 2022 at 12:43:48PM +0000, David Woodhouse wrote: > On Wed, 2022-03-23 at 10:03 +0200, Andy Shevchenko wrote: > > On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > > > setting PCI_COMMAND_INTX_DISABLE, > > > > > > MSI doesn't work either. > > > > I think this is not correct. > > I think it was perfectly correct until you added a couple of newlines > in the middle of the sentence, then took it out of context. :) > > "If INTX is disabled, MSI doesn't work either". > > But really, in that case surely the solution is *not* to disable INTX > for this device. Then MSI will work, right? That's what Andy's original patch [1] does, and MSI *does* work if we skip disabling INTx. I'm hesitant [2] about that approach because it creates two classes of devices using MSI (most have INTx disabled but a few do not), which makes it harder to reason about them. For example, there are non-MSI paths that read or set the "disable INTx" bit, so we have to consider: - will readers be surprised if a device using MSI has INTx enabled? - will writers care disabling INTx disables *all* interrupts, not just INTx? So skipping the INTx disable certainly works most of the time and *likely* works all the time, but there are cases that could be problems and we don't have a compelling reason to use MSI on these devices. [1] https://lore.kernel.org/r/20220314101448.90074-1-andriy.shevchenko@linux.intel.com [2] https://lore.kernel.org/r/20220318210947.GA845994@bhelgaas
On Wed, Mar 23, 2022 at 2:43 PM David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2022-03-23 at 10:03 +0200, Andy Shevchenko wrote: > > On Wed, Mar 23, 2022 at 4:26 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Mon, Mar 21, 2022 at 01:34:46PM -0500, Bjorn Helgaas wrote: > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Some ATI SB600 USB adapters advertise MSI, but if INTx is disabled by > > > > setting PCI_COMMAND_INTX_DISABLE, > > > > > > MSI doesn't work either. > > > > I think this is not correct. > > I think it was perfectly correct until you added a couple of newlines > in the middle of the sentence, then took it out of context. :) > > "If INTX is disabled, MSI doesn't work either". Ah, I stand corrected. Thanks for the English lesson! > But really, in that case surely the solution is *not* to disable INTX > for this device. Then MSI will work, right? That was my intention, but Bjorn has concerns.
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index d2dd6a6cda60..5f46fed01e6c 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -1811,6 +1811,18 @@ static void quirk_alder_ioapic(struct pci_dev *pdev) DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EESSC, quirk_alder_ioapic); #endif +static void quirk_no_msi(struct pci_dev *dev) +{ + pci_info(dev, "avoiding MSI to work around a hardware defect\n"); + dev->no_msi = 1; +} +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4386, quirk_no_msi); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4387, quirk_no_msi); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4388, quirk_no_msi); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4389, quirk_no_msi); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x438a, quirk_no_msi); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x438b, quirk_no_msi); + static void quirk_pcie_mch(struct pci_dev *pdev) { pdev->no_msi = 1;