[PATCH v2 00/15] Add support for binding ACPI platform profile to multiple drivers

[PATCH v2 00/15] Add support for binding ACPI platform profile to multiple drivers

[Mario Limonciello]

Currently there are a number of ASUS products on the market that happen to
have ACPI objects for amd-pmf to bind to as well as an ACPI platform profile
provided by asus-wmi.

The ACPI platform profile support created by amd-pmf on these ASUS products is "Function 9"
which is specifically for "BIOS or EC notification" of power slider position.
This feature is actively used by some designs such as Framework 13 and Framework 16.

On these ASUS designs we keep on quirking more and more of them to turn off this
notification so that asus-wmi can bind.

This however isn't how Windows works.  "Multiple" things are notified for the power
slider position. This series adjusts Linux to behave similarly.

Multiple drivers can now register an ACPI platform profile and will react to set requests.

To avoid chaos, only positions that are common to both drivers are accepted.

This also allows dropping all of the PMF quirks from amd-pmf.

v2:
 * Split to many more patches
 * Account for feedback from M/L

Mario Limonciello (15):
  ACPI: platform-profile: Add a name member to handlers
  platform/surface: aggregator: Add platform handler pointer to device
  ACPI: platform_profile: Add platform handler argument to
    platform_profile_remove()
  ACPI: platform_profile: Add a list to platform profile handler
  ACPI: platform_profile: Move sanity check out of the mutex
  ACPI: platform_profile: Use guard(mutex) for register/unregister
  ACPI: platform_profile: Only remove group when no more handler
    registered
  ACPI: platform_profile: Require handlers to support balanced profile
  ACPI: platform_profile: Notify change events on register and
    unregister
  ACPI: platform_profile: Only show profiles common for all handlers
  ACPI: platform_profile: Set profile for all registered handlers
  ACPI: platform_profile: Make sure all profile handlers agree on
    profile
  ACPI: platform_profile: Check all profile handler to calculate next
  ACPI: platform_profile: Allow multiple handlers
  platform/x86/amd: pmf: Drop all quirks

 drivers/acpi/platform_profile.c               | 258 +++++++++++-------
 .../surface/surface_platform_profile.c        |   7 +-
 drivers/platform/x86/acer-wmi.c               |   5 +-
 drivers/platform/x86/amd/pmf/Makefile         |   2 +-
 drivers/platform/x86/amd/pmf/core.c           |   1 -
 drivers/platform/x86/amd/pmf/pmf-quirks.c     |  66 -----
 drivers/platform/x86/amd/pmf/pmf.h            |   3 -
 drivers/platform/x86/amd/pmf/sps.c            |   3 +-
 drivers/platform/x86/asus-wmi.c               |   5 +-
 drivers/platform/x86/dell/dell-pc.c           |   3 +-
 drivers/platform/x86/hp/hp-wmi.c              |   3 +-
 drivers/platform/x86/ideapad-laptop.c         |   3 +-
 .../platform/x86/inspur_platform_profile.c    |   5 +-
 drivers/platform/x86/thinkpad_acpi.c          |   3 +-
 include/linux/platform_profile.h              |   4 +-
 15 files changed, 190 insertions(+), 181 deletions(-)
 delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c

-- 
2.43.0

[PATCH v2 01/15] ACPI: platform-profile: Add a name member to handlers

[Mario Limonciello]

In order to prepare for allowing multiple handlers, introduce
a name field that can be used to distinguish between different
handlers.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/surface/surface_platform_profile.c | 1 +
 drivers/platform/x86/acer-wmi.c                     | 1 +
 drivers/platform/x86/amd/pmf/sps.c                  | 1 +
 drivers/platform/x86/asus-wmi.c                     | 1 +
 drivers/platform/x86/dell/dell-pc.c                 | 1 +
 drivers/platform/x86/hp/hp-wmi.c                    | 1 +
 drivers/platform/x86/ideapad-laptop.c               | 1 +
 drivers/platform/x86/inspur_platform_profile.c      | 1 +
 drivers/platform/x86/thinkpad_acpi.c                | 1 +
 include/linux/platform_profile.h                    | 1 +
 10 files changed, 10 insertions(+)

diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 3de864bc66108..61aa488a80eb5 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -211,6 +211,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 
 	tpd->sdev = sdev;
 
+	tpd->handler.name = "Surface Platform Profile";
 	tpd->handler.profile_get = ssam_platform_profile_get;
 	tpd->handler.profile_set = ssam_platform_profile_set;
 
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index d09baa3d3d902..53fbc9b4d3df7 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1878,6 +1878,7 @@ static int acer_platform_profile_setup(void)
 	if (quirks->predator_v4) {
 		int err;
 
+		platform_profile_handler.name = "acer-wmi";
 		platform_profile_handler.profile_get =
 			acer_predator_v4_platform_profile_get;
 		platform_profile_handler.profile_set =
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 92f7fb22277dc..e2d0cc92c4396 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -405,6 +405,7 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 		amd_pmf_set_sps_power_limits(dev);
 	}
 
+	dev->pprof.name = "amd-pmf";
 	dev->pprof.profile_get = amd_pmf_profile_get;
 	dev->pprof.profile_set = amd_pmf_profile_set;
 
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2ccc23b259d3e..c7c104c65a85a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3910,6 +3910,7 @@ static int platform_profile_setup(struct asus_wmi *asus)
 
 	dev_info(dev, "Using throttle_thermal_policy for platform_profile support\n");
 
+	asus->platform_profile_handler.name = "asus-wmi";
 	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
 	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
 
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 972385ca1990b..3cf79e55e3129 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -247,6 +247,7 @@ static int thermal_init(void)
 	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
 	if (!thermal_handler)
 		return -ENOMEM;
+	thermal_handler->name = "dell-pc";
 	thermal_handler->profile_get = thermal_platform_profile_get;
 	thermal_handler->profile_set = thermal_platform_profile_set;
 
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 81ccc96ffe40a..26cac73caf2b9 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1624,6 +1624,7 @@ static int thermal_profile_setup(void)
 		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
 	}
 
+	platform_profile_handler.name = "hp-wmi";
 	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
 	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
 
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 9d8c3f064050e..1f94c14c3b832 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1102,6 +1102,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 
 	mutex_init(&priv->dytc->mutex);
 
+	priv->dytc->pprof.name = "ideapad-laptop";
 	priv->dytc->priv = priv;
 	priv->dytc->pprof.profile_get = dytc_profile_get;
 	priv->dytc->pprof.profile_set = dytc_profile_set;
diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 8440defa67886..03da2c8cf6789 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -177,6 +177,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
+	priv->handler.name = "inspur-wmi";
 	priv->handler.profile_get = inspur_platform_profile_get;
 	priv->handler.profile_set = inspur_platform_profile_set;
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4c1b0553f8720..c8c316b8507a5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10549,6 +10549,7 @@ static void dytc_profile_refresh(void)
 }
 
 static struct platform_profile_handler dytc_profile = {
+	.name = "thinkpad-acpi",
 	.profile_get = dytc_profile_get,
 	.profile_set = dytc_profile_set,
 };
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index f5492ed413f36..6fa988e417428 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -27,6 +27,7 @@ enum platform_profile_option {
 };
 
 struct platform_profile_handler {
+	const char *name;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
 	int (*profile_get)(struct platform_profile_handler *pprof,
 				enum platform_profile_option *profile);
-- 
2.43.0

Re: [PATCH v2 01/15] ACPI: platform-profile: Add a name member to handlers

[Maximilian Luz]

On 10/28/24 3:01 AM, Mario Limonciello wrote:
> In order to prepare for allowing multiple handlers, introduce
> a name field that can be used to distinguish between different
> handlers.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/surface/surface_platform_profile.c | 1 +
>   drivers/platform/x86/acer-wmi.c                     | 1 +
>   drivers/platform/x86/amd/pmf/sps.c                  | 1 +
>   drivers/platform/x86/asus-wmi.c                     | 1 +
>   drivers/platform/x86/dell/dell-pc.c                 | 1 +
>   drivers/platform/x86/hp/hp-wmi.c                    | 1 +
>   drivers/platform/x86/ideapad-laptop.c               | 1 +
>   drivers/platform/x86/inspur_platform_profile.c      | 1 +
>   drivers/platform/x86/thinkpad_acpi.c                | 1 +
>   include/linux/platform_profile.h                    | 1 +
>   10 files changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index 3de864bc66108..61aa488a80eb5 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -211,6 +211,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
>   
>   	tpd->sdev = sdev;
>   
> +	tpd->handler.name = "Surface Platform Profile";
>   	tpd->handler.profile_get = ssam_platform_profile_get;
>   	tpd->handler.profile_set = ssam_platform_profile_set;
>   
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index d09baa3d3d902..53fbc9b4d3df7 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1878,6 +1878,7 @@ static int acer_platform_profile_setup(void)
>   	if (quirks->predator_v4) {
>   		int err;
>   
> +		platform_profile_handler.name = "acer-wmi";
>   		platform_profile_handler.profile_get =
>   			acer_predator_v4_platform_profile_get;
>   		platform_profile_handler.profile_set =
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index 92f7fb22277dc..e2d0cc92c4396 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -405,6 +405,7 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>   		amd_pmf_set_sps_power_limits(dev);
>   	}
>   
> +	dev->pprof.name = "amd-pmf";
>   	dev->pprof.profile_get = amd_pmf_profile_get;
>   	dev->pprof.profile_set = amd_pmf_profile_set;
>   
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 2ccc23b259d3e..c7c104c65a85a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3910,6 +3910,7 @@ static int platform_profile_setup(struct asus_wmi *asus)
>   
>   	dev_info(dev, "Using throttle_thermal_policy for platform_profile support\n");
>   
> +	asus->platform_profile_handler.name = "asus-wmi";
>   	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
>   	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
>   
> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
> index 972385ca1990b..3cf79e55e3129 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -247,6 +247,7 @@ static int thermal_init(void)
>   	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
>   	if (!thermal_handler)
>   		return -ENOMEM;
> +	thermal_handler->name = "dell-pc";
>   	thermal_handler->profile_get = thermal_platform_profile_get;
>   	thermal_handler->profile_set = thermal_platform_profile_set;
>   
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 81ccc96ffe40a..26cac73caf2b9 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -1624,6 +1624,7 @@ static int thermal_profile_setup(void)
>   		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
>   	}
>   
> +	platform_profile_handler.name = "hp-wmi";
>   	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
>   	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
>   
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 9d8c3f064050e..1f94c14c3b832 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1102,6 +1102,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
>   
>   	mutex_init(&priv->dytc->mutex);
>   
> +	priv->dytc->pprof.name = "ideapad-laptop";
>   	priv->dytc->priv = priv;
>   	priv->dytc->pprof.profile_get = dytc_profile_get;
>   	priv->dytc->pprof.profile_set = dytc_profile_set;
> diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
> index 8440defa67886..03da2c8cf6789 100644
> --- a/drivers/platform/x86/inspur_platform_profile.c
> +++ b/drivers/platform/x86/inspur_platform_profile.c
> @@ -177,6 +177,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
>   	priv->wdev = wdev;
>   	dev_set_drvdata(&wdev->dev, priv);
>   
> +	priv->handler.name = "inspur-wmi";
>   	priv->handler.profile_get = inspur_platform_profile_get;
>   	priv->handler.profile_set = inspur_platform_profile_set;
>   
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 4c1b0553f8720..c8c316b8507a5 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10549,6 +10549,7 @@ static void dytc_profile_refresh(void)
>   }
>   
>   static struct platform_profile_handler dytc_profile = {
> +	.name = "thinkpad-acpi",
>   	.profile_get = dytc_profile_get,
>   	.profile_set = dytc_profile_set,
>   };
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index f5492ed413f36..6fa988e417428 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -27,6 +27,7 @@ enum platform_profile_option {
>   };
>   
>   struct platform_profile_handler {
> +	const char *name;
>   	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>   	int (*profile_get)(struct platform_profile_handler *pprof,
>   				enum platform_profile_option *profile);

Looks good to me.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

[PATCH v2 02/15] platform/surface: aggregator: Add platform handler pointer to device

[Mario Limonciello]

To be able to reference the platform handler in remove, add
a pointer to `struct ssam_device`.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Use ssam_device_set_drvdata()
---
 drivers/platform/surface/surface_platform_profile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 61aa488a80eb5..a18eb93eebb92 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -210,6 +210,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 		return -ENOMEM;
 
 	tpd->sdev = sdev;
+	ssam_device_set_drvdata(sdev, tpd);
 
 	tpd->handler.name = "Surface Platform Profile";
 	tpd->handler.profile_get = ssam_platform_profile_get;
-- 
2.43.0

Re: [PATCH v2 02/15] platform/surface: aggregator: Add platform handler pointer to device

[Maximilian Luz]

On 10/28/24 3:01 AM, Mario Limonciello wrote:
> To be able to reference the platform handler in remove, add
> a pointer to `struct ssam_device`.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>   * Use ssam_device_set_drvdata()
> ---
>   drivers/platform/surface/surface_platform_profile.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index 61aa488a80eb5..a18eb93eebb92 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -210,6 +210,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
>   		return -ENOMEM;
>   
>   	tpd->sdev = sdev;
> +	ssam_device_set_drvdata(sdev, tpd);
>   
>   	tpd->handler.name = "Surface Platform Profile";
>   	tpd->handler.profile_get = ssam_platform_profile_get;

Looks good to me now, thanks!

Maybe this could/should be squashed into patch 03 now that it's just a
one-line change, but I'll leave that decision to Hans or Ilpo.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

Re: [PATCH v2 02/15] platform/surface: aggregator: Add platform handler pointer to device

[Mario Limonciello]

On 10/28/2024 12:30, Maximilian Luz wrote:
> On 10/28/24 3:01 AM, Mario Limonciello wrote:
>> To be able to reference the platform handler in remove, add
>> a pointer to `struct ssam_device`.
>>
>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v2:
>>   * Use ssam_device_set_drvdata()
>> ---
>>   drivers/platform/surface/surface_platform_profile.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/platform/surface/surface_platform_profile.c b/ 
>> drivers/platform/surface/surface_platform_profile.c
>> index 61aa488a80eb5..a18eb93eebb92 100644
>> --- a/drivers/platform/surface/surface_platform_profile.c
>> +++ b/drivers/platform/surface/surface_platform_profile.c
>> @@ -210,6 +210,7 @@ static int surface_platform_profile_probe(struct 
>> ssam_device *sdev)
>>           return -ENOMEM;
>>       tpd->sdev = sdev;
>> +    ssam_device_set_drvdata(sdev, tpd);
>>       tpd->handler.name = "Surface Platform Profile";
>>       tpd->handler.profile_get = ssam_platform_profile_get;
> 
> Looks good to me now, thanks!
> 
> Maybe this could/should be squashed into patch 03 now that it's just a
> one-line change, but I'll leave that decision to Hans or Ilpo.
> 
> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

Oh yeah; totally makes sense to squash now.  I'll merge the two patches 
for the next go around and pick up your tag.

[PATCH v2 03/15] ACPI: platform_profile: Add platform handler argument to platform_profile_remove()

[Mario Limonciello]

To allow registering and unregistering multiple platform handlers calls
to platform_profile_remove() will need to know which handler is to be
removed.  Add an argument for this.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Use ssam_device_get_drvdata() for surface
---
 drivers/acpi/platform_profile.c                     | 2 +-
 drivers/platform/surface/surface_platform_profile.c | 5 ++++-
 drivers/platform/x86/acer-wmi.c                     | 4 ++--
 drivers/platform/x86/amd/pmf/sps.c                  | 2 +-
 drivers/platform/x86/asus-wmi.c                     | 4 ++--
 drivers/platform/x86/dell/dell-pc.c                 | 2 +-
 drivers/platform/x86/hp/hp-wmi.c                    | 2 +-
 drivers/platform/x86/ideapad-laptop.c               | 2 +-
 drivers/platform/x86/inspur_platform_profile.c      | 5 ++++-
 drivers/platform/x86/thinkpad_acpi.c                | 2 +-
 include/linux/platform_profile.h                    | 2 +-
 11 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d2f7fd7743a13..c24744da20916 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -205,7 +205,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
 
-int platform_profile_remove(void)
+int platform_profile_remove(struct platform_profile_handler *pprof)
 {
 	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index a18eb93eebb92..1c9e37b1f4761 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -228,7 +228,10 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 
 static void surface_platform_profile_remove(struct ssam_device *sdev)
 {
-	platform_profile_remove();
+	struct ssam_platform_profile_device *tpd;
+
+	tpd = ssam_device_get_drvdata(sdev);
+	platform_profile_remove(&tpd->handler);
 }
 
 static const struct ssam_device_id ssam_platform_profile_match[] = {
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 53fbc9b4d3df7..71761d4220c26 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -2546,7 +2546,7 @@ static int acer_platform_probe(struct platform_device *device)
 
 error_hwmon:
 	if (platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&platform_profile_handler);
 error_platform_profile:
 	acer_rfkill_exit();
 error_rfkill:
@@ -2569,7 +2569,7 @@ static void acer_platform_remove(struct platform_device *device)
 	acer_rfkill_exit();
 
 	if (platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&platform_profile_handler);
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index e2d0cc92c4396..cfa88c0c9e594 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -425,5 +425,5 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 
 void amd_pmf_deinit_sps(struct amd_pmf_dev *dev)
 {
-	platform_profile_remove();
+	platform_profile_remove(&dev->pprof);
 }
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index c7c104c65a85a..f5f8cda7fd19c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4885,7 +4885,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_custom_fan_curve:
 fail_platform_profile_setup:
 	if (asus->platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&asus->platform_profile_handler);
 fail_fan_boost_mode:
 fail_platform:
 	kfree(asus);
@@ -4912,7 +4912,7 @@ static void asus_wmi_remove(struct platform_device *device)
 	asus_wmi_battery_exit(asus);
 
 	if (asus->platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&asus->platform_profile_handler);
 
 	kfree(asus);
 }
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 3cf79e55e3129..4196154cc37d9 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -273,7 +273,7 @@ static int thermal_init(void)
 static void thermal_cleanup(void)
 {
 	if (thermal_handler) {
-		platform_profile_remove();
+		platform_profile_remove(thermal_handler);
 		kfree(thermal_handler);
 	}
 }
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 26cac73caf2b9..bb8771d8b5cd8 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1692,7 +1692,7 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
 	}
 
 	if (platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&platform_profile_handler);
 }
 
 static int hp_wmi_resume_handler(struct device *device)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 1f94c14c3b832..50819ac919e87 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1135,7 +1135,7 @@ static void ideapad_dytc_profile_exit(struct ideapad_private *priv)
 	if (!priv->dytc)
 		return;
 
-	platform_profile_remove();
+	platform_profile_remove(&priv->dytc->pprof);
 	mutex_destroy(&priv->dytc->mutex);
 	kfree(priv->dytc);
 
diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 03da2c8cf6789..d8fd59b826f3a 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -190,7 +190,10 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 
 static void inspur_wmi_remove(struct wmi_device *wdev)
 {
-	platform_profile_remove();
+	struct inspur_wmi_priv *priv;
+
+	priv = dev_get_drvdata(&wdev->dev);
+	platform_profile_remove(&priv->handler);
 }
 
 static const struct wmi_device_id inspur_wmi_id_table[] = {
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c8c316b8507a5..619a4db74e5f3 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10637,7 +10637,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 
 static void dytc_profile_exit(void)
 {
-	platform_profile_remove();
+	platform_profile_remove(&dytc_profile);
 }
 
 static struct ibm_struct  dytc_profile_driver_data = {
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 6fa988e417428..58279b76d740e 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -36,7 +36,7 @@ struct platform_profile_handler {
 };
 
 int platform_profile_register(struct platform_profile_handler *pprof);
-int platform_profile_remove(void);
+int platform_profile_remove(struct platform_profile_handler *pprof);
 int platform_profile_cycle(void);
 void platform_profile_notify(void);
 
-- 
2.43.0

Re: [PATCH v2 03/15] ACPI: platform_profile: Add platform handler argument to platform_profile_remove()

[Maximilian Luz]

On 10/28/24 3:01 AM, Mario Limonciello wrote:
> To allow registering and unregistering multiple platform handlers calls
> to platform_profile_remove() will need to know which handler is to be
> removed.  Add an argument for this.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>   * Use ssam_device_get_drvdata() for surface
> ---
>   drivers/acpi/platform_profile.c                     | 2 +-
>   drivers/platform/surface/surface_platform_profile.c | 5 ++++-
>   drivers/platform/x86/acer-wmi.c                     | 4 ++--
>   drivers/platform/x86/amd/pmf/sps.c                  | 2 +-
>   drivers/platform/x86/asus-wmi.c                     | 4 ++--
>   drivers/platform/x86/dell/dell-pc.c                 | 2 +-
>   drivers/platform/x86/hp/hp-wmi.c                    | 2 +-
>   drivers/platform/x86/ideapad-laptop.c               | 2 +-
>   drivers/platform/x86/inspur_platform_profile.c      | 5 ++++-
>   drivers/platform/x86/thinkpad_acpi.c                | 2 +-
>   include/linux/platform_profile.h                    | 2 +-
>   11 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index d2f7fd7743a13..c24744da20916 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -205,7 +205,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_register);
>   
> -int platform_profile_remove(void)
> +int platform_profile_remove(struct platform_profile_handler *pprof)
>   {
>   	sysfs_remove_group(acpi_kobj, &platform_profile_group);
>   
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index a18eb93eebb92..1c9e37b1f4761 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -228,7 +228,10 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
>   
>   static void surface_platform_profile_remove(struct ssam_device *sdev)
>   {
> -	platform_profile_remove();
> +	struct ssam_platform_profile_device *tpd;
> +
> +	tpd = ssam_device_get_drvdata(sdev);
> +	platform_profile_remove(&tpd->handler);
>   }
>   
>   static const struct ssam_device_id ssam_platform_profile_match[] = {
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 53fbc9b4d3df7..71761d4220c26 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -2546,7 +2546,7 @@ static int acer_platform_probe(struct platform_device *device)
>   
>   error_hwmon:
>   	if (platform_profile_support)
> -		platform_profile_remove();
> +		platform_profile_remove(&platform_profile_handler);
>   error_platform_profile:
>   	acer_rfkill_exit();
>   error_rfkill:
> @@ -2569,7 +2569,7 @@ static void acer_platform_remove(struct platform_device *device)
>   	acer_rfkill_exit();
>   
>   	if (platform_profile_support)
> -		platform_profile_remove();
> +		platform_profile_remove(&platform_profile_handler);
>   }
>   
>   #ifdef CONFIG_PM_SLEEP
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index e2d0cc92c4396..cfa88c0c9e594 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -425,5 +425,5 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>   
>   void amd_pmf_deinit_sps(struct amd_pmf_dev *dev)
>   {
> -	platform_profile_remove();
> +	platform_profile_remove(&dev->pprof);
>   }
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index c7c104c65a85a..f5f8cda7fd19c 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -4885,7 +4885,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>   fail_custom_fan_curve:
>   fail_platform_profile_setup:
>   	if (asus->platform_profile_support)
> -		platform_profile_remove();
> +		platform_profile_remove(&asus->platform_profile_handler);
>   fail_fan_boost_mode:
>   fail_platform:
>   	kfree(asus);
> @@ -4912,7 +4912,7 @@ static void asus_wmi_remove(struct platform_device *device)
>   	asus_wmi_battery_exit(asus);
>   
>   	if (asus->platform_profile_support)
> -		platform_profile_remove();
> +		platform_profile_remove(&asus->platform_profile_handler);
>   
>   	kfree(asus);
>   }
> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
> index 3cf79e55e3129..4196154cc37d9 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -273,7 +273,7 @@ static int thermal_init(void)
>   static void thermal_cleanup(void)
>   {
>   	if (thermal_handler) {
> -		platform_profile_remove();
> +		platform_profile_remove(thermal_handler);
>   		kfree(thermal_handler);
>   	}
>   }
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 26cac73caf2b9..bb8771d8b5cd8 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -1692,7 +1692,7 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
>   	}
>   
>   	if (platform_profile_support)
> -		platform_profile_remove();
> +		platform_profile_remove(&platform_profile_handler);
>   }
>   
>   static int hp_wmi_resume_handler(struct device *device)
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 1f94c14c3b832..50819ac919e87 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1135,7 +1135,7 @@ static void ideapad_dytc_profile_exit(struct ideapad_private *priv)
>   	if (!priv->dytc)
>   		return;
>   
> -	platform_profile_remove();
> +	platform_profile_remove(&priv->dytc->pprof);
>   	mutex_destroy(&priv->dytc->mutex);
>   	kfree(priv->dytc);
>   
> diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
> index 03da2c8cf6789..d8fd59b826f3a 100644
> --- a/drivers/platform/x86/inspur_platform_profile.c
> +++ b/drivers/platform/x86/inspur_platform_profile.c
> @@ -190,7 +190,10 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
>   
>   static void inspur_wmi_remove(struct wmi_device *wdev)
>   {
> -	platform_profile_remove();
> +	struct inspur_wmi_priv *priv;
> +
> +	priv = dev_get_drvdata(&wdev->dev);
> +	platform_profile_remove(&priv->handler);
>   }
>   
>   static const struct wmi_device_id inspur_wmi_id_table[] = {
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c8c316b8507a5..619a4db74e5f3 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10637,7 +10637,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>   
>   static void dytc_profile_exit(void)
>   {
> -	platform_profile_remove();
> +	platform_profile_remove(&dytc_profile);
>   }
>   
>   static struct ibm_struct  dytc_profile_driver_data = {
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 6fa988e417428..58279b76d740e 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -36,7 +36,7 @@ struct platform_profile_handler {
>   };
>   
>   int platform_profile_register(struct platform_profile_handler *pprof);
> -int platform_profile_remove(void);
> +int platform_profile_remove(struct platform_profile_handler *pprof);
>   int platform_profile_cycle(void);
>   void platform_profile_notify(void);
>   

Looks good to me, thanks!

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

[PATCH v2 04/15] ACPI: platform_profile: Add a list to platform profile handler

[Mario Limonciello]

In order to prepare for having support for multiple platform handlers
a list will be needed to iterate over them for various platform
profile handler calls.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c  | 5 ++++-
 include/linux/platform_profile.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c24744da20916..0c60fc970b6e8 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -10,6 +10,7 @@
 #include <linux/sysfs.h>
 
 static struct platform_profile_handler *cur_profile;
+static LIST_HEAD(platform_profile_handler_list);
 static DEFINE_MUTEX(profile_lock);
 
 static const char * const profile_names[] = {
@@ -198,6 +199,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		mutex_unlock(&profile_lock);
 		return err;
 	}
+	list_add_tail(&pprof->list, &platform_profile_handler_list);
 
 	cur_profile = pprof;
 	mutex_unlock(&profile_lock);
@@ -207,8 +209,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register);
 
 int platform_profile_remove(struct platform_profile_handler *pprof)
 {
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	list_del(&pprof->list);
 
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 	mutex_lock(&profile_lock);
 	cur_profile = NULL;
 	mutex_unlock(&profile_lock);
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 58279b76d740e..9ded63a9ae6f1 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -29,6 +29,7 @@ enum platform_profile_option {
 struct platform_profile_handler {
 	const char *name;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	struct list_head list;
 	int (*profile_get)(struct platform_profile_handler *pprof,
 				enum platform_profile_option *profile);
 	int (*profile_set)(struct platform_profile_handler *pprof,
-- 
2.43.0

[PATCH v2 05/15] ACPI: platform_profile: Move sanity check out of the mutex

[Mario Limonciello]

The sanity check that the platform handler had choices set doesn't
need the mutex taken.  Move it to earlier in the registration.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 0c60fc970b6e8..c20256bb39579 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
 
+	/* Sanity check the profile handler field are set */
+	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
+		!pprof->profile_set || !pprof->profile_get) {
+		return -EINVAL;
+	}
+
 	mutex_lock(&profile_lock);
 	/* We can only have one active profile */
 	if (cur_profile) {
@@ -187,13 +193,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		return -EEXIST;
 	}
 
-	/* Sanity check the profile handler field are set */
-	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
-		!pprof->profile_set || !pprof->profile_get) {
-		mutex_unlock(&profile_lock);
-		return -EINVAL;
-	}
-
 	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
 	if (err) {
 		mutex_unlock(&profile_lock);
-- 
2.43.0

Re: [PATCH v2 05/15] ACPI: platform_profile: Move sanity check out of the mutex

[Ilpo Järvinen]

On Sun, 27 Oct 2024, Mario Limonciello wrote:

> The sanity check that the platform handler had choices set doesn't
> need the mutex taken.  Move it to earlier in the registration.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 0c60fc970b6e8..c20256bb39579 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>  {
>  	int err;
>  
> +	/* Sanity check the profile handler field are set */
> +	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
> +		!pprof->profile_set || !pprof->profile_get) {

While moving the line, please change the misleading indentation too 
(you'd have probably done it based on my comment on v1 too but since 
this series has changed things a bit, I'm noting it also against this 
patch).

-- 
 i.

> +		return -EINVAL;
> +	}
> +
>  	mutex_lock(&profile_lock);
>  	/* We can only have one active profile */
>  	if (cur_profile) {
> @@ -187,13 +193,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>  		return -EEXIST;
>  	}
>  
> -	/* Sanity check the profile handler field are set */
> -	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
> -		!pprof->profile_set || !pprof->profile_get) {
> -		mutex_unlock(&profile_lock);
> -		return -EINVAL;
> -	}
> -
>  	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>  	if (err) {
>  		mutex_unlock(&profile_lock);
> 

Re: [PATCH v2 05/15] ACPI: platform_profile: Move sanity check out of the mutex

[Mario Limonciello]

On 10/29/2024 05:12, Ilpo Järvinen wrote:
> On Sun, 27 Oct 2024, Mario Limonciello wrote:
> 
>> The sanity check that the platform handler had choices set doesn't
>> need the mutex taken.  Move it to earlier in the registration.
>>
>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index 0c60fc970b6e8..c20256bb39579 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>>   {
>>   	int err;
>>   
>> +	/* Sanity check the profile handler field are set */
>> +	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
>> +		!pprof->profile_set || !pprof->profile_get) {
> 
> While moving the line, please change the misleading indentation too
> (you'd have probably done it based on my comment on v1 too but since
> this series has changed things a bit, I'm noting it also against this
> patch).
> 

Ack, thanks!

[PATCH v2 06/15] ACPI: platform_profile: Use guard(mutex) for register/unregister

[Mario Limonciello]

guard(mutex) can be used to automatically release mutexes when going
out of scope.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c20256bb39579..100d6ffb10cdb 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -186,34 +186,29 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		return -EINVAL;
 	}
 
-	mutex_lock(&profile_lock);
+	guard(mutex)(&profile_lock);
 	/* We can only have one active profile */
-	if (cur_profile) {
-		mutex_unlock(&profile_lock);
+	if (cur_profile)
 		return -EEXIST;
-	}
 
 	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
-	if (err) {
-		mutex_unlock(&profile_lock);
+	if (err)
 		return err;
-	}
 	list_add_tail(&pprof->list, &platform_profile_handler_list);
 
 	cur_profile = pprof;
-	mutex_unlock(&profile_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
 
 int platform_profile_remove(struct platform_profile_handler *pprof)
 {
+	guard(mutex)(&profile_lock);
+
 	list_del(&pprof->list);
 
 	sysfs_remove_group(acpi_kobj, &platform_profile_group);
-	mutex_lock(&profile_lock);
 	cur_profile = NULL;
-	mutex_unlock(&profile_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
-- 
2.43.0

[PATCH v2 07/15] ACPI: platform_profile: Only remove group when no more handler registered

[Mario Limonciello]

As multiple handlers may register for ACPI platform profile handler,
only remove the sysfs group when the last one unregisters.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 100d6ffb10cdb..ede1550170f1a 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -23,6 +23,16 @@ static const char * const profile_names[] = {
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
+static bool platform_profile_is_registered(void)
+{
+	struct list_head *pos;
+	int count = 0;
+
+	list_for_each(pos, &platform_profile_handler_list)
+		count++;
+	return count > 0;
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -207,8 +217,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 
 	list_del(&pprof->list);
 
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 	cur_profile = NULL;
+	if (!platform_profile_is_registered())
+		sysfs_remove_group(acpi_kobj, &platform_profile_group);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
-- 
2.43.0

[PATCH v2 08/15] ACPI: platform_profile: Require handlers to support balanced profile

[Mario Limonciello]

As support for multiple simultaneous platform handers is introduced it's
important they have at least the balanced profile in common.

This will be used as a fallback in case setting the profile across one of the
handlers happens to fail.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index ede1550170f1a..c0d0775831214 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -190,9 +190,14 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
 
-	/* Sanity check the profile handler field are set */
+	/* Sanity check the profile handler field are set and balanced is supported */
 	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
 		!pprof->profile_set || !pprof->profile_get) {
+		pr_err("platform_profile: handler is invalid\n");
+		return -EINVAL;
+	}
+	if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
+		pr_err("platform_profile: handler does not support balanced profile\n");
 		return -EINVAL;
 	}
 
-- 
2.43.0

[PATCH v2 09/15] ACPI: platform_profile: Notify change events on register and unregister

[Mario Limonciello]

As multiple platform profile handlers may come and go, send a notification
to userspace each time that a platform profile handler is registered or
unregistered.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c0d0775831214..5fee467614eac 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -210,6 +210,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	if (err)
 		return err;
 	list_add_tail(&pprof->list, &platform_profile_handler_list);
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
 	cur_profile = pprof;
 	return 0;
@@ -223,6 +224,8 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 	list_del(&pprof->list);
 
 	cur_profile = NULL;
+
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 	if (!platform_profile_is_registered())
 		sysfs_remove_group(acpi_kobj, &platform_profile_group);
 
-- 
2.43.0

[PATCH v2 10/15] ACPI: platform_profile: Only show profiles common for all handlers

[Mario Limonciello]

If multiple platform profile handlers have been registered, don't allow
switching to profiles unique to only one handler.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 35 ++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 5fee467614eac..a83842f05022b 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -33,30 +33,43 @@ static bool platform_profile_is_registered(void)
 	return count > 0;
 }
 
+/* expected to be called under mutex */
+static unsigned long platform_profile_get_choices(void)
+{
+	struct platform_profile_handler *handler;
+	unsigned long seen = 0;
+	int i;
+
+	list_for_each_entry(handler, &platform_profile_handler_list, list) {
+		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST) {
+			if (seen & BIT(i))
+				continue;
+			seen |= BIT(i);
+		}
+	}
+
+	return seen;
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
 {
+	unsigned long choices;
 	int len = 0;
-	int err, i;
-
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
+	int i;
 
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
+		choices = platform_profile_get_choices();
 
-	for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) {
+	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
 		if (len == 0)
 			len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
 		else
 			len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
 	}
+
 	len += sysfs_emit_at(buf, len, "\n");
-	mutex_unlock(&profile_lock);
 	return len;
 }
 
-- 
2.43.0

Re: [PATCH v2 10/15] ACPI: platform_profile: Only show profiles common for all handlers

[Ilpo Järvinen]

On Sun, 27 Oct 2024, Mario Limonciello wrote:

> If multiple platform profile handlers have been registered, don't allow
> switching to profiles unique to only one handler.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 35 ++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 5fee467614eac..a83842f05022b 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -33,30 +33,43 @@ static bool platform_profile_is_registered(void)
>  	return count > 0;
>  }
>  
> +/* expected to be called under mutex */
> +static unsigned long platform_profile_get_choices(void)
> +{
> +	struct platform_profile_handler *handler;
> +	unsigned long seen = 0;
> +	int i;
> +
> +	list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST) {
> +			if (seen & BIT(i))
> +				continue;
> +			seen |= BIT(i);

The condition+continue part seems unnecessary if all this function does is 
accumulate bits into seen.

-- 
 i.

> +		}
> +	}
> +
> +	return seen;
> +}
> +
>  static ssize_t platform_profile_choices_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
>  {
> +	unsigned long choices;
>  	int len = 0;
> -	int err, i;
> -
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> +	int i;
>  
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
> +		choices = platform_profile_get_choices();
> -	for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) {
> +	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>  		if (len == 0)
>  			len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>  		else
>  			len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>  	}
> +
>  	len += sysfs_emit_at(buf, len, "\n");
> -	mutex_unlock(&profile_lock);
>  	return len;
>  }
>  
> 

[PATCH v2 11/15] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

If multiple platform profile handlers have been registered then when
setting a profile verify that all profile handlers support the requested
profile and set it to each handler.

If this fails for any given handler, revert all profile handlers back to
balanced and log an error into the kernel ring buffer.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 47 ++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index a83842f05022b..db2ebd0393cf7 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
+	struct platform_profile_handler *handler;
+	unsigned long choices;
 	int err, i;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
-
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
-
 	/* Scan for a matching profile */
 	i = sysfs_match_string(profile_names, buf);
 	if (i < 0) {
-		mutex_unlock(&profile_lock);
 		return -EINVAL;
 	}
 
-	/* Check that platform supports this profile choice */
-	if (!test_bit(i, cur_profile->choices)) {
-		mutex_unlock(&profile_lock);
-		return -EOPNOTSUPP;
-	}
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!platform_profile_is_registered())
+			return -ENODEV;
 
-	err = cur_profile->profile_set(cur_profile, i);
-	if (!err)
-		sysfs_notify(acpi_kobj, NULL, "platform_profile");
+		/* Check that all handlers support this profile choice */
+		choices = platform_profile_get_choices();
+		if (!test_bit(i, &choices))
+			return -EOPNOTSUPP;
+
+		list_for_each_entry(handler, &platform_profile_handler_list, list) {
+			err = handler->profile_set(handler, i);
+			if (err) {
+				pr_err("Failed to set profile for handler %s\n", handler->name);
+				break;
+			}
+		}
+		if (err) {
+			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
+				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
+					pr_err("Failed to revert profile for handler %s\n", handler->name);
+			}
+			return err;
+		}
+	}
 
-	mutex_unlock(&profile_lock);
-	if (err)
-		return err;
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 	return count;
 }
 
-- 
2.43.0

Re: [PATCH v2 11/15] ACPI: platform_profile: Set profile for all registered handlers

[Ilpo Järvinen]

On Sun, 27 Oct 2024, Mario Limonciello wrote:

> If multiple platform profile handlers have been registered then when
> setting a profile verify that all profile handlers support the requested
> profile and set it to each handler.
> 
> If this fails for any given handler, revert all profile handlers back to
> balanced and log an error into the kernel ring buffer.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 47 ++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index a83842f05022b..db2ebd0393cf7 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf, size_t count)
>  {
> +	struct platform_profile_handler *handler;
> +	unsigned long choices;
>  	int err, i;
>  
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> -
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> -
>  	/* Scan for a matching profile */
>  	i = sysfs_match_string(profile_names, buf);
>  	if (i < 0) {
> -		mutex_unlock(&profile_lock);
>  		return -EINVAL;
>  	}
>  
> -	/* Check that platform supports this profile choice */
> -	if (!test_bit(i, cur_profile->choices)) {
> -		mutex_unlock(&profile_lock);
> -		return -EOPNOTSUPP;
> -	}
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {

You made guard() conversions in the earlier patch but for some reason 
left scoped_cond_guard() ones mixed into other changes still. Is there
a very good reason for that?

-- 
 i.

> +		if (!platform_profile_is_registered())
> +			return -ENODEV;
>  
> -	err = cur_profile->profile_set(cur_profile, i);
> -	if (!err)
> -		sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +		/* Check that all handlers support this profile choice */
> +		choices = platform_profile_get_choices();
> +		if (!test_bit(i, &choices))
> +			return -EOPNOTSUPP;
> +
> +		list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +			err = handler->profile_set(handler, i);
> +			if (err) {
> +				pr_err("Failed to set profile for handler %s\n", handler->name);
> +				break;
> +			}
> +		}
> +		if (err) {
> +			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
> +				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
> +					pr_err("Failed to revert profile for handler %s\n", handler->name);
> +			}
> +			return err;
> +		}
> +	}
>  
> -	mutex_unlock(&profile_lock);
> -	if (err)
> -		return err;
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>  	return count;
>  }
>  
> 

Re: [PATCH v2 11/15] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

On 10/29/2024 05:22, Ilpo Järvinen wrote:
> On Sun, 27 Oct 2024, Mario Limonciello wrote:
> 
>> If multiple platform profile handlers have been registered then when
>> setting a profile verify that all profile handlers support the requested
>> profile and set it to each handler.
>>
>> If this fails for any given handler, revert all profile handlers back to
>> balanced and log an error into the kernel ring buffer.
>>
>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c | 47 ++++++++++++++++++---------------
>>   1 file changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index a83842f05022b..db2ebd0393cf7 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device *dev,
>>   			    struct device_attribute *attr,
>>   			    const char *buf, size_t count)
>>   {
>> +	struct platform_profile_handler *handler;
>> +	unsigned long choices;
>>   	int err, i;
>>   
>> -	err = mutex_lock_interruptible(&profile_lock);
>> -	if (err)
>> -		return err;
>> -
>> -	if (!cur_profile) {
>> -		mutex_unlock(&profile_lock);
>> -		return -ENODEV;
>> -	}
>> -
>>   	/* Scan for a matching profile */
>>   	i = sysfs_match_string(profile_names, buf);
>>   	if (i < 0) {
>> -		mutex_unlock(&profile_lock);
>>   		return -EINVAL;
>>   	}
>>   
>> -	/* Check that platform supports this profile choice */
>> -	if (!test_bit(i, cur_profile->choices)) {
>> -		mutex_unlock(&profile_lock);
>> -		return -EOPNOTSUPP;
>> -	}
>> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> 
> You made guard() conversions in the earlier patch but for some reason
> left scoped_cond_guard() ones mixed into other changes still. Is there
> a very good reason for that?
> 

Using scoped_cond_guard() requires changing the indentation which meant 
a bit of back and forth with code coming and going.  If you think it 
makes more sense to split up even considering the indentation changes 
I'll do another set of patches for the scoped_cond_guard changes only.

Re: [PATCH v2 11/15] ACPI: platform_profile: Set profile for all registered handlers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On Tue, 29 Oct 2024, Mario Limonciello wrote:

> On 10/29/2024 05:22, Ilpo Järvinen wrote:
> > On Sun, 27 Oct 2024, Mario Limonciello wrote:
> > 
> > > If multiple platform profile handlers have been registered then when
> > > setting a profile verify that all profile handlers support the requested
> > > profile and set it to each handler.
> > > 
> > > If this fails for any given handler, revert all profile handlers back to
> > > balanced and log an error into the kernel ring buffer.
> > > 
> > > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/acpi/platform_profile.c | 47 ++++++++++++++++++---------------
> > >   1 file changed, 26 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/platform_profile.c
> > > b/drivers/acpi/platform_profile.c
> > > index a83842f05022b..db2ebd0393cf7 100644
> > > --- a/drivers/acpi/platform_profile.c
> > > +++ b/drivers/acpi/platform_profile.c
> > > @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device
> > > *dev,
> > >   			    struct device_attribute *attr,
> > >   			    const char *buf, size_t count)
> > >   {
> > > +	struct platform_profile_handler *handler;
> > > +	unsigned long choices;
> > >   	int err, i;
> > >   -	err = mutex_lock_interruptible(&profile_lock);
> > > -	if (err)
> > > -		return err;
> > > -
> > > -	if (!cur_profile) {
> > > -		mutex_unlock(&profile_lock);
> > > -		return -ENODEV;
> > > -	}
> > > -
> > >   	/* Scan for a matching profile */
> > >   	i = sysfs_match_string(profile_names, buf);
> > >   	if (i < 0) {
> > > -		mutex_unlock(&profile_lock);
> > >   		return -EINVAL;
> > >   	}
> > >   -	/* Check that platform supports this profile choice */
> > > -	if (!test_bit(i, cur_profile->choices)) {
> > > -		mutex_unlock(&profile_lock);
> > > -		return -EOPNOTSUPP;
> > > -	}
> > > +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> > 
> > You made guard() conversions in the earlier patch but for some reason
> > left scoped_cond_guard() ones mixed into other changes still. Is there
> > a very good reason for that?
> > 
> 
> Using scoped_cond_guard() requires changing the indentation which meant a bit
> of back and forth with code coming and going.  If you think it makes more
> sense to split up even considering the indentation changes I'll do another set
> of patches for the scoped_cond_guard changes only.

There are ways to combat indentation changes while reviewing. However, 
it's a strange argument to bring up because now there are indentation 
changes in these patches exactly because you chose to make the 
scoped_cond_guard() change "while at it" rather than in a separate patch.

I believe the patches will become cleaner and easier to review if you do 
scoped_cond_guard() change separate from any other logic changes.

-- 
 i.

Re: [PATCH v2 11/15] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

On 10/29/2024 11:50, Ilpo Järvinen wrote:
> On Tue, 29 Oct 2024, Mario Limonciello wrote:
> 
>> On 10/29/2024 05:22, Ilpo Järvinen wrote:
>>> On Sun, 27 Oct 2024, Mario Limonciello wrote:
>>>
>>>> If multiple platform profile handlers have been registered then when
>>>> setting a profile verify that all profile handlers support the requested
>>>> profile and set it to each handler.
>>>>
>>>> If this fails for any given handler, revert all profile handlers back to
>>>> balanced and log an error into the kernel ring buffer.
>>>>
>>>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/acpi/platform_profile.c | 47 ++++++++++++++++++---------------
>>>>    1 file changed, 26 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c
>>>> b/drivers/acpi/platform_profile.c
>>>> index a83842f05022b..db2ebd0393cf7 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device
>>>> *dev,
>>>>    			    struct device_attribute *attr,
>>>>    			    const char *buf, size_t count)
>>>>    {
>>>> +	struct platform_profile_handler *handler;
>>>> +	unsigned long choices;
>>>>    	int err, i;
>>>>    -	err = mutex_lock_interruptible(&profile_lock);
>>>> -	if (err)
>>>> -		return err;
>>>> -
>>>> -	if (!cur_profile) {
>>>> -		mutex_unlock(&profile_lock);
>>>> -		return -ENODEV;
>>>> -	}
>>>> -
>>>>    	/* Scan for a matching profile */
>>>>    	i = sysfs_match_string(profile_names, buf);
>>>>    	if (i < 0) {
>>>> -		mutex_unlock(&profile_lock);
>>>>    		return -EINVAL;
>>>>    	}
>>>>    -	/* Check that platform supports this profile choice */
>>>> -	if (!test_bit(i, cur_profile->choices)) {
>>>> -		mutex_unlock(&profile_lock);
>>>> -		return -EOPNOTSUPP;
>>>> -	}
>>>> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>>
>>> You made guard() conversions in the earlier patch but for some reason
>>> left scoped_cond_guard() ones mixed into other changes still. Is there
>>> a very good reason for that?
>>>
>>
>> Using scoped_cond_guard() requires changing the indentation which meant a bit
>> of back and forth with code coming and going.  If you think it makes more
>> sense to split up even considering the indentation changes I'll do another set
>> of patches for the scoped_cond_guard changes only.
> 
> There are ways to combat indentation changes while reviewing. However,
> it's a strange argument to bring up because now there are indentation
> changes in these patches exactly because you chose to make the
> scoped_cond_guard() change "while at it" rather than in a separate patch.
> 
> I believe the patches will become cleaner and easier to review if you do
> scoped_cond_guard() change separate from any other logic changes.

OK will do.

[PATCH v2 12/15] ACPI: platform_profile: Make sure all profile handlers agree on profile

[Mario Limonciello]

If for any reason multiple profile handlers don't agree on the profile
set for the system then the value shown in sysfs can be wrong.

Explicitly check that they match.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 61 ++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index db2ebd0393cf7..d22c4eb5f0c36 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -51,6 +51,45 @@ static unsigned long platform_profile_get_choices(void)
 	return seen;
 }
 
