We have some hardware that takes about 30 seconds to setup common clocks for
ASPM, but our bios'es don't actually allow ASPM. It seems we had this thing in
place where we would disable ASPM after the pci bus probe so that we would make
sure that pre pcie 1.1 devices would be properly skipped during initialization.
This is because the mechanism to disable ASPM doesn't actually disable the
setting up of the link state stuff, it just keeps us from changing the link
state after the fact. So instead make it so that when we call pcie_no_aspm()
that we disable ASPM completley, that is we skip setting up the link state and
everything. This way we avoid the costly setup for a feature we cannot support
in the first place and we also make sure we are safe from future tampering with
the ASPM link state. Thanks,
Signed-off-by: Josef Bacik <[email protected]>
---
drivers/acpi/pci_root.c | 22 +++++-----------------
drivers/pci/pcie/aspm.c | 1 +
2 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 393706a..00e164a 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -419,7 +419,7 @@ out:
}
EXPORT_SYMBOL(acpi_pci_osc_control_set);
-static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
+static void negotiate_os_control(struct acpi_pci_root *root)
{
u32 support, control, requested;
acpi_status status;
@@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
if (ACPI_FAILURE(status)) {
dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
acpi_format_exception(status));
- *no_aspm = 1;
+ pcie_no_aspm();
return;
}
@@ -495,22 +495,14 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
* intact and prevent the OS from touching it.
*/
dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
- *no_aspm = 1;
+ pcie_no_aspm();
}
} else {
decode_osc_control(root, "OS requested", requested);
decode_osc_control(root, "platform willing to grant", control);
dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
acpi_format_exception(status));
-
- /*
- * We want to disable ASPM here, but aspm_disabled
- * needs to remain in its state from boot so that we
- * properly handle PCIe 1.1 devices. So we set this
- * flag here, to defer the action until after the ACPI
- * root scan.
- */
- *no_aspm = 1;
+ pcie_no_aspm();
}
}
@@ -522,7 +514,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
int result;
struct acpi_pci_root *root;
acpi_handle handle = device->handle;
- int no_aspm = 0;
bool hotadd = system_state != SYSTEM_BOOTING;
root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
@@ -581,7 +572,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
- negotiate_os_control(root, &no_aspm);
+ negotiate_os_control(root);
/*
* TBD: Need PCI interface for enumeration/configuration of roots.
@@ -604,9 +595,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
goto remove_dmar;
}
- if (no_aspm)
- pcie_no_aspm();
-
pci_acpi_add_bus_pm_notifier(device);
if (device->wakeup.flags.run_wake)
device_set_run_wake(root->bus->bridge, true);
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 317e355..5f84af2 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -954,6 +954,7 @@ void pcie_no_aspm(void)
if (!aspm_force) {
aspm_policy = POLICY_DEFAULT;
aspm_disabled = 1;
+ aspm_support_enabled = false;
}
}
--
2.1.0
[+cc Matthew]
Hi Josef,
On Wed, Nov 18, 2015 at 09:25:03AM -0500, Josef Bacik wrote:
> We have some hardware that takes about 30 seconds to setup common clocks for
> ASPM, but our bios'es don't actually allow ASPM. It seems we had this thing in
> place where we would disable ASPM after the pci bus probe so that we would make
> sure that pre pcie 1.1 devices would be properly skipped during initialization.
> This is because the mechanism to disable ASPM doesn't actually disable the
> setting up of the link state stuff, it just keeps us from changing the link
> state after the fact. So instead make it so that when we call pcie_no_aspm()
> that we disable ASPM completley, that is we skip setting up the link state and
> everything. This way we avoid the costly setup for a feature we cannot support
> in the first place and we also make sure we are safe from future tampering with
> the ASPM link state. Thanks,
Can you clarify what your BIOS does? I assume it's an x86 ACPI BIOS.
Does it enable ASPM before handing off to the OS? Does it set the
ACPI_FADT_NO_ASPM bit? (ACPI spec 5.0, sec 5.2.9.3)
You're changing the _OSC failure path. Why does _OSC fail? I know
there is some sort of unresolved problem there
(https://bugzilla.kernel.org/show_bug.cgi?id=94661); maybe you're
tripping over that?
It seems like you're proposing to reverse what Matthew just did with
387d37577fdd ("PCI: Don't clear ASPM bits when the FADT declares it's
unsupported").
Bjorn
> Signed-off-by: Josef Bacik <[email protected]>
> ---
> drivers/acpi/pci_root.c | 22 +++++-----------------
> drivers/pci/pcie/aspm.c | 1 +
> 2 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 393706a..00e164a 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -419,7 +419,7 @@ out:
> }
> EXPORT_SYMBOL(acpi_pci_osc_control_set);
>
> -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> +static void negotiate_os_control(struct acpi_pci_root *root)
> {
> u32 support, control, requested;
> acpi_status status;
> @@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> if (ACPI_FAILURE(status)) {
> dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
> acpi_format_exception(status));
> - *no_aspm = 1;
> + pcie_no_aspm();
> return;
> }
>
> @@ -495,22 +495,14 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> * intact and prevent the OS from touching it.
> */
> dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
> - *no_aspm = 1;
> + pcie_no_aspm();
> }
> } else {
> decode_osc_control(root, "OS requested", requested);
> decode_osc_control(root, "platform willing to grant", control);
> dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
> acpi_format_exception(status));
> -
> - /*
> - * We want to disable ASPM here, but aspm_disabled
> - * needs to remain in its state from boot so that we
> - * properly handle PCIe 1.1 devices. So we set this
> - * flag here, to defer the action until after the ACPI
> - * root scan.
> - */
> - *no_aspm = 1;
> + pcie_no_aspm();
> }
> }
>
> @@ -522,7 +514,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
> int result;
> struct acpi_pci_root *root;
> acpi_handle handle = device->handle;
> - int no_aspm = 0;
> bool hotadd = system_state != SYSTEM_BOOTING;
>
> root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
> @@ -581,7 +572,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
>
> root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
>
> - negotiate_os_control(root, &no_aspm);
> + negotiate_os_control(root);
>
> /*
> * TBD: Need PCI interface for enumeration/configuration of roots.
> @@ -604,9 +595,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
> goto remove_dmar;
> }
>
> - if (no_aspm)
> - pcie_no_aspm();
> -
> pci_acpi_add_bus_pm_notifier(device);
> if (device->wakeup.flags.run_wake)
> device_set_run_wake(root->bus->bridge, true);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 317e355..5f84af2 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -954,6 +954,7 @@ void pcie_no_aspm(void)
> if (!aspm_force) {
> aspm_policy = POLICY_DEFAULT;
> aspm_disabled = 1;
> + aspm_support_enabled = false;
> }
> }
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/18/2015 01:40 PM, Bjorn Helgaas wrote:
> [+cc Matthew]
>
> Hi Josef,
>
> On Wed, Nov 18, 2015 at 09:25:03AM -0500, Josef Bacik wrote:
>> We have some hardware that takes about 30 seconds to setup common clocks for
>> ASPM, but our bios'es don't actually allow ASPM. It seems we had this thing in
>> place where we would disable ASPM after the pci bus probe so that we would make
>> sure that pre pcie 1.1 devices would be properly skipped during initialization.
>> This is because the mechanism to disable ASPM doesn't actually disable the
>> setting up of the link state stuff, it just keeps us from changing the link
>> state after the fact. So instead make it so that when we call pcie_no_aspm()
>> that we disable ASPM completley, that is we skip setting up the link state and
>> everything. This way we avoid the costly setup for a feature we cannot support
>> in the first place and we also make sure we are safe from future tampering with
>> the ASPM link state. Thanks,
>
> Can you clarify what your BIOS does? I assume it's an x86 ACPI BIOS.
> Does it enable ASPM before handing off to the OS? Does it set the
> ACPI_FADT_NO_ASPM bit? (ACPI spec 5.0, sec 5.2.9.3)
>
> You're changing the _OSC failure path. Why does _OSC fail? I know
> there is some sort of unresolved problem there
> (https://bugzilla.kernel.org/show_bug.cgi?id=94661); maybe you're
> tripping over that?
>
This is what I see in dmesg
[ 8.647398] acpi PNP0A03:00: _OSC: OS supports [ExtendedConfig ASPM
ClockPM Segments MSI]
[ 8.647716] acpi PNP0A03:00: _OSC failed (AE_NOT_FOUND); disabling ASPM
So seems like the same bug?
> It seems like you're proposing to reverse what Matthew just did with
> 387d37577fdd ("PCI: Don't clear ASPM bits when the FADT declares it's
> unsupported").
>
It looks to me I'm doing what Matthew set out to do, only with a bigger
hammer ;). His patch still allowed the initialization of the ASPM
stuff, like setting up the clocks and such, but then made it so we
couldn't change the ASPM state at all. My patch goes the step further
of not even doing the initialization stuff. Thanks,
Josef
On Wed, Nov 18, 2015 at 11:00 AM, Josef Bacik <[email protected]> wrote:
> It looks to me I'm doing what Matthew set out to do, only with a bigger
> hammer ;). His patch still allowed the initialization of the ASPM stuff,
> like setting up the clocks and such, but then made it so we couldn't change
> the ASPM state at all. My patch goes the step further of not even doing the
> initialization stuff. Thanks,
That sounds fine - if we don't gain _OSC control then we should avoid
touching any of the ASPM register state that the firmware has set. I'm
happy to call the current state broken.
On Wed, Nov 18, 2015 at 09:25:03AM -0500, Josef Bacik wrote:
> We have some hardware that takes about 30 seconds to setup common clocks for
> ASPM, but our bios'es don't actually allow ASPM. It seems we had this thing in
> place where we would disable ASPM after the pci bus probe so that we would make
> sure that pre pcie 1.1 devices would be properly skipped during initialization.
> This is because the mechanism to disable ASPM doesn't actually disable the
> setting up of the link state stuff, it just keeps us from changing the link
> state after the fact. So instead make it so that when we call pcie_no_aspm()
> that we disable ASPM completley, that is we skip setting up the link state and
> everything. This way we avoid the costly setup for a feature we cannot support
> in the first place and we also make sure we are safe from future tampering with
> the ASPM link state. Thanks,
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 317e355..5f84af2 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -954,6 +954,7 @@ void pcie_no_aspm(void)
> if (!aspm_force) {
> aspm_policy = POLICY_DEFAULT;
> aspm_disabled = 1;
> + aspm_support_enabled = false;
After this patch, I think aspm_disabled and aspm_support_enabled are
equivalent. If that's the case, we should get rid of one of them.
> }
> }
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi!
> We have some hardware that takes about 30 seconds to setup common clocks for
> ASPM, but our bios'es don't actually allow ASPM. It seems we had this thing in
> place where we would disable ASPM after the pci bus probe so that we would make
> sure that pre pcie 1.1 devices would be properly skipped during initialization.
> This is because the mechanism to disable ASPM doesn't actually disable the
> setting up of the link state stuff, it just keeps us from changing the link
> state after the fact. So instead make it so that when we call pcie_no_aspm()
> that we disable ASPM completley, that is we skip setting up the link state and
> everything. This way we avoid the costly setup for a feature we cannot support
> in the first place and we also make sure we are safe from future tampering with
> the ASPM link state. Thanks,
On thinkpad x60, I have similar problem.
I'm carying this for long long time now:
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a32ba75..5eb7621 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -700,7 +700,7 @@ static int __init acpi_pci_init(void)
if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
pr_info("ACPI FADT declares the system doesn't support PCIe ASPM, so disable it\n");
- pcie_no_aspm();
+// pcie_no_aspm();
}
ret = register_acpi_bus_type(&acpi_pci_bus);
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html