1. Patchew
  2. linux
  3. [PATCH v2 0/6] Fixups for s2idle on various Rembrandt laptops
  4. View patch

[PATCH v2 1/6] acpi/x86: s2idle: Move _HID handling for AMD systems into structures

Mario Limonciello posted 6 patches 3 years, 6 months ago
There is a newer version of this series
[PATCH v2 1/6] acpi/x86: s2idle: Move _HID handling for AMD systems into structures
Posted by Mario Limonciello 3 years, 6 months ago 
Right now the information about which cases to use for what are in a
comment, but this is error prone.  Instead move all information into
a dedicated structure.

Tested-by: catalin@antebit.com
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 63 ++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index f9ac12b778e6..a7757551f750 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -363,6 +363,39 @@ static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *d
 	return ret;
 }
 
+struct amd_lps0_hid_device_data {
+	const unsigned int rev_id;
+	const bool check_off_by_one;
+	const bool prefer_amd_guid;
+};
+
+static const struct amd_lps0_hid_device_data amd_picasso = {
+	.rev_id = 0,
+	.check_off_by_one = true,
+	.prefer_amd_guid = false,
+};
+
+static const struct amd_lps0_hid_device_data amd_cezanne = {
+	.rev_id = 0,
+	.check_off_by_one = false,
+	.prefer_amd_guid = false,
+};
+
+static const struct amd_lps0_hid_device_data amd_rembrandt = {
+	.rev_id = 2,
+	.check_off_by_one = false,
+	.prefer_amd_guid = true,
+};
+
+static const struct acpi_device_id amd_hid_ids[] = {
+	{"AMD0004",	(kernel_ulong_t)&amd_picasso,	},
+	{"AMD0005",	(kernel_ulong_t)&amd_picasso,	},
+	{"AMDI0005",	(kernel_ulong_t)&amd_picasso,	},
+	{"AMDI0006",	(kernel_ulong_t)&amd_cezanne,	},
+	{"AMDI0007",	(kernel_ulong_t)&amd_rembrandt,	},
+	{}
+};
+
 static int lps0_device_attach(struct acpi_device *adev,
 			      const struct acpi_device_id *not_used)
 {
@@ -370,31 +403,27 @@ static int lps0_device_attach(struct acpi_device *adev,
 		return 0;
 
 	if (acpi_s2idle_vendor_amd()) {
-		/* AMD0004, AMD0005, AMDI0005:
-		 * - Should use rev_id 0x0
-		 * - function mask > 0x3: Should use AMD method, but has off by one bug
-		 * - function mask = 0x3: Should use Microsoft method
-		 * AMDI0006:
-		 * - should use rev_id 0x0
-		 * - function mask = 0x3: Should use Microsoft method
-		 * AMDI0007:
-		 * - Should use rev_id 0x2
-		 * - Should only use AMD method
-		 */
-		const char *hid = acpi_device_hid(adev);
-		rev_id = strcmp(hid, "AMDI0007") ? 0 : 2;
+		static const struct acpi_device_id *dev_id;
+		const struct amd_lps0_hid_device_data *data;
+
+		for (dev_id = &amd_hid_ids[0]; dev_id->id[0]; dev_id++)
+			if (acpi_dev_hid_uid_match(adev, dev_id->id, NULL))
+				break;
+		if (dev_id != NULL)
+			data = (const struct amd_lps0_hid_device_data *) dev_id->driver_data;
+		else
+			return 0;
+		rev_id = data->rev_id;
 		lps0_dsm_func_mask = validate_dsm(adev->handle,
 					ACPI_LPS0_DSM_UUID_AMD, rev_id, &lps0_dsm_guid);
 		lps0_dsm_func_mask_microsoft = validate_dsm(adev->handle,
 					ACPI_LPS0_DSM_UUID_MICROSOFT, 0,
 					&lps0_dsm_guid_microsoft);
-		if (lps0_dsm_func_mask > 0x3 && (!strcmp(hid, "AMD0004") ||
-						 !strcmp(hid, "AMD0005") ||
-						 !strcmp(hid, "AMDI0005"))) {
+		if (lps0_dsm_func_mask > 0x3 && data->check_off_by_one) {
 			lps0_dsm_func_mask = (lps0_dsm_func_mask << 1) | 0x1;
 			acpi_handle_debug(adev->handle, "_DSM UUID %s: Adjusted function mask: 0x%x\n",
 					  ACPI_LPS0_DSM_UUID_AMD, lps0_dsm_func_mask);
-		} else if (lps0_dsm_func_mask_microsoft > 0 &&
+		} else if (lps0_dsm_func_mask_microsoft > 0 && data->prefer_amd_guid &&
 				(!strcmp(hid, "AMDI0007") ||
 				 !strcmp(hid, "AMDI0008"))) {
 			lps0_dsm_func_mask_microsoft = -EINVAL;
-- 
2.34.1
Re: [PATCH v2 1/6] acpi/x86: s2idle: Move _HID handling for AMD systems into structures
Posted by Philipp Zabel 3 years, 6 months ago 
Am Mon, Sep 12, 2022 at 12:23:55PM -0500 schrieb Mario Limonciello:
> Right now the information about which cases to use for what are in a
> comment, but this is error prone.  Instead move all information into
> a dedicated structure.
> 
> Tested-by: catalin@antebit.com
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/x86/s2idle.c | 63 ++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index f9ac12b778e6..a7757551f750 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -363,6 +363,39 @@ static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *d
>  	return ret;
>  }
>  
> +struct amd_lps0_hid_device_data {
> +	const unsigned int rev_id;
> +	const bool check_off_by_one;
> +	const bool prefer_amd_guid;
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_picasso = {
> +	.rev_id = 0,
> +	.check_off_by_one = true,
> +	.prefer_amd_guid = false,
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_cezanne = {
> +	.rev_id = 0,
> +	.check_off_by_one = false,
> +	.prefer_amd_guid = false,
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_rembrandt = {
> +	.rev_id = 2,
> +	.check_off_by_one = false,
> +	.prefer_amd_guid = true,
> +};
> +
> +static const struct acpi_device_id amd_hid_ids[] = {
> +	{"AMD0004",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMD0005",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMDI0005",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMDI0006",	(kernel_ulong_t)&amd_cezanne,	},
> +	{"AMDI0007",	(kernel_ulong_t)&amd_rembrandt,	},
> +	{}
> +};
> +
>  static int lps0_device_attach(struct acpi_device *adev,
>  			      const struct acpi_device_id *not_used)
>  {
> @@ -370,31 +403,27 @@ static int lps0_device_attach(struct acpi_device *adev,
>  		return 0;
>  
>  	if (acpi_s2idle_vendor_amd()) {
> -		/* AMD0004, AMD0005, AMDI0005:
> -		 * - Should use rev_id 0x0
> -		 * - function mask > 0x3: Should use AMD method, but has off by one bug
> -		 * - function mask = 0x3: Should use Microsoft method
> -		 * AMDI0006:
> -		 * - should use rev_id 0x0
> -		 * - function mask = 0x3: Should use Microsoft method
> -		 * AMDI0007:
> -		 * - Should use rev_id 0x2
> -		 * - Should only use AMD method
> -		 */
> -		const char *hid = acpi_device_hid(adev);
> -		rev_id = strcmp(hid, "AMDI0007") ? 0 : 2;
> +		static const struct acpi_device_id *dev_id;
> +		const struct amd_lps0_hid_device_data *data;
> +
> +		for (dev_id = &amd_hid_ids[0]; dev_id->id[0]; dev_id++)
> +			if (acpi_dev_hid_uid_match(adev, dev_id->id, NULL))
> +				break;
> +		if (dev_id != NULL)
> +			data = (const struct amd_lps0_hid_device_data *) dev_id->driver_data;
> +		else
> +			return 0;

The "!= NULL" seems unnecessary, I would change this to:

+		if (!dev_id)
+			return 0;
+		data = (const struct amd_lps0_hid_device_data *) dev_id->driver_data;

But either way,

Reviewed-by: Philipp Zabel <philipp.zabel@gmail.com>
Tested-by: Philipp Zabel <philipp.zabel@gmail.com> # GA402RJ

regards
Philipp

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.