Re: [PATCH] PCI/DPC: Request DPC only if also requesting AER

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Feb 20, 2024 at 06:45:32PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/20/24 3:55 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > When booting with "pci=noaer", we don't request control of AER, but we
> > previously *did* request control of DPC, as in the dmesg log attached at
> > the bugzilla below:
> >
> >   Command line: ... pci=noaer
> >   acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
> >   acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]
> >
> > That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which
> > says:
> >
> >   If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it
> >   must also set bit 7 of the Support field (indicating support for Error
> >   Disconnect Recover notifications) and bits 3 and 4 of the Control field
> >   (requesting control of PCI Express Advanced Error Reporting and the PCI
> >   Express Capability Structure).
> >
> > Request DPC control only if we have also requested AER control.
> 
> Can you also add similar check in calculate_support call?
> 
>         if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_EDR))
>                 support |= OSC_PCI_EDR_SUPPORT;

That doesn't seem right to me.  The implementation note in sec 4.6.12
suggests that EDR Notifications may be used even when the firmware
maintains control of AER and DPC.  Maybe that note is wrong or
misleading, but as written, I interpret that as meaning that it may be
useful for the platform to know that the OS supports EDR even if it
AER control isn't requested or granted.

> > Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: <stable@vger.kernel.org>	# v5.7+
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: Matthew W Carlis <mattc@purestorage.com>
> > Cc: Keith Busch <kbusch@kernel.org>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > ---
> >  drivers/acpi/pci_root.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 58b89b8d950e..1c16965427b3 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -518,17 +518,19 @@ static u32 calculate_control(void)
> >  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> >  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> >  
> > -	if (pci_aer_available())
> > +	if (pci_aer_available()) {
> >  		control |= OSC_PCI_EXPRESS_AER_CONTROL;
> >  
> > -	/*
> > -	 * Per the Downstream Port Containment Related Enhancements ECN to
> > -	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> > -	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
> > -	 * and EDR.
> > -	 */
> > -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> > -		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> > +		/*
> > +		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
> > +		 * OS can request DPC control only if it has advertised
> > +		 * OSC_PCI_EDR_SUPPORT and requested both
> > +		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
>
> I think you mean OSC_PCI_EXPRESS_DPC_CONTROL.

No, I just tried to rephrase the text for _OSC Control, bit 7 (quoted
in the commit log), so I think requesting control of bits 3 and 4 (AER
and PCIe Capability) is right.

> > +		 * OSC_PCI_EXPRESS_AER_CONTROL.
> > +		 */
> > +		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> > +			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> 
> Since you are cleaning up this part, why not add a patch to remove
> CONFIG_PCIE_EDR?

Good idea, I'll do that, too.

Bjorn