[+cc Rafael] On Thu, Apr 07, 2022 at 09:16:02PM +0800, Yicong Yang wrote: > Currently we regard ASPM as a necessary PCIe service and if it's disabled > by pcie_aspm=off we cannot enable other services like AER and hotplug. > However the ASPM is just one of the PCIe services and other services > mentioned no dependency on this. So this patch decouples the negotiation > of ASPM and other PCIe services, then we can make use of other services > in the absence of ASPM. Why do you want to boot with "pcie_aspm=off"? If we have to use a PCI-related parameter to boot, something is already wrong, so if there's a problem that requires ASPM to be disabled, we should fix that first. If there's a known hardware or firmware issue with ASPM, we should quirk it so users don't have to discover this parameter. > Aaron Sierra tried to fix this originally: > https://lore.kernel.org/linux-pci/20190702201318.GC128603@xxxxxxxxxx/ Yes. My question from that review is still open: But Rafael added ACPI_PCIE_REQ_SUPPORT with 415e12b23792 ("PCI/ACPI: Request _OSC control once for each root bridge (v3)") [1], apparently related to a bug [2]. I assume there was some reason for requiring all those things together, so I'd really like his comments. [1] https://git.kernel.org/linus/415e12b23792 [2] https://bugzilla.kernel.org/show_bug.cgi?id=20232 Rafael clearly said in [1] that we need to: ... check if all of the requisite _OSC support bits are set before calling acpi_pci_osc_control_set() for a given root complex. We would still need to explain why Rafael thought all these _OSC support bits were required, but now they're not. _OSC does not negotiate directly for control of ASPM (though of course it *does* negotiate for control of the PCIe Capability, which contains the ASPM control bits), but the PCI Firmware spec, r3.3, sec 4.5.3, has this comment in a sample _OSC implementation: // Only allow native hot plug control if the OS supports: // * ASPM // * Clock PM // * MSI/MSI-X which matches the current ACPI_PCIE_REQ_SUPPORT. So I think I would approach this by reworking the _OSC negotiation so we always advertise "OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT" if CONFIG_PCIEASPM=y. Advertising support for ASPM doesn't mean Linux has to actually *enable* it, so we could make a different mechanism to prevent use of ASPM if we have a device or platform quirk or we're booting with "pcie_aspm=off". > Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx> > --- > drivers/acpi/pci_root.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 6f9e75d14808..16fa7c5a11ad 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -37,8 +37,6 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev) > } > > #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \ > - | OSC_PCI_ASPM_SUPPORT \ > - | OSC_PCI_CLOCK_PM_SUPPORT \ > | OSC_PCI_MSI_SUPPORT) > > static const struct acpi_device_id root_device_ids[] = { > -- > 2.24.0 >
