linux-kernel - Re: [PATCH] PCI/ACPI: Decouple the negotiation of ASPM and other PCIe services

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220407154257.GA235990@bhelgaas>
Date:   Thu, 7 Apr 2022 10:42:57 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Yicong Yang <yangyicong@...ilicon.com>
Cc:     bhelgaas@...gle.com, rafael@...nel.org, lenb@...nel.org,
        linux-pci@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH] PCI/ACPI: Decouple the negotiation of ASPM and other
 PCIe services

[+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@google.com/

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@...ilicon.com>
> ---
>  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
>