+/* expected to be called under mutex */
+static int platform_profile_get_active(enum platform_profile_option *profile)
+{
+	struct platform_profile_handler *handler;
+	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
+	enum platform_profile_option active2 = PLATFORM_PROFILE_LAST;
+	int err;
+
+	list_for_each_entry(handler, &platform_profile_handler_list, list) {
+		if (active == PLATFORM_PROFILE_LAST)
+			err = handler->profile_get(handler, &active);
+		else
+			err = handler->profile_get(handler, &active2);
+		if (err) {
+			pr_err("Failed to get profile for handler %s\n", handler->name);
+			return err;
+		}
+
+		if (WARN_ON(active == PLATFORM_PROFILE_LAST))
+			return -EINVAL;
+		if (active2 == PLATFORM_PROFILE_LAST)
+			continue;
+
+		if (active != active2) {
+			pr_warn("Profile handlers don't agree on current profile\n");
+			return -EINVAL;
+		}
+		active = active2;
+	}
+
+	/* Check that profile is valid index */
+	if (WARN_ON((active < 0) || (active >= ARRAY_SIZE(profile_names))))
+		return -EIO;
+
+	*profile = active;
+
+	return 0;
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -80,24 +119,14 @@ static ssize_t platform_profile_show(struct device *dev,
 	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
 	int err;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
-
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!platform_profile_is_registered())
+			return -ENODEV;
+		err = platform_profile_get_active(&profile);
+		if (err)
+			return err;
 	}
 
-	err = cur_profile->profile_get(cur_profile, &profile);
-	mutex_unlock(&profile_lock);
-	if (err)
-		return err;
-
-	/* Check that profile is valid index */
-	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
-		return -EIO;
-
 	return sysfs_emit(buf, "%s\n", profile_names[profile]);
 }
 
-- 
2.43.0

Re: [PATCH v2 12/15] ACPI: platform_profile: Make sure all profile handlers agree on profile

[Ilpo Järvinen]

On Sun, 27 Oct 2024, Mario Limonciello wrote:

> If for any reason multiple profile handlers don't agree on the profile
> set for the system then the value shown in sysfs can be wrong.
> 
> Explicitly check that they match.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 61 ++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index db2ebd0393cf7..d22c4eb5f0c36 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -51,6 +51,45 @@ static unsigned long platform_profile_get_choices(void)
>  	return seen;
>  }
>  
> +/* expected to be called under mutex */

Don't add comments like this but enforce it with a lockdep annotation.

"mutex" would have been too vague anyway :-).

> +static int platform_profile_get_active(enum platform_profile_option *profile)
> +{
> +	struct platform_profile_handler *handler;
> +	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
> +	enum platform_profile_option active2 = PLATFORM_PROFILE_LAST;
> +	int err;
> +
> +	list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +		if (active == PLATFORM_PROFILE_LAST)
> +			err = handler->profile_get(handler, &active);
> +		else
> +			err = handler->profile_get(handler, &active2);
> +		if (err) {
> +			pr_err("Failed to get profile for handler %s\n", handler->name);
> +			return err;
> +		}
> +
> +		if (WARN_ON(active == PLATFORM_PROFILE_LAST))
> +			return -EINVAL;
> +		if (active2 == PLATFORM_PROFILE_LAST)
> +			continue;
> +
> +		if (active != active2) {
> +			pr_warn("Profile handlers don't agree on current profile\n");
> +			return -EINVAL;
> +		}
> +		active = active2;

This looked very confusing (IMO). How about this:

	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
	enum platform_profile_option val;
	...

		err = handler->profile_get(handler, &val);
		if (err) {
			pr_err(...);
			return err;
		}

		if (WARN_ON(val == PLATFORM_PROFILE_LAST))
			return -EINVAL;

		if (active != val && active != PLATFORM_PROFILE_LAST) {
			pr_warn("Profile handlers don't agree on current profile\n");
			return -EINVAL;
		}
		active = val;

> +	}
> +
> +	/* Check that profile is valid index */
> +	if (WARN_ON((active < 0) || (active >= ARRAY_SIZE(profile_names))))

What does that < 0 check do? Should it be checked right after reading 
profile_get()? Or perhaps check both of these right there?

> +		return -EIO;
> +
> +	*profile = active;
> +
> +	return 0;
> +}
> +
>  static ssize_t platform_profile_choices_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
> @@ -80,24 +119,14 @@ static ssize_t platform_profile_show(struct device *dev,
>  	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
>  	int err;
>  
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> -
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {

scoped_cond_guard() conversion should be made in the guard patch?

> +		if (!platform_profile_is_registered())
> +			return -ENODEV;
> +		err = platform_profile_get_active(&profile);
> +		if (err)
> +			return err;
>  	}
>  
> -	err = cur_profile->profile_get(cur_profile, &profile);
> -	mutex_unlock(&profile_lock);
> -	if (err)
> -		return err;
> -
> -	/* Check that profile is valid index */
> -	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> -		return -EIO;
> -
>  	return sysfs_emit(buf, "%s\n", profile_names[profile]);
>  }
>  
> 

-- 
 i.

[PATCH v2 13/15] ACPI: platform_profile: Check all profile handler to calculate next

[Mario Limonciello]

As multiple platform profile handlers might not all support the same
profile, cycling to the next profile could have a different result
depending on what handler are registered.

Check what is active and supported by all handlers to decide what
to do.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 52 +++++++++++++++++----------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d22c4eb5f0c36..e6ea5b600e2af 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -196,40 +196,42 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
 
 int platform_profile_cycle(void)
 {
+	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
+	struct platform_profile_handler *handler;
 	enum platform_profile_option profile;
-	enum platform_profile_option next;
+	unsigned long choices;
 	int err;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
-
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		err = platform_profile_get_active(&profile);
+		if (err)
+			return err;
 
-	err = cur_profile->profile_get(cur_profile, &profile);
-	if (err) {
-		mutex_unlock(&profile_lock);
-		return err;
-	}
+		choices = platform_profile_get_choices();
 
-	next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
-				  profile + 1);
+		next = find_next_bit_wrap(&choices,
+					  PLATFORM_PROFILE_LAST,
+					  profile + 1);
 
-	if (WARN_ON(next == PLATFORM_PROFILE_LAST)) {
-		mutex_unlock(&profile_lock);
-		return -EINVAL;
+		list_for_each_entry(handler, &platform_profile_handler_list, list) {
+			err = handler->profile_set(handler, next);
+			if (err) {
+				pr_err("Failed to set profile for handler %s\n", handler->name);
+				break;
+			}
+		}
+		if (err) {
+			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
+				err = handler->profile_set(handler, PLATFORM_PROFILE_BALANCED);
+				if (err)
+					pr_err("Failed to revert profile for handler %s\n", handler->name);
+			}
+		}
 	}
 
-	err = cur_profile->profile_set(cur_profile, next);
-	mutex_unlock(&profile_lock);
-
-	if (!err)
-		sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_cycle);
 
-- 
2.43.0

[PATCH v2 14/15] ACPI: platform_profile: Allow multiple handlers

[Mario Limonciello]

Multiple drivers may attempt to register platform profile handlers,
but only one may be registered and the behavior is non-deterministic
for which one wins.  It's mostly controlled by probing order.

This can be problematic if one driver changes CPU settings and another
driver notifies the EC for changing fan curves.

Modify the ACPI platform profile handler to let multiple drivers
register platform profile handlers and abstract this detail from userspace.

To avoid undefined behaviors only offer profiles that are commonly
advertised across multiple handlers.

If any problems occur when changing profiles for any driver, then revert
back to the balanced profile, which is now required.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index e6ea5b600e2af..2794a8f686200 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -9,7 +9,6 @@
 #include <linux/platform_profile.h>
 #include <linux/sysfs.h>
 
-static struct platform_profile_handler *cur_profile;
 static LIST_HEAD(platform_profile_handler_list);
 static DEFINE_MUTEX(profile_lock);
 
@@ -188,7 +187,8 @@ static const struct attribute_group platform_profile_group = {
 
 void platform_profile_notify(void)
 {
-	if (!cur_profile)
+	guard(mutex)(&profile_lock);
+	if (!platform_profile_is_registered())
 		return;
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 }
@@ -251,17 +251,15 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	}
 
 	guard(mutex)(&profile_lock);
-	/* We can only have one active profile */
-	if (cur_profile)
-		return -EEXIST;
 
-	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
-	if (err)
-		return err;
+	if (!platform_profile_is_registered()) {
+		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
+		if (err)
+			return err;
+	}
 	list_add_tail(&pprof->list, &platform_profile_handler_list);
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	cur_profile = pprof;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
@@ -272,9 +270,8 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 
 	list_del(&pprof->list);
 
-	cur_profile = NULL;
-
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
 	if (!platform_profile_is_registered())
 		sysfs_remove_group(acpi_kobj, &platform_profile_group);
 
-- 
2.43.0

[PATCH v2 15/15] platform/x86/amd: pmf: Drop all quirks

[Mario Limonciello]

As multiple platform profile handlers can now be registered, the quirks
to avoid registering amd-pmf as a handler are no longer necessary.
Drop them.

Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmf/Makefile     |  2 +-
 drivers/platform/x86/amd/pmf/core.c       |  1 -
 drivers/platform/x86/amd/pmf/pmf-quirks.c | 66 -----------------------
 drivers/platform/x86/amd/pmf/pmf.h        |  3 --
 4 files changed, 1 insertion(+), 71 deletions(-)
 delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c

diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
index 7d6079b02589c..6b26e48ce8ad2 100644
--- a/drivers/platform/x86/amd/pmf/Makefile
+++ b/drivers/platform/x86/amd/pmf/Makefile
@@ -7,4 +7,4 @@
 obj-$(CONFIG_AMD_PMF) += amd-pmf.o
 amd-pmf-objs := core.o acpi.o sps.o \
 		auto-mode.o cnqf.o \
-		tee-if.o spc.o pmf-quirks.o
+		tee-if.o spc.o
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 47126abd13ca0..6ad00b3d472fe 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -455,7 +455,6 @@ static int amd_pmf_probe(struct platform_device *pdev)
 	mutex_init(&dev->lock);
 	mutex_init(&dev->update_mutex);
 
-	amd_pmf_quirks_init(dev);
 	apmf_acpi_init(dev);
 	platform_set_drvdata(pdev, dev);
 	amd_pmf_dbgfs_register(dev);
diff --git a/drivers/platform/x86/amd/pmf/pmf-quirks.c b/drivers/platform/x86/amd/pmf/pmf-quirks.c
deleted file mode 100644
index 7cde5733b9cac..0000000000000
--- a/drivers/platform/x86/amd/pmf/pmf-quirks.c
+++ /dev/null
@@ -1,66 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * AMD Platform Management Framework Driver Quirks
- *
- * Copyright (c) 2024, Advanced Micro Devices, Inc.
- * All Rights Reserved.
- *
- * Author: Mario Limonciello <mario.limonciello@amd.com>
- */
-
-#include <linux/dmi.h>
-
-#include "pmf.h"
-
-struct quirk_entry {
-	u32 supported_func;
-};
-
-static struct quirk_entry quirk_no_sps_bug = {
-	.supported_func = 0x4003,
-};
-
-static const struct dmi_system_id fwbug_list[] = {
-	{
-		.ident = "ROG Zephyrus G14",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "GA403U"),
-		},
-		.driver_data = &quirk_no_sps_bug,
-	},
-	{
-		.ident = "ROG Ally X",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "RC72LA"),
-		},
-		.driver_data = &quirk_no_sps_bug,
-	},
-	{
-		.ident = "ASUS TUF Gaming A14",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "FA401W"),
-		},
-		.driver_data = &quirk_no_sps_bug,
-	},
-	{}
-};
-
-void amd_pmf_quirks_init(struct amd_pmf_dev *dev)
-{
-	const struct dmi_system_id *dmi_id;
-	struct quirk_entry *quirks;
-
-	dmi_id = dmi_first_match(fwbug_list);
-	if (!dmi_id)
-		return;
-
-	quirks = dmi_id->driver_data;
-	if (quirks->supported_func) {
-		dev->supported_func = quirks->supported_func;
-		pr_info("Using supported funcs quirk to avoid %s platform firmware bug\n",
-			dmi_id->ident);
-	}
-}
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 8ce8816da9c16..b89aa38434faa 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -795,7 +795,4 @@ int amd_pmf_smartpc_apply_bios_output(struct amd_pmf_dev *dev, u32 val, u32 preq
 void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
 void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
 
-/* Quirk infrastructure */
-void amd_pmf_quirks_init(struct amd_pmf_dev *dev);
-
 #endif /* PMF_H */
-- 
2.43.0

Re: [PATCH v2 15/15] platform/x86/amd: pmf: Drop all quirks

[Shyam Sundar S K]

On 10/28/2024 07:31, Mario Limonciello wrote:
> As multiple platform profile handlers can now be registered, the quirks
> to avoid registering amd-pmf as a handler are no longer necessary.
> Drop them.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thanks,
Shyam

> ---
>  drivers/platform/x86/amd/pmf/Makefile     |  2 +-
>  drivers/platform/x86/amd/pmf/core.c       |  1 -
>  drivers/platform/x86/amd/pmf/pmf-quirks.c | 66 -----------------------
>  drivers/platform/x86/amd/pmf/pmf.h        |  3 --
>  4 files changed, 1 insertion(+), 71 deletions(-)
>  delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
> 
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index 7d6079b02589c..6b26e48ce8ad2 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -7,4 +7,4 @@
>  obj-$(CONFIG_AMD_PMF) += amd-pmf.o
>  amd-pmf-objs := core.o acpi.o sps.o \
>  		auto-mode.o cnqf.o \
> -		tee-if.o spc.o pmf-quirks.o
> +		tee-if.o spc.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 47126abd13ca0..6ad00b3d472fe 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -455,7 +455,6 @@ static int amd_pmf_probe(struct platform_device *pdev)
>  	mutex_init(&dev->lock);
>  	mutex_init(&dev->update_mutex);
>  
> -	amd_pmf_quirks_init(dev);
>  	apmf_acpi_init(dev);
>  	platform_set_drvdata(pdev, dev);
>  	amd_pmf_dbgfs_register(dev);
> diff --git a/drivers/platform/x86/amd/pmf/pmf-quirks.c b/drivers/platform/x86/amd/pmf/pmf-quirks.c
> deleted file mode 100644
> index 7cde5733b9cac..0000000000000
> --- a/drivers/platform/x86/amd/pmf/pmf-quirks.c
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -/*
> - * AMD Platform Management Framework Driver Quirks
> - *
> - * Copyright (c) 2024, Advanced Micro Devices, Inc.
> - * All Rights Reserved.
> - *
> - * Author: Mario Limonciello <mario.limonciello@amd.com>
> - */
> -
> -#include <linux/dmi.h>
> -
> -#include "pmf.h"
> -
> -struct quirk_entry {
> -	u32 supported_func;
> -};
> -
> -static struct quirk_entry quirk_no_sps_bug = {
> -	.supported_func = 0x4003,
> -};
> -
> -static const struct dmi_system_id fwbug_list[] = {
> -	{
> -		.ident = "ROG Zephyrus G14",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "GA403U"),
> -		},
> -		.driver_data = &quirk_no_sps_bug,
> -	},
> -	{
> -		.ident = "ROG Ally X",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "RC72LA"),
> -		},
> -		.driver_data = &quirk_no_sps_bug,
> -	},
> -	{
> -		.ident = "ASUS TUF Gaming A14",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "FA401W"),
> -		},
> -		.driver_data = &quirk_no_sps_bug,
> -	},
> -	{}
> -};
> -
> -void amd_pmf_quirks_init(struct amd_pmf_dev *dev)
> -{
> -	const struct dmi_system_id *dmi_id;
> -	struct quirk_entry *quirks;
> -
> -	dmi_id = dmi_first_match(fwbug_list);
> -	if (!dmi_id)
> -		return;
> -
> -	quirks = dmi_id->driver_data;
> -	if (quirks->supported_func) {
> -		dev->supported_func = quirks->supported_func;
> -		pr_info("Using supported funcs quirk to avoid %s platform firmware bug\n",
> -			dmi_id->ident);
> -	}
> -}
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 8ce8816da9c16..b89aa38434faa 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -795,7 +795,4 @@ int amd_pmf_smartpc_apply_bios_output(struct amd_pmf_dev *dev, u32 val, u32 preq
>  void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
>  void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
>  
> -/* Quirk infrastructure */
> -void amd_pmf_quirks_init(struct amd_pmf_dev *dev);
> -
>  #endif /* PMF_H */

Re: [PATCH v2 00/15] Add support for binding ACPI platform profile to multiple drivers

[Hans de Goede]

Hi Mario,

On 28-Oct-24 3:01 AM, Mario Limonciello wrote:
> Currently there are a number of ASUS products on the market that happen to
> have ACPI objects for amd-pmf to bind to as well as an ACPI platform profile
> provided by asus-wmi.
> 
> The ACPI platform profile support created by amd-pmf on these ASUS products is "Function 9"
> which is specifically for "BIOS or EC notification" of power slider position.
> This feature is actively used by some designs such as Framework 13 and Framework 16.
> 
> On these ASUS designs we keep on quirking more and more of them to turn off this
> notification so that asus-wmi can bind.
> 
> This however isn't how Windows works.  "Multiple" things are notified for the power
> slider position. This series adjusts Linux to behave similarly.
> 
> Multiple drivers can now register an ACPI platform profile and will react to set requests.
> 
> To avoid chaos, only positions that are common to both drivers are accepted.
> 
> This also allows dropping all of the PMF quirks from amd-pmf.
> 
> v2:
>  * Split to many more patches
>  * Account for feedback from M/L

Thank you for the new version. I just did a quick check of
patches 8 - 13 and this looks much better.

I see from various discussions that a v3 is incoming so I've
not done a full review of patches 8 - 13.

Regards,

Hans





> 
> Mario Limonciello (15):
>   ACPI: platform-profile: Add a name member to handlers
>   platform/surface: aggregator: Add platform handler pointer to device
>   ACPI: platform_profile: Add platform handler argument to
>     platform_profile_remove()
>   ACPI: platform_profile: Add a list to platform profile handler
>   ACPI: platform_profile: Move sanity check out of the mutex
>   ACPI: platform_profile: Use guard(mutex) for register/unregister
>   ACPI: platform_profile: Only remove group when no more handler
>     registered
>   ACPI: platform_profile: Require handlers to support balanced profile
>   ACPI: platform_profile: Notify change events on register and
>     unregister
>   ACPI: platform_profile: Only show profiles common for all handlers
>   ACPI: platform_profile: Set profile for all registered handlers
>   ACPI: platform_profile: Make sure all profile handlers agree on
>     profile
>   ACPI: platform_profile: Check all profile handler to calculate next
>   ACPI: platform_profile: Allow multiple handlers
>   platform/x86/amd: pmf: Drop all quirks
> 
>  drivers/acpi/platform_profile.c               | 258 +++++++++++-------
>  .../surface/surface_platform_profile.c        |   7 +-
>  drivers/platform/x86/acer-wmi.c               |   5 +-
>  drivers/platform/x86/amd/pmf/Makefile         |   2 +-
>  drivers/platform/x86/amd/pmf/core.c           |   1 -
>  drivers/platform/x86/amd/pmf/pmf-quirks.c     |  66 -----
>  drivers/platform/x86/amd/pmf/pmf.h            |   3 -
>  drivers/platform/x86/amd/pmf/sps.c            |   3 +-
>  drivers/platform/x86/asus-wmi.c               |   5 +-
>  drivers/platform/x86/dell/dell-pc.c           |   3 +-
>  drivers/platform/x86/hp/hp-wmi.c              |   3 +-
>  drivers/platform/x86/ideapad-laptop.c         |   3 +-
>  .../platform/x86/inspur_platform_profile.c    |   5 +-
>  drivers/platform/x86/thinkpad_acpi.c          |   3 +-
>  include/linux/platform_profile.h              |   4 +-
>  15 files changed, 190 insertions(+), 181 deletions(-)
>  delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
> 

Re: [PATCH v2 00/15] Add support for binding ACPI platform profile to multiple drivers

[Mario Limonciello]

On 10/30/2024 08:41, Hans de Goede wrote:
> Hi Mario,
> 
> On 28-Oct-24 3:01 AM, Mario Limonciello wrote:
>> Currently there are a number of ASUS products on the market that happen to
>> have ACPI objects for amd-pmf to bind to as well as an ACPI platform profile
>> provided by asus-wmi.
>>
>> The ACPI platform profile support created by amd-pmf on these ASUS products is "Function 9"
>> which is specifically for "BIOS or EC notification" of power slider position.
>> This feature is actively used by some designs such as Framework 13 and Framework 16.
>>
>> On these ASUS designs we keep on quirking more and more of them to turn off this
>> notification so that asus-wmi can bind.
>>
>> This however isn't how Windows works.  "Multiple" things are notified for the power
>> slider position. This series adjusts Linux to behave similarly.
>>
>> Multiple drivers can now register an ACPI platform profile and will react to set requests.
>>
>> To avoid chaos, only positions that are common to both drivers are accepted.
>>
>> This also allows dropping all of the PMF quirks from amd-pmf.
>>
>> v2:
>>   * Split to many more patches
>>   * Account for feedback from M/L
> 
> Thank you for the new version. I just did a quick check of
> patches 8 - 13 and this looks much better.
> 
> I see from various discussions that a v3 is incoming so I've
> not done a full review of patches 8 - 13.
> 

Yeah no problem waiting for v3 for more complete review, I haven't 
decided if I'm squashing the class stuff in or doing it in separate 
patches.  Once I have it all working in a satisfactory way I'll decide.

> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>>
>> Mario Limonciello (15):
>>    ACPI: platform-profile: Add a name member to handlers
>>    platform/surface: aggregator: Add platform handler pointer to device
>>    ACPI: platform_profile: Add platform handler argument to
>>      platform_profile_remove()
>>    ACPI: platform_profile: Add a list to platform profile handler
>>    ACPI: platform_profile: Move sanity check out of the mutex
>>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>>    ACPI: platform_profile: Only remove group when no more handler
>>      registered
>>    ACPI: platform_profile: Require handlers to support balanced profile
>>    ACPI: platform_profile: Notify change events on register and
>>      unregister
>>    ACPI: platform_profile: Only show profiles common for all handlers
>>    ACPI: platform_profile: Set profile for all registered handlers
>>    ACPI: platform_profile: Make sure all profile handlers agree on
>>      profile
>>    ACPI: platform_profile: Check all profile handler to calculate next
>>    ACPI: platform_profile: Allow multiple handlers
>>    platform/x86/amd: pmf: Drop all quirks
>>
>>   drivers/acpi/platform_profile.c               | 258 +++++++++++-------
>>   .../surface/surface_platform_profile.c        |   7 +-
>>   drivers/platform/x86/acer-wmi.c               |   5 +-
>>   drivers/platform/x86/amd/pmf/Makefile         |   2 +-
>>   drivers/platform/x86/amd/pmf/core.c           |   1 -
>>   drivers/platform/x86/amd/pmf/pmf-quirks.c     |  66 -----
>>   drivers/platform/x86/amd/pmf/pmf.h            |   3 -
>>   drivers/platform/x86/amd/pmf/sps.c            |   3 +-
>>   drivers/platform/x86/asus-wmi.c               |   5 +-
>>   drivers/platform/x86/dell/dell-pc.c           |   3 +-
>>   drivers/platform/x86/hp/hp-wmi.c              |   3 +-
>>   drivers/platform/x86/ideapad-laptop.c         |   3 +-
>>   .../platform/x86/inspur_platform_profile.c    |   5 +-
>>   drivers/platform/x86/thinkpad_acpi.c          |   3 +-
>>   include/linux/platform_profile.h              |   4 +-
>>   15 files changed, 190 insertions(+), 181 deletions(-)
>>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>>
>