[PATCH] PCI: Avoid broken MSI on SB600 USB devices

[PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Bjorn Helgaas]

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>
---
 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

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Bjorn Helgaas]

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
> 

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Bjorn Helgaas]

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

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Bjorn Helgaas]

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

Re: [PATCH] PCI: Avoid broken MSI on SB600 USB devices

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko