| Message ID | 1540483292-24049-2-git-send-email-asierra@xes-inc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Improve _OSC control request granularity | expand |
Hi Aaron, On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote: > Move the simple test for when PCIe native services are disabled > closer to the top, prior to where things get more complicated. > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > drivers/acpi/pci_root.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 707aafc..eb9f14e 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -441,6 +441,12 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > return; > } > > + if (pcie_ports_disabled) { > + dev_info(&device->dev, > + "PCIe port services disabled; not requesting _OSC control\n"); > + return; > + } Today we always set "*no_aspm = 1" if _OSC fails, which means we later call pcie_no_aspm(). After this patch, when pcie_ports_disabled is "true", we don't even try to evaluate _OSC, and we will never set *no_aspm, so we will never call pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these cases: 1) CONFIG_PCIEPORTBUS is unset, or 2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat" Case 1) isn't a problem because pcie_no_aspm() is only implemented when CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in this case today we only call the empty stub pcie_no_aspm() function. But case 2) is a behavior change that seems unintended. Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code is still active and useful even when we boot with "pcie_ports=compat". Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another question. I tend to think maybe it should not, but that's an orthogonal question. > /* > * All supported architectures that use ACPI have support for > * PCI domains, so we indicate this in _OSC support capabilities. > @@ -468,11 +474,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > return; > } > > - if (pcie_ports_disabled) { > - dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n"); > - return; > - } > - > if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) { > decode_osc_support(root, "not requesting OS control; OS requires", > ACPI_PCIE_REQ_SUPPORT); > -- > 2.7.4 >
----- Original Message ----- > From: "Bjorn Helgaas" <helgaas@kernel.org> > To: "Aaron Sierra" <asierra@xes-inc.com> > Sent: Wednesday, January 30, 2019 4:44:15 PM > Subject: Re: [PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top > Hi Aaron, > > On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote: >> Move the simple test for when PCIe native services are disabled >> closer to the top, prior to where things get more complicated. >> >> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> >> --- >> drivers/acpi/pci_root.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 707aafc..eb9f14e 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -441,6 +441,12 @@ static void negotiate_os_control(struct acpi_pci_root >> *root, int *no_aspm, >> return; >> } >> >> + if (pcie_ports_disabled) { >> + dev_info(&device->dev, >> + "PCIe port services disabled; not requesting _OSC control\n"); >> + return; >> + } > > Today we always set "*no_aspm = 1" if _OSC fails, which means we later > call pcie_no_aspm(). > > After this patch, when pcie_ports_disabled is "true", we don't even try to > evaluate _OSC, and we will never set *no_aspm, so we will never call > pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these > cases: > > 1) CONFIG_PCIEPORTBUS is unset, or > 2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat" > > Case 1) isn't a problem because pcie_no_aspm() is only implemented when > CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in > this case today we only call the empty stub pcie_no_aspm() function. > > But case 2) is a behavior change that seems unintended. > > Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't > actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code > is still active and useful even when we boot with "pcie_ports=compat". > > Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another > question. I tend to think maybe it should not, but that's an orthogonal > question. > Bjorn, thanks for the review. I certainly did not mean to change behavior to the extent that you describe. This patch is also not really needed by the second patch in the series, so I will drop this from v3. Sorry for the noise. -Aaron
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 707aafc..eb9f14e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -441,6 +441,12 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, return; } + if (pcie_ports_disabled) { + dev_info(&device->dev, + "PCIe port services disabled; not requesting _OSC control\n"); + return; + } + /* * All supported architectures that use ACPI have support for * PCI domains, so we indicate this in _OSC support capabilities. @@ -468,11 +474,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, return; } - if (pcie_ports_disabled) { - dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n"); - return; - } - if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) { decode_osc_support(root, "not requesting OS control; OS requires", ACPI_PCIE_REQ_SUPPORT);
Move the simple test for when PCIe native services are disabled closer to the top, prior to where things get more complicated. Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- drivers/acpi/pci_root.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)