Re: [PATCH 1/1] PCI/portdrv: Allow DPC if the OS controls AER natively.

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Jan 22, 2024 at 06:37:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 1/22/24 11:32 AM, Bjorn Helgaas wrote:
> > On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
> >> A small part is probably historical; we've been using DPC on PCIe
> >> switches since before there was any EDR support in the kernel. It
> >> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
> >> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
> >> was even later. Its not immediately clear I would want to use EDR in
> >> my newer architecures & then there are also the older architecures
> >> still requiring support. When I submitted this patch I came at it
> >> with the approach of trying to keep the old behavior & still support
> >> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
> >> change the behavior for both root ports & switch ports, requiring
> >> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
> >> bit 7 (EDR) or a kernel command line value. I think no matter what,
> >> we want to ensure that PCIe Root Ports and PCIe switches arrive at
> >> the same policy here.
> > Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
> > control negotiation, but does *not* add the EDR requirement?
> >
> > I'm looking at
> > https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
> > seems to be the final "Downstream Port Containment Related
> > Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
> > Firmware spec r3.2.
> >
> > It adds bit 7, "PCI Express Downstream Port Containment Configuration
> > control", to the passed-in _OSC Control field, which indicates that
> > the OS supports both "native OS control and firmware ownership models
> > (i.e. Error Disconnect Recover notification) of Downstream Port
> > Containment."
> >
> > It also adds the dependency that "If the OS sets bit 7 of the Control
> > field, it must set bit 7 of the Support field, indicating support for
> > the Error Disconnect Recover event."
> >
> > So I'm trying to figure out if the "support DPC but not EDR" situation
> > was ever a valid place to be.  Maybe it's a mistake to have separate
> > CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.
> 
> My understanding is also similar. I have raised the same point in
> https://lore.kernel.org/all/3c02a6d6-917e-486c-ad41-bdf176639ff2@linux.intel.com/

Ah, sorry, I missed that.

> IMO, we don't need a separate config for EDR. I don't think user can
> gain anything with disabling EDR and enabling DPC. As long as
> firmware does not user EDR support, just compiling the code should
> be harmless.
> 
> So we can either remove it, or select it by default if user selects
> DPC config.
> 
> > CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
> > bit murky on non-ACPI systems that support DPC.
> 
> If we are going to remove the EDR config, it might need #ifdef
> CONFIG_ACPI changes in edr.c to not compile ACPI specific code.
> Alternative choice is to compile edr.c with CONFIG_ACPI.

Right.  I think we should probably remove CONFIG_PCIE_EDR completely
and make everything controlled by CONFIG_PCIE_DPC.

edr.c only provides two interfaces: pci_acpi_add_edr_notifier() and
pci_acpi_remove_edr_notifier(), which are only used by pci-acpi.c,
which is only compiled if CONFIG_ACPI, so we could probably also
compile edr.c only if CONFIG_ACPI.

And the declarations in include/linux/pci-acpi.h could probably be
moved to drivers/pci/pci.h since they're never used outside
drivers/pci/.

Bjorn