Re: [PATCH v14 3/4] PCI/ACPI: Centralize pcie_ports_native checking

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Oct 01, 2026 at 02:48:46PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 10/1/2026 1:54 PM, Bjorn Helgaas wrote:
> > On Tue, Sep 22, 2026 at 01:45:47PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >> If the user booted with "pcie_ports=native", we take control of the PCIe
> >> port services unconditionally, regardless of what _OSC says.
> >>
> >> Centralize the testing of pcie_ports_native in acpi_pci_root_create(),
> >> where we interpret the _OSC results, so other places only have to check
> >> host_bridge->native_X and we don't have to sprinkle tests of
> >> pcie_ports_native everywhere.
> >>
> >> Rather than overriding the host_bridge->native_X flags after the fact,
> >> fold "pcie_ports=native" into the _OSC control mask we evaluate, i.e.,
> >> proceed as though the platform had granted control of the port services.
> >> That way there is a single mechanism deciding each native_X flag, and we
> >> can report exactly which features we are overriding _OSC for instead of
> >> just noting that we are overriding something:
> >>
> >>   acpi PNP0A08:00: _OSC: OS forcing control ("pcie_ports=native") of [PCIeHotplug PME AER DPC]
> >>
> >> Forcing host_bridge->native_aer also restores DPC service binding under
> >> "pcie_ports=native", which regressed in commit 97ca178c899d ("PCI/DPC:
> >> Allow DPC on all Downstream Ports when OS controls AER").  That commit
> >> replaced the AER service test in get_port_device_capability() with
> >> host->native_aer, so DPC stayed off when host_bridge->native_aer was not
> >> set.
> >>
> >> This also extends "pcie_ports=native" to host_bridge->native_dpc, which
> >> had no pcie_ports_native fallback before.  That effect is narrow.
> >> native_dpc is only consumed by pci_dpc_recovered(), where with
> >> CONFIG_PCIE_EDR=n it previously gave up immediately and now lets pciehp
> >> wait for DPC recovery before treating a Link Down as a hotplug event.
> >>
> >> host_bridge->native_ltr is deliberately not forced.  "pcie_ports="
> >> controls PCIe port services and LTR is not one.  There is no
> >> PCIE_PORT_SERVICE_LTR, and native_ltr is only used by
> >> pci_configure_ltr() to enable ASPM L1.2, so forcing it would be an ASPM
> >> policy decision users did not ask for.
> >>
> >> SHPC hotplug is left alone for a simpler reason: SHPC is a conventional
> >> PCI feature rather than a PCIe one, so "pcie_ports=" has no bearing on
> >> it.
> ...

> >> +	/*
> >> +	 * If the user specified "pcie_ports=native", use the PCIe port
> >> +	 * services regardless of what _OSC says, i.e., proceed as though the
> >> +	 * platform had granted us control of them.  This may conflict with
> >> +	 * firmware that expects to own those features.
> >> +	 */
> >> +	if (pcie_ports_native) {
> >> +		u32 override = OSC_PCIE_PORT_SERVICE_CONTROLS & ~ctrl;
> >> +
> >> +		if (override)
> >> +			decode_osc_control(root, "OS forcing control (\"pcie_ports=native\") of",
> >> +					   override);
> >> +		ctrl |= override;
> >> +	}
> >> +
> >>  	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_NATIVE_HP_CONTROL,
> >>  		  host_bridge->native_pcie_hotplug);
> >>  	OSC_OWNER(ctrl, OSC_PCI_SHPC_NATIVE_HP_CONTROL,
> >> @@ -1051,6 +1079,14 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >>  	OSC_OWNER(ext_ctrl, OSC_CXL_ERROR_REPORTING_CONTROL,
> >>  		  host_bridge->native_cxl_error);
> >>  
> >> +	dev_info(&root->device->dev, "OS native features: SHPCHotplug%c PCIeHotplug%c PME%c AER%c DPC%c LTR%c\n",
> >> +		 FLAG(host_bridge->native_shpc_hotplug),
> >> +		 FLAG(host_bridge->native_pcie_hotplug),
> >> +		 FLAG(host_bridge->native_pme),
> >> +		 FLAG(host_bridge->native_aer),
> >> +		 FLAG(host_bridge->native_dpc),
> > 
> > I think the native_dpc printed here is inaccurate if booted with
> > "pci=dpc-native", isn't it?
> 
> Yes, it is inaccurate.  "pcie_ports=dpc-native" has never set
> native_dpc, but this patch is the first to print it, so I will fix it
> here.
> 
> I will fold it into the same override block, so both parameters go
> through the control mask and get logged the same way, something like:
> 
> 	u32 override = 0;
> 
> 	if (pcie_ports_native)
> 		override = OSC_PCIE_PORT_SERVICE_CONTROLS & ~ctrl;
> 	else if (pcie_ports_dpc_native)
> 		override = OSC_PCI_EXPRESS_DPC_CONTROL & ~ctrl;
> 
> 	if (override) {
> 		decode_osc_control(root, pcie_ports_native ?
> 				   "OS forcing control (\"pcie_ports=native\") of" :
> 				   "OS forcing control (\"pcie_ports=dpc-native\") of",
> 				   override);
> 		ctrl |= override;
> 	}

Sounds good.

> > The DPC probe handling still seems kind of convoluted.  In portdrv.c,
> > we have this:
> > 
> >   get_port_device_capability()
> >   {
> >     if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> > 	pci_aer_available() &&
> > 	(pcie_ports_dpc_native || host->native_aer))
> > 	    services |= PCIE_PORT_SERVICE_DPC;
> > 
> > And then we have this in dpc.c:
> > 
> >   dpc_probe()
> >   {
> >     if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
> > 	    return -ENOTSUPP;
> > 
> > The combination of get_port_device_capability() and dpc_probe() makes
> > my head hurt.
> > 
> > It seems like the DPC clause of get_port_device_capability() should be
> > parallel to the AER clause, i.e., set PCIE_PORT_SERVICE_DPC if the
> > device has a DPC Capability and the OS owns DPC (host->native_dpc).
> > Anything other than that needs explanation.
> > 
> > I'm confused about why host->native_dpc is not involved in the
> > dpc_probe() path at all.
> 
> Agreed, DPC should be decided on its own, independent of AER.  Today it
> is tied to AER in several places:
> 
> 1. portdrv binds DPC on host->native_aer, not native_dpc, following
>    the PCIe r7.0 sec 6.2.11 implementation note that links DPC control
>    to AER control (97ca178c899d).
> 2. The pci_aer_available() test in the same clause means "pci=noaer"
>    also disables DPC.
> 3. dpc_probe() repeats the ownership check via pcie_aer_is_native(),
>    which also tests dev->aer_cap, so DPC never binds on ports without
>    an AER Capability.
> 4. native_dpc is only consumed by pci_dpc_recovered().
> 
> With the change above, both "pcie_ports=native" and
> "pcie_ports=dpc-native" set native_dpc, so binding on native_dpc would
> work for them.  The problem is the default case, with no "pcie_ports="
> parameter.  We only request OSC_PCI_EXPRESS_DPC_CONTROL when
> CONFIG_PCIE_EDR=y, as the PCI Firmware spec requires, so on an EDR=n
> kernel native_dpc is never set on ACPI systems.

Right, I forgot about that.  This deserves a comment in the code
somewhere.

I think we still need to be able to use DPC without EDR on non-ACPI
systems.

> Today DPC still binds there through native_aer, and switching
> portdrv to test native_dpc alone would silently turn DPC off on
> those systems.

It would be nice if we could make native_dpc be the end result of AER,
EDR, and anything else that determines whether the OS can actually use
DPC.

> I think the simplest fix is to make PCIE_DPC "select PCIE_EDR if ACPI",
> so an ACPI kernel with DPC always negotiates DPC control and native_dpc
> reflects what firmware actually granted.  Then portdrv can bind DPC on
> "DPC Capability && native_dpc", just like the AER clause.
> 
> Lukas has posted a series that removes the DPC/AER dependency:
> 
>   https://lore.kernel.org/r/cover.1790531238.git.lukas@wunner.de
> 
> His 1/7 lets DPC work without an AER Capability, and 2/7 removes the
> dpc_probe() check entirely, so portdrv becomes the only place that
> decides.  The native_dpc change fits naturally on top of that.
> 
> Would you like me to do the native_dpc change in this series, or as a
> separate patch on top of Lukas's series?  If we do it as a separate
> patch on top of Lukas's work, this series can stay a cleanup (plus the
> fix for Guixin's issue).

Doing it on top of Lukas's series sounds good to me.