Re: [PATCH] PCI: Add quirk to clear MSI-X

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Mar 20, 2023 at 05:20:55PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Monday, March 20, 2023 12:15
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>;
> > bhelgaas@google.com; linux-pci@vger.kernel.org; thomas@glanzmann.de;
> > Rafael J. Wysocki <rjw@rjwysocki.net>
> > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X

Whatever you're using for email adds all these redundant headers to
every response...

> > On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > Sent: Friday, March 10, 2023 16:14
> > > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> > > > <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> > > > pci@vger.kernel.org; thomas@glanzmann.de
> > > > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> > > >
> > > > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > > > > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > > > > ...

> > it's important that the commit log is accurate and makes sense even to
> > people who don't know the internals of the device.
> > 
> > It *sounds* like what's happening is:
> > 
> >   - OS writes PMCSR to put device in D3hot
> >   - BIOS traps D0->D3hot transition via something like SMI and
> >     captures MSI-X state
> >   - Device enters D3hot
> >   - Device internal MSI-X state is lost
> >   - BIOS traps D3hot->D0 transition via SMI
> >   - Device enters D0
> >   - BIOS restores MSI-X state
> >   - OS resumes use of device
> > 
> > If that's what's happening, the fact that the device loses the
> > internal state in D3hot sounds like a *hardware* defect -- if you put
> > the device in a system without a BIOS, the D0->D3hot->D0 transitions
> > would not work as required by the PCIe spec.
> 
> Actually it's a controller integrated into the APU.
> 
> So any system you put this APU into has a BIOS.  Because it's a socketed
> APU people can very easily move it from one motherboard to another and one
> vendor may have the BIOS properly configuring but another might not.
>
> > We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
> > defect, but the only reason BIOS would *need* that save/restore is
> > because of the underlying *hardware* defect.
> > 
> > If that's the case, I would expect a commit log something like this:
> > 
> >   The AMD [1022:15b8] USB controller loses some internal functional
> >   MSI-X context when transitioning from D0 to D3hot.  BIOS normally
> >   traps D0->D3hot and D3hot->D0 transitions so it can save and restore
> >   that internal context, but some firmware in the field lacks this
> >   workaround.
> 
> I wouldn't call it a workaround.  The hardware is doing exactly as it's
> intended for how the firmware programmed.

The whole point of the PCI spec is to build devices where standard
features like power management can be operated without device-specific
knowledge.  If we need device-specific code in BIOS or Linux, I'd say
that's a workaround.

Does this device set No_Soft_Reset?  If it does, when it receives a
config write to PMCSR that puts it in D3hot, followed by a config
write that puts it back in D0, it's supposed to return to D0 with no
additional software intervention required.  I think that also means no
BIOS intervention is required.

If it does not set No_Soft_Reset, pci_pm_reset() assumes that a
D0->D3hot->D0 transition resets the device.  We save and restore the
MSI-X Capability in that case, but we do NOT run the
DECLARE_PCI_FIXUP_RESUME_EARLY quirks.  I think that means MSI-X would
not work after a PM reset of this device.

Bjorn