From: Joerg Roedel <[email protected]>
Get rid of acpi_pci_osc_support() and check for _OSC supported features
directly in acpi_pci_osc_control_set(). There is no point in doing an
unconditional _OSC query with control=0 even when the kernel later wants
to take control over more features.
This safes one _OSC query and simplifies the code by getting rid of
the acpi_pci_osc_support() function. As a side effect, the !control
checks in acpi_pci_query_osc() can also be removed.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/acpi/pci_root.c | 81 ++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 49 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f12e512bcddc..ab2f7dfb0c44 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -203,26 +203,16 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
capbuf[OSC_SUPPORT_DWORD] = support;
- if (control)
- capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
- else
- /* Run _OSC query only with existing controls. */
- capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
+ capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
if (ACPI_SUCCESS(status)) {
root->osc_support_set = support;
- if (control)
- *control = result;
+ *control = result;
}
return status;
}
-static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
-{
- return acpi_pci_query_osc(root, flags, NULL);
-}
-
struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
{
struct acpi_pci_root *root;
@@ -345,8 +335,9 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
* _OSC bits the BIOS has granted control of, but its contents are meaningless
* on failure.
**/
-static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
+static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support)
{
+ u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
struct acpi_pci_root *root;
acpi_status status;
u32 ctrl, capbuf[3];
@@ -354,22 +345,16 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
if (!mask)
return AE_BAD_PARAMETER;
- ctrl = *mask;
- if ((ctrl & req) != req)
- return AE_TYPE;
-
root = acpi_pci_find_root(handle);
if (!root)
return AE_NOT_EXIST;
- *mask = ctrl | root->osc_control_set;
- /* No need to evaluate _OSC if the control was already granted. */
- if ((root->osc_control_set & ctrl) == ctrl)
- return AE_OK;
+ ctrl = *mask;
+ *mask |= root->osc_control_set;
/* Need to check the available controls bits before requesting them. */
- while (*mask) {
- status = acpi_pci_query_osc(root, root->osc_support_set, mask);
+ do {
+ status = acpi_pci_query_osc(root, support, mask);
if (ACPI_FAILURE(status))
return status;
if (ctrl == *mask)
@@ -377,7 +362,11 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
decode_osc_control(root, "platform does not support",
ctrl & ~(*mask));
ctrl = *mask;
- }
+ } while (*mask);
+
+ /* No need to request _OSC if the control was already granted. */
+ if ((root->osc_control_set & ctrl) == ctrl)
+ return AE_OK;
if ((ctrl & req) != req) {
decode_osc_control(root, "not requesting control; platform does not support",
@@ -470,7 +459,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
bool is_pcie)
{
- u32 support, control, requested;
+ u32 support, control = 0, requested = 0;
acpi_status status;
struct acpi_device *device = root->device;
acpi_handle handle = device->handle;
@@ -490,28 +479,15 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
support = calculate_support();
decode_osc_support(root, "OS supports", support);
- status = acpi_pci_osc_support(root, support);
- if (ACPI_FAILURE(status)) {
- *no_aspm = 1;
-
- /* _OSC is optional for PCI host bridges */
- if ((status == AE_NOT_FOUND) && !is_pcie)
- return;
-
- dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
- acpi_format_exception(status));
- return;
- }
-
- if (!os_control_query_checks(root, support))
- return;
- requested = control = calculate_control();
+ if (os_control_query_checks(root, support))
+ requested = control = calculate_control();
- status = acpi_pci_osc_control_set(handle, &control,
- OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
+ status = acpi_pci_osc_control_set(handle, &control, support);
if (ACPI_SUCCESS(status)) {
- decode_osc_control(root, "OS now controls", control);
+ if (control)
+ decode_osc_control(root, "OS now controls", control);
+
if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
/*
* We have ASPM control, but the FADT indicates that
@@ -522,11 +498,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
*no_aspm = 1;
}
} else {
- decode_osc_control(root, "OS requested", requested);
- decode_osc_control(root, "platform willing to grant", control);
- dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\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
@@ -535,6 +506,18 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
* root scan.
*/
*no_aspm = 1;
+
+ /* _OSC is optional for PCI host bridges */
+ if ((status == AE_NOT_FOUND) && !is_pcie)
+ return;
+
+ if (control) {
+ decode_osc_control(root, "OS requested", requested);
+ decode_osc_control(root, "platform willing to grant", control);
+ }
+
+ dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
+ acpi_format_exception(status));
}
}
--
2.32.0
On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <[email protected]> wrote:
>
> From: Joerg Roedel <[email protected]>
>
> Get rid of acpi_pci_osc_support() and check for _OSC supported features
> directly in acpi_pci_osc_control_set(). There is no point in doing an
> unconditional _OSC query with control=0 even when the kernel later wants
> to take control over more features.
>
> This safes one _OSC query and simplifies the code by getting rid of
s/safes/saves/
> the acpi_pci_osc_support() function. As a side effect, the !control
> checks in acpi_pci_query_osc() can also be removed.
>
> Signed-off-by: Joerg Roedel <[email protected]>
Apart from the typo above, I haven't found any issues in the patch, so
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/pci_root.c | 81 ++++++++++++++++-------------------------
> 1 file changed, 32 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index f12e512bcddc..ab2f7dfb0c44 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -203,26 +203,16 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>
> capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> capbuf[OSC_SUPPORT_DWORD] = support;
> - if (control)
> - capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
> - else
> - /* Run _OSC query only with existing controls. */
> - capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
> + capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
>
> status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
> if (ACPI_SUCCESS(status)) {
> root->osc_support_set = support;
> - if (control)
> - *control = result;
> + *control = result;
> }
> return status;
> }
>
> -static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
> -{
> - return acpi_pci_query_osc(root, flags, NULL);
> -}
> -
> struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
> {
> struct acpi_pci_root *root;
> @@ -345,8 +335,9 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
> * _OSC bits the BIOS has granted control of, but its contents are meaningless
> * on failure.
> **/
> -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
> +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support)
> {
> + u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
> struct acpi_pci_root *root;
> acpi_status status;
> u32 ctrl, capbuf[3];
> @@ -354,22 +345,16 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
> if (!mask)
> return AE_BAD_PARAMETER;
>
> - ctrl = *mask;
> - if ((ctrl & req) != req)
> - return AE_TYPE;
> -
> root = acpi_pci_find_root(handle);
> if (!root)
> return AE_NOT_EXIST;
>
> - *mask = ctrl | root->osc_control_set;
> - /* No need to evaluate _OSC if the control was already granted. */
> - if ((root->osc_control_set & ctrl) == ctrl)
> - return AE_OK;
> + ctrl = *mask;
> + *mask |= root->osc_control_set;
>
> /* Need to check the available controls bits before requesting them. */
> - while (*mask) {
> - status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> + do {
> + status = acpi_pci_query_osc(root, support, mask);
> if (ACPI_FAILURE(status))
> return status;
> if (ctrl == *mask)
> @@ -377,7 +362,11 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
> decode_osc_control(root, "platform does not support",
> ctrl & ~(*mask));
> ctrl = *mask;
> - }
> + } while (*mask);
> +
> + /* No need to request _OSC if the control was already granted. */
> + if ((root->osc_control_set & ctrl) == ctrl)
> + return AE_OK;
>
> if ((ctrl & req) != req) {
> decode_osc_control(root, "not requesting control; platform does not support",
> @@ -470,7 +459,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
> static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> bool is_pcie)
> {
> - u32 support, control, requested;
> + u32 support, control = 0, requested = 0;
> acpi_status status;
> struct acpi_device *device = root->device;
> acpi_handle handle = device->handle;
> @@ -490,28 +479,15 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> support = calculate_support();
>
> decode_osc_support(root, "OS supports", support);
> - status = acpi_pci_osc_support(root, support);
> - if (ACPI_FAILURE(status)) {
> - *no_aspm = 1;
> -
> - /* _OSC is optional for PCI host bridges */
> - if ((status == AE_NOT_FOUND) && !is_pcie)
> - return;
> -
> - dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> - acpi_format_exception(status));
> - return;
> - }
> -
> - if (!os_control_query_checks(root, support))
> - return;
>
> - requested = control = calculate_control();
> + if (os_control_query_checks(root, support))
> + requested = control = calculate_control();
>
> - status = acpi_pci_osc_control_set(handle, &control,
> - OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> + status = acpi_pci_osc_control_set(handle, &control, support);
> if (ACPI_SUCCESS(status)) {
> - decode_osc_control(root, "OS now controls", control);
> + if (control)
> + decode_osc_control(root, "OS now controls", control);
> +
> if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
> /*
> * We have ASPM control, but the FADT indicates that
> @@ -522,11 +498,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> *no_aspm = 1;
> }
> } else {
> - decode_osc_control(root, "OS requested", requested);
> - decode_osc_control(root, "platform willing to grant", control);
> - dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\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
> @@ -535,6 +506,18 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> * root scan.
> */
> *no_aspm = 1;
> +
> + /* _OSC is optional for PCI host bridges */
> + if ((status == AE_NOT_FOUND) && !is_pcie)
> + return;
> +
> + if (control) {
> + decode_osc_control(root, "OS requested", requested);
> + decode_osc_control(root, "platform willing to grant", control);
> + }
> +
> + dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> + acpi_format_exception(status));
> }
> }
>
> --
> 2.32.0
>