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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Sorry about that, let me use a real email client this time.


On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
-----Original Message-----
From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
Sent: Friday, March 10, 2023 16:14
To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
Cc: Natikar, Basavaraj <Basavaraj.Natikar@xxxxxxx>; Natikar, Basavaraj
<Basavaraj.Natikar@xxxxxxx>; bhelgaas@xxxxxxxxxx; linux-
pci@xxxxxxxxxxxxxxx; thomas@xxxxxxxxxxxx
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.

Right.  I "think" the confusion here might stem from the term "BIOS".

The initialization of the hardware happens from a series of microcontrollers in the APU before X86 cores are released from reset. By the time UEFI runs all of this should have already happened.

When I say "BIOS" I mean collectively "all" of this firmware.

If we need device-specific code in BIOS or Linux, I'd say
that's a workaround.

Something I'd like to point out in case it wasn't apparent is Windows doesn't actually hit this problem. It doesn't matter if the correct hardware initialization code is included in "BIOS" or not.

So I wonder maybe we should be looking at this another way?


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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux