[PATCH v6 00/22] Add support for binding ACPI platform profile to multiple drivers

[PATCH v6 00/22] 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 when the legacy /sys/firmware/acpi/platform_profile interface
is used.

This series also adds a new concept of a "custom" profile.  This allows
userspace to discover that there are multiple driver handlers that are
configured differently.

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

---
v6:
 * Add patch dev patch but don't make mandatory
 * See other patches changelogs for individualized changes

Mario Limonciello (22):
  ACPI: platform-profile: Add a name member to handlers
  platform/x86/dell: dell-pc: Create platform device
  ACPI: platform_profile: Add device pointer into platform profile
    handler
  ACPI: platform_profile: Add platform handler argument to
    platform_profile_remove()
  ACPI: platform_profile: Pass the profile handler into
    platform_profile_notify()
  ACPI: platform_profile: Move sanity check out of the mutex
  ACPI: platform_profile: Move matching string for new profile out of
    mutex
  ACPI: platform_profile: Use guard(mutex) for register/unregister
  ACPI: platform_profile: Use `scoped_cond_guard`
  ACPI: platform_profile: Create class for ACPI platform profile
  ACPI: platform_profile: Add name attribute to class interface
  ACPI: platform_profile: Add choices attribute for class interface
  ACPI: platform_profile: Add profile attribute for class interface
  ACPI: platform_profile: Notify change events on register and
    unregister
  ACPI: platform_profile: Only show profiles common for all handlers
  ACPI: platform_profile: Add concept of a "custom" profile
  ACPI: platform_profile: Make sure all profile handlers agree on
    profile
  ACPI: platform_profile: Check all profile handler to calculate next
  ACPI: platform_profile: Notify class device from
    platform_profile_notify()
  ACPI: platform_profile: Allow multiple handlers
  platform/x86/amd: pmf: Drop all quirks
  Documentation: Add documentation about class interface for platform
    profiles

 .../ABI/testing/sysfs-platform_profile        |   5 +
 .../userspace-api/sysfs-platform_profile.rst  |  28 +
 drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
 .../surface/surface_platform_profile.c        |   8 +-
 drivers/platform/x86/acer-wmi.c               |  12 +-
 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            |   4 +-
 drivers/platform/x86/asus-wmi.c               |  10 +-
 drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
 drivers/platform/x86/dell/dell-pc.c           |  36 +-
 drivers/platform/x86/hp/hp-wmi.c              |   8 +-
 drivers/platform/x86/ideapad-laptop.c         |   6 +-
 .../platform/x86/inspur_platform_profile.c    |   7 +-
 drivers/platform/x86/thinkpad_acpi.c          |  16 +-
 include/linux/platform_profile.h              |   9 +-
 18 files changed, 553 insertions(+), 213 deletions(-)
 delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c


base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
-- 
2.43.0

[PATCH v6 01/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * pick up tag
---
 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/alienware-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 +
 11 files changed, 11 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/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index a800c28bb4d51..ac0038afd98fa 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1056,6 +1056,7 @@ static int create_thermal_profile(void)
 
 	pp_handler.profile_get = thermal_profile_get;
 	pp_handler.profile_set = thermal_profile_set;
+	pp_handler.name = "alienware-wmi";
 
 	return platform_profile_register(&pp_handler);
 }
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);

base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
-- 
2.43.0

[PATCH v6 02/22] platform/x86/dell: dell-pc: Create platform device

[Mario Limonciello]

In order to have a device for the platform profile core to reference
create a platform device for dell-pc.

While doing this change the memory allocation for the thermal handler
to be device managed to follow the lifecycle of that device.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Use PLATFORM_DEVID_NONE (Armin)
v5:
 * use platform_device_register_simple()
---
 drivers/platform/x86/dell/dell-pc.c | 32 +++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 3cf79e55e3129..805497e44b3a5 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -18,10 +18,13 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_profile.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #include "dell-smbios.h"
 
+static struct platform_device *platform_device;
+
 static const struct dmi_system_id dell_device_table[] __initconst = {
 	{
 		.ident = "Dell Inc.",
@@ -244,9 +247,15 @@ static int thermal_init(void)
 	if (!supported_modes)
 		return 0;
 
-	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
-	if (!thermal_handler)
+	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
+	if (!platform_device)
 		return -ENOMEM;
+
+	thermal_handler = devm_kzalloc(&platform_device->dev, sizeof(*thermal_handler), GFP_KERNEL);
+	if (!thermal_handler) {
+		ret = -ENOMEM;
+		goto cleanup_platform_device;
+	}
 	thermal_handler->name = "dell-pc";
 	thermal_handler->profile_get = thermal_platform_profile_get;
 	thermal_handler->profile_set = thermal_platform_profile_set;
@@ -262,20 +271,25 @@ static int thermal_init(void)
 
 	/* Clean up if failed */
 	ret = platform_profile_register(thermal_handler);
-	if (ret) {
-		kfree(thermal_handler);
-		thermal_handler = NULL;
-	}
+	if (ret)
+		goto cleanup_thermal_handler;
+
+	return 0;
+
+cleanup_thermal_handler:
+	thermal_handler = NULL;
+
+cleanup_platform_device:
+	platform_device_unregister(platform_device);
 
 	return ret;
 }
 
 static void thermal_cleanup(void)
 {
-	if (thermal_handler) {
+	if (thermal_handler)
 		platform_profile_remove();
-		kfree(thermal_handler);
-	}
+	platform_device_unregister(platform_device);
 }
 
 static int __init dell_init(void)
-- 
2.43.0

[PATCH v6 03/22] ACPI: platform_profile: Add device pointer into platform profile handler

[Mario Limonciello]

In order to let platform profile handlers manage platform profile
for their driver the core code will need a pointer to the device.

Add this to the structure and use it in the trivial driver cases.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Don't require to be set
v4:
 * add alienware-wmi too
---
 drivers/platform/surface/surface_platform_profile.c | 1 +
 drivers/platform/x86/acer-wmi.c                     | 5 +++--
 drivers/platform/x86/amd/pmf/sps.c                  | 1 +
 drivers/platform/x86/asus-wmi.c                     | 1 +
 drivers/platform/x86/dell/alienware-wmi.c           | 5 +++--
 drivers/platform/x86/dell/dell-pc.c                 | 1 +
 drivers/platform/x86/hp/hp-wmi.c                    | 5 +++--
 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 +
 11 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 61aa488a80eb5..5f45f8e8cd69b 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -212,6 +212,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 	tpd->sdev = sdev;
 
 	tpd->handler.name = "Surface Platform Profile";
+	tpd->handler.dev = &sdev->dev;
 	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 53fbc9b4d3df7..aca4a5746bee1 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1873,12 +1873,13 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
-static int acer_platform_profile_setup(void)
+static int acer_platform_profile_setup(struct platform_device *device)
 {
 	if (quirks->predator_v4) {
 		int err;
 
 		platform_profile_handler.name = "acer-wmi";
+		platform_profile_handler.dev = &device->dev;
 		platform_profile_handler.profile_get =
 			acer_predator_v4_platform_profile_get;
 		platform_profile_handler.profile_set =
@@ -2531,7 +2532,7 @@ static int acer_platform_probe(struct platform_device *device)
 		goto error_rfkill;
 
 	if (has_cap(ACER_CAP_PLATFORM_PROFILE)) {
-		err = acer_platform_profile_setup();
+		err = acer_platform_profile_setup(device);
 		if (err)
 			goto error_platform_profile;
 	}
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index e2d0cc92c4396..1b94af7c0e0c4 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -406,6 +406,7 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 	}
 
 	dev->pprof.name = "amd-pmf";
+	dev->pprof.dev = dev->dev;
 	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 c7c104c65a85a..78621b2af0ddb 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3911,6 +3911,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.dev = dev;
 	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/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index ac0038afd98fa..c03b1aa7dfb5f 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1017,7 +1017,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
 	return wmax_thermal_control(supported_thermal_profiles[profile]);
 }
 
-static int create_thermal_profile(void)
+static int create_thermal_profile(struct platform_device *platform_device)
 {
 	u32 out_data;
 	enum wmax_thermal_mode mode;
@@ -1057,6 +1057,7 @@ static int create_thermal_profile(void)
 	pp_handler.profile_get = thermal_profile_get;
 	pp_handler.profile_set = thermal_profile_set;
 	pp_handler.name = "alienware-wmi";
+	pp_handler.dev = &platform_device->dev;
 
 	return platform_profile_register(&pp_handler);
 }
@@ -1125,7 +1126,7 @@ static int __init alienware_wmi_init(void)
 	}
 
 	if (quirks->thermal) {
-		ret = create_thermal_profile();
+		ret = create_thermal_profile(platform_device);
 		if (ret)
 			goto fail_prep_thermal_profile;
 	}
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 805497e44b3a5..e5724e76a2082 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -257,6 +257,7 @@ static int thermal_init(void)
 		goto cleanup_platform_device;
 	}
 	thermal_handler->name = "dell-pc";
+	thermal_handler->dev = &platform_device->dev;
 	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 26cac73caf2b9..ffb09799142bc 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1565,7 +1565,7 @@ static inline void omen_unregister_powersource_event_handler(void)
 	unregister_acpi_notifier(&platform_power_source_nb);
 }
 
-static int thermal_profile_setup(void)
+static int thermal_profile_setup(struct platform_device *device)
 {
 	int err, tp;
 
@@ -1625,6 +1625,7 @@ static int thermal_profile_setup(void)
 	}
 
 	platform_profile_handler.name = "hp-wmi";
+	platform_profile_handler.dev = &device->dev;
 	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
 	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
 
@@ -1664,7 +1665,7 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 	if (err < 0)
 		return err;
 
-	thermal_profile_setup();
+	thermal_profile_setup(device);
 
 	return 0;
 }
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 1f94c14c3b832..24367c3590c99 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1103,6 +1103,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 	mutex_init(&priv->dytc->mutex);
 
 	priv->dytc->pprof.name = "ideapad-laptop";
+	priv->dytc->pprof.dev = &priv->platform_device->dev;
 	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 03da2c8cf6789..5a53949bbbf5f 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -178,6 +178,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 	dev_set_drvdata(&wdev->dev, priv);
 
 	priv->handler.name = "inspur-wmi";
+	priv->handler.dev = &wdev->dev;
 	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 c8c316b8507a5..222fba97d79a7 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10616,6 +10616,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 	dbg_printk(TPACPI_DBG_INIT,
 			"DYTC version %d: thermal mode available\n", dytc_version);
 
+	dytc_profile.dev = &tpacpi_pdev->dev;
 	/* Create platform_profile structure and register */
 	err = platform_profile_register(&dytc_profile);
 	/*
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 6fa988e417428..daec6b9bad81f 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -28,6 +28,7 @@ enum platform_profile_option {
 
 struct platform_profile_handler {
 	const char *name;
+	struct device *dev;
 	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

[PATCH v6 04/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * Add tag
v4:
 * Add alienware-wmi too
---
 drivers/acpi/platform_profile.c                     | 2 +-
 drivers/platform/surface/surface_platform_profile.c | 6 +++++-
 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/alienware-wmi.c           | 2 +-
 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 +-
 12 files changed, 21 insertions(+), 14 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 5f45f8e8cd69b..b449c4c8b883c 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.dev = &sdev->dev;
@@ -228,7 +229,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 aca4a5746bee1..b12965d9fcdb7 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -2547,7 +2547,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:
@@ -2570,7 +2570,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 1b94af7c0e0c4..bd2bd6cfc39a0 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -426,5 +426,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 78621b2af0ddb..0750e2fe65325 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4886,7 +4886,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);
@@ -4913,7 +4913,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/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index c03b1aa7dfb5f..3cf6c371d4220 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1065,7 +1065,7 @@ static int create_thermal_profile(struct platform_device *platform_device)
 static void remove_thermal_profile(void)
 {
 	if (quirks->thermal)
-		platform_profile_remove();
+		platform_profile_remove(&pp_handler);
 }
 
 static int __init alienware_wmi_init(void)
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index e5724e76a2082..5c4d48e4b667a 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -289,7 +289,7 @@ static int thermal_init(void)
 static void thermal_cleanup(void)
 {
 	if (thermal_handler)
-		platform_profile_remove();
+		platform_profile_remove(thermal_handler);
 	platform_device_unregister(platform_device);
 }
 
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index ffb09799142bc..6d6e13a0c6e2d 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1693,7 +1693,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 24367c3590c99..80797c6ae8b0b 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1136,7 +1136,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 5a53949bbbf5f..53af73a7fbf7b 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -191,7 +191,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 222fba97d79a7..13798c6d5fcf3 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10638,7 +10638,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 daec6b9bad81f..bcaf3aa39160f 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -37,7 +37,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

[PATCH v6 05/22] ACPI: platform_profile: Pass the profile handler into platform_profile_notify()

[Mario Limonciello]

The profile handler will be used to notify the appropriate class
devices.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c       |  2 +-
 drivers/platform/x86/acer-wmi.c       |  2 +-
 drivers/platform/x86/asus-wmi.c       |  4 ++--
 drivers/platform/x86/ideapad-laptop.c |  2 +-
 drivers/platform/x86/thinkpad_acpi.c  | 14 +++++++-------
 include/linux/platform_profile.h      |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c24744da20916..927a2f7456c9a 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -128,7 +128,7 @@ static const struct attribute_group platform_profile_group = {
 	.attrs = platform_profile_attrs
 };
 
-void platform_profile_notify(void)
+void platform_profile_notify(struct platform_profile_handler *pprof)
 {
 	if (!cur_profile)
 		return;
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index b12965d9fcdb7..0018818258070 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1988,7 +1988,7 @@ static int acer_thermal_profile_change(void)
 		if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
 			last_non_turbo_profile = tp;
 
-		platform_profile_notify();
+		platform_profile_notify(&platform_profile_handler);
 	}
 
 	return 0;
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 0750e2fe65325..2a8d789ee05cf 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3754,7 +3754,7 @@ static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
 	 * Ensure that platform_profile updates userspace with the change to ensure
 	 * that platform_profile and throttle_thermal_policy_mode are in sync.
 	 */
-	platform_profile_notify();
+	platform_profile_notify(&asus->platform_profile_handler);
 
 	return 0;
 }
@@ -3793,7 +3793,7 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 	 * Ensure that platform_profile updates userspace with the change to ensure
 	 * that platform_profile and throttle_thermal_policy_mode are in sync.
 	 */
-	platform_profile_notify();
+	platform_profile_notify(&asus->platform_profile_handler);
 
 	return count;
 }
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 80797c6ae8b0b..79c65c24b433a 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1041,7 +1041,7 @@ static void dytc_profile_refresh(struct ideapad_private *priv)
 
 	if (profile != priv->dytc->current_profile) {
 		priv->dytc->current_profile = profile;
-		platform_profile_notify();
+		platform_profile_notify(&priv->dytc->pprof);
 	}
 }
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 13798c6d5fcf3..8539cac042be8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10516,6 +10516,12 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	return err;
 }
 
+static struct platform_profile_handler dytc_profile = {
+	.name = "thinkpad-acpi",
+	.profile_get = dytc_profile_get,
+	.profile_set = dytc_profile_set,
+};
+
 static void dytc_profile_refresh(void)
 {
 	enum platform_profile_option profile;
@@ -10544,16 +10550,10 @@ static void dytc_profile_refresh(void)
 	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
 	if (!err && profile != dytc_current_profile) {
 		dytc_current_profile = profile;
-		platform_profile_notify();
+		platform_profile_notify(&dytc_profile);
 	}
 }
 
-static struct platform_profile_handler dytc_profile = {
-	.name = "thinkpad-acpi",
-	.profile_get = dytc_profile_get,
-	.profile_set = dytc_profile_set,
-};
-
 static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 {
 	int err, output;
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index bcaf3aa39160f..8ec0b8da56db5 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -39,6 +39,6 @@ struct platform_profile_handler {
 int platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_remove(struct platform_profile_handler *pprof);
 int platform_profile_cycle(void);
-void platform_profile_notify(void);
+void platform_profile_notify(struct platform_profile_handler *pprof);
 
 #endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.43.0

[PATCH v6 06/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * Add tag
---
 drivers/acpi/platform_profile.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 927a2f7456c9a..4f5623fc27c09 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -179,6 +179,13 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
 
+	/* Sanity check the profile handler */
+	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;
+	}
+
 	mutex_lock(&profile_lock);
 	/* We can only have one active profile */
 	if (cur_profile) {
@@ -186,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

[PATCH v6 07/22] ACPI: platform_profile: Move matching string for new profile out of mutex

[Mario Limonciello]

Holding the mutex is not necessary while scanning the string passed into
platform_profile_store().

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 4f5623fc27c09..45ffd85a71dd5 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -83,6 +83,11 @@ static ssize_t platform_profile_store(struct device *dev,
 {
 	int err, i;
 
+	/* Scan for a matching profile */
+	i = sysfs_match_string(profile_names, buf);
+	if (i < 0)
+		return -EINVAL;
+
 	err = mutex_lock_interruptible(&profile_lock);
 	if (err)
 		return err;
@@ -92,13 +97,6 @@ static ssize_t platform_profile_store(struct device *dev,
 		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);
-- 
2.43.0

[PATCH v6 08/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 45ffd85a71dd5..9729543df6333 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -184,32 +184,26 @@ 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;
-	}
 
 	cur_profile = pprof;
-	mutex_unlock(&profile_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
 
 int platform_profile_remove(struct platform_profile_handler *pprof)
 {
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	guard(mutex)(&profile_lock);
 
-	mutex_lock(&profile_lock);
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 	cur_profile = NULL;
-	mutex_unlock(&profile_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
-- 
2.43.0

[PATCH v6 09/22] ACPI: platform_profile: Use `scoped_cond_guard`

[Mario Limonciello]

Migrate away from using an interruptible mutex to scoped_cond_guard
in all functions. While changing, move the sysfs notification
used in platform_profile_store() outside of mutex scope.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 111 +++++++++++++-------------------
 1 file changed, 43 insertions(+), 68 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 9729543df6333..32affb75e782d 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -27,25 +27,20 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 					char *buf)
 {
 	int len = 0;
-	int err, i;
-
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
-
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
-
-	for_each_set_bit(i, cur_profile->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]);
+	int i;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!cur_profile)
+			return -ENODEV;
+		for_each_set_bit(i, cur_profile->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;
 }
 
@@ -56,20 +51,15 @@ 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;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!cur_profile)
+			return -ENODEV;
 
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
+		err = cur_profile->profile_get(cur_profile, &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;
@@ -88,28 +78,21 @@ static ssize_t platform_profile_store(struct device *dev,
 	if (i < 0)
 		return -EINVAL;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!cur_profile)
+			return -ENODEV;
 
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
+		/* Check that platform supports this profile choice */
+		if (!test_bit(i, cur_profile->choices))
+			return -EOPNOTSUPP;
 
-	/* Check that platform supports this profile choice */
-	if (!test_bit(i, cur_profile->choices)) {
-		mutex_unlock(&profile_lock);
-		return -EOPNOTSUPP;
+		err = cur_profile->profile_set(cur_profile, i);
+		if (err)
+			return err;
 	}
 
-	err = cur_profile->profile_set(cur_profile, i);
-	if (!err)
-		sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	mutex_unlock(&profile_lock);
-	if (err)
-		return err;
 	return count;
 }
 
@@ -140,36 +123,28 @@ int platform_profile_cycle(void)
 	enum platform_profile_option next;
 	int err;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!cur_profile)
+			return -ENODEV;
 
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
+		err = cur_profile->profile_get(cur_profile, &profile);
+		if (err)
+			return err;
 
-	err = cur_profile->profile_get(cur_profile, &profile);
-	if (err) {
-		mutex_unlock(&profile_lock);
-		return err;
-	}
+		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
+					  profile + 1);
 
-	next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
-				  profile + 1);
+		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
+			return -EINVAL;
 
-	if (WARN_ON(next == PLATFORM_PROFILE_LAST)) {
-		mutex_unlock(&profile_lock);
-		return -EINVAL;
+		err = cur_profile->profile_set(cur_profile, next);
+		if (err)
+			return err;
 	}
 
-	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 v6 10/22] ACPI: platform_profile: Create class for ACPI platform profile

[Mario Limonciello]

When registering a platform profile handler create a class device
that will allow changing a single platform profile handler.

The class and sysfs group are no longer needed when the platform profile
core is a module and unloaded, so remove them at that time as well.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Catch failures in ida_alloc
 * Use 4th argument of device_create instead of dev_set_drvdata()
 * Squash unregister patch
 * Add module init callback
 * Move class creation to module init
 * Update visibility based on group presence
 * Add back parent device
v5:
 * Use ida instead of idr
 * Use device_unregister instead of device_destroy()
 * MKDEV (0, 0)
---
 drivers/acpi/platform_profile.c  | 88 ++++++++++++++++++++++++++++++--
 include/linux/platform_profile.h |  2 +
 2 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 32affb75e782d..ef6af2c655524 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -5,6 +5,7 @@
 #include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/init.h>
+#include <linux/kdev_t.h>
 #include <linux/mutex.h>
 #include <linux/platform_profile.h>
 #include <linux/sysfs.h>
@@ -22,6 +23,12 @@ static const char * const profile_names[] = {
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
+static DEFINE_IDA(platform_profile_ida);
+
+static const struct class platform_profile_class = {
+	.name = "platform-profile",
+};
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -105,8 +112,25 @@ static struct attribute *platform_profile_attrs[] = {
 	NULL
 };
 
+static int profile_class_registered(struct device *dev, const void *data)
+{
+	return 1;
+}
+
+static umode_t profile_class_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
+{
+	if (!class_find_device(&platform_profile_class, NULL, NULL, profile_class_registered))
+		return 0;
+	if (attr == &dev_attr_platform_profile_choices.attr)
+		return 0444;
+	if (attr == &dev_attr_platform_profile.attr)
+		return 0644;
+	return 0;
+}
+
 static const struct attribute_group platform_profile_group = {
-	.attrs = platform_profile_attrs
+	.attrs = platform_profile_attrs,
+	.is_visible = profile_class_is_visible,
 };
 
 void platform_profile_notify(struct platform_profile_handler *pprof)
@@ -123,6 +147,9 @@ int platform_profile_cycle(void)
 	enum platform_profile_option next;
 	int err;
 
+	if (!class_is_registered(&platform_profile_class))
+		return -ENODEV;
+
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		if (!cur_profile)
 			return -ENODEV;
@@ -164,25 +191,76 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	if (cur_profile)
 		return -EEXIST;
 
-	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
-	if (err)
-		return err;
+	/* create class interface for individual handler */
+	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
+	if (pprof->minor < 0)
+		return pprof->minor;
+	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
+					 MKDEV(0, 0), pprof, "platform-profile-%d",
+					 pprof->minor);
+	if (IS_ERR(pprof->class_dev)) {
+		err = PTR_ERR(pprof->class_dev);
+		goto cleanup_ida;
+	}
 
 	cur_profile = pprof;
+
+	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
+	if (err)
+		goto cleanup_cur;
+
 	return 0;
+
+cleanup_cur:
+	cur_profile = NULL;
+	device_unregister(pprof->class_dev);
+
+cleanup_ida:
+	ida_free(&platform_profile_ida, pprof->minor);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
 
 int platform_profile_remove(struct platform_profile_handler *pprof)
 {
+	int id;
 	guard(mutex)(&profile_lock);
 
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	id = pprof->minor;
+	device_unregister(pprof->class_dev);
+	ida_free(&platform_profile_ida, id);
+
 	cur_profile = NULL;
+
+	sysfs_update_group(acpi_kobj, &platform_profile_group);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
 
+static int __init platform_profile_init(void)
+{
+	int err;
+
+	err = class_register(&platform_profile_class);
+	if (err)
+		return err;
+
+	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
+	if (err)
+		class_unregister(&platform_profile_class);
+
+	return err;
+}
+static void __exit platform_profile_exit(void)
+{
+	class_unregister(&platform_profile_class);
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+}
+module_init(platform_profile_init);
+module_exit(platform_profile_exit);
+
 MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
 MODULE_DESCRIPTION("ACPI platform profile sysfs interface");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 8ec0b8da56db5..a888fd085c513 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -29,6 +29,8 @@ enum platform_profile_option {
 struct platform_profile_handler {
 	const char *name;
 	struct device *dev;
+	struct device *class_dev;
+	int minor;
 	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

[PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Mario Limonciello]

The name attribute shows the name of the associated platform profile
handler.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index ef6af2c655524..4e2eda18f7f5f 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
 static DEFINE_IDA(platform_profile_ida);
 
+/**
+ * name_show - Show the name of the profile handler
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to write to
+ * Return: The number of bytes written
+ */
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct platform_profile_handler *handler = dev_get_drvdata(dev);
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		return sysfs_emit(buf, "%s\n", handler->name);
+	}
+	return -ERESTARTSYS;
+}
+
+static DEVICE_ATTR_RO(name);
+static struct attribute *profile_attrs[] = {
+	&dev_attr_name.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(profile);
+
 static const struct class platform_profile_class = {
 	.name = "platform-profile",
+	.dev_groups = profile_groups,
 };
 
 static ssize_t platform_profile_choices_show(struct device *dev,
-- 
2.43.0

[PATCH v6 12/22] ACPI: platform_profile: Add choices attribute for class interface

[Mario Limonciello]

The `choices` file will show all possible choices that a given platform
profile handler can support.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * Fix kdoc
 * Add tag
 * Fix whitespace
 * Adjust mutex use
---
 drivers/acpi/platform_profile.c | 44 +++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 4e2eda18f7f5f..86d0eb7cc6fc3 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -25,6 +25,27 @@ static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
 static DEFINE_IDA(platform_profile_ida);
 
+/**
+ * _commmon_choices_show - Show the available profile choices
+ * @choices: The available profile choices
+ * @buf: The buffer to write to
+ * Return: The number of bytes written
+ */
+static ssize_t _commmon_choices_show(unsigned long *choices, char *buf)
+{
+	int i, len = 0;
+
+	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");
+
+	return len;
+}
+
 /**
  * name_show - Show the name of the profile handler
  * @dev: The device
@@ -44,9 +65,32 @@ static ssize_t name_show(struct device *dev,
 	return -ERESTARTSYS;
 }
 
+/**
+ * choices_show - Show the available profile choices
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to write to
+ */
+static ssize_t choices_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct platform_profile_handler *handler;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		handler = dev_get_drvdata(dev);
+		return _commmon_choices_show(handler->choices, buf);
+	}
+
+	return -ERESTARTSYS;
+}
+
 static DEVICE_ATTR_RO(name);
+static DEVICE_ATTR_RO(choices);
+
 static struct attribute *profile_attrs[] = {
 	&dev_attr_name.attr,
+	&dev_attr_choices.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(profile);
-- 
2.43.0

[PATCH v6 13/22] ACPI: platform_profile: Add profile attribute for class interface

[Mario Limonciello]

Reading and writing the `profile` sysfs file will use the callbacks for
the platform profile handler to read or set the given profile.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Fix return
v5:
 * Drop recovery flow
 * Don't get profile before setting (not needed)
 * Simplify casting for call to _store_class_profile()
 * Only notify legacy interface of changes
 * Adjust mutex use
---
 drivers/acpi/platform_profile.c | 106 ++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 86d0eb7cc6fc3..c5487d3928c1b 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -46,6 +46,58 @@ static ssize_t _commmon_choices_show(unsigned long *choices, char *buf)
 	return len;
 }
 
+/**
+ * _store_class_profile - Set the profile for a class device
+ * @dev: The class device
+ * @data: The profile to set
+ */
+static int _store_class_profile(struct device *dev, void *data)
+{
+	struct platform_profile_handler *handler;
+	int *i = (int *)data;
+	int err;
+
+	lockdep_assert_held(&profile_lock);
+	handler = dev_get_drvdata(dev);
+	if (!test_bit(*i, handler->choices))
+		return -EOPNOTSUPP;
+
+	handler = dev_get_drvdata(dev);
+	err = handler->profile_set(handler, *i);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/**
+ * get_class_profile - Show the current profile for a class device
+ * @dev: The class device
+ * @profile: The profile to return
+ * Return: 0 on success, -errno on failure
+ */
+static int get_class_profile(struct device *dev,
+			     enum platform_profile_option *profile)
+{
+	struct platform_profile_handler *handler;
+	enum platform_profile_option val;
+	int err;
+
+	lockdep_assert_held(&profile_lock);
+	handler = dev_get_drvdata(dev);
+	err = handler->profile_get(handler, &val);
+	if (err) {
+		pr_err("Failed to get profile for handler %s\n", handler->name);
+		return err;
+	}
+
+	if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
+		return -EINVAL;
+	*profile = val;
+
+	return 0;
+}
+
 /**
  * name_show - Show the name of the profile handler
  * @dev: The device
@@ -85,12 +137,66 @@ static ssize_t choices_show(struct device *dev,
 	return -ERESTARTSYS;
 }
 
+/**
+ * profile_show - Show the current profile for a class device
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to write to
+ * Return: The number of bytes written
+ */
+static ssize_t profile_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
+	int err;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		err = get_class_profile(dev, &profile);
+		if (err)
+			return err;
+	}
+
+	return sysfs_emit(buf, "%s\n", profile_names[profile]);
+}
+
+/**
+ * profile_store - Set the profile for a class device
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to read from
+ * @count: The number of bytes to read
+ * Return: The number of bytes read
+ */
+static ssize_t profile_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	int i, ret;
+
+	i = sysfs_match_string(profile_names, buf);
+	if (i < 0)
+		return -EINVAL;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		ret = _store_class_profile(dev, &i);
+		if (ret)
+			return ret;
+	}
+
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
+	return count;
+}
+
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RO(choices);
+static DEVICE_ATTR_RW(profile);
 
 static struct attribute *profile_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_choices.attr,
+	&dev_attr_profile.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(profile);
-- 
2.43.0

[PATCH v6 14/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c5487d3928c1b..c4d451782ff18 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -380,6 +380,8 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		goto cleanup_ida;
 	}
 
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
 	cur_profile = pprof;
 
 	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
@@ -408,6 +410,8 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 	device_unregister(pprof->class_dev);
 	ida_free(&platform_profile_ida, id);
 
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
 	cur_profile = NULL;
 
 	sysfs_update_group(acpi_kobj, &platform_profile_group);
-- 
2.43.0

[PATCH v6 15/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * adjust mutex use
---
 drivers/acpi/platform_profile.c | 56 ++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c4d451782ff18..c4e2c75116988 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -206,26 +206,54 @@ static const struct class platform_profile_class = {
 	.dev_groups = profile_groups,
 };
 
+/**
+ * _aggregate_choices - Aggregate the available profile choices
+ * @dev: The device
+ * @data: The available profile choices
+ * Return: 0 on success, -errno on failure
+ */
+static int _aggregate_choices(struct device *dev, void *data)
+{
+	struct platform_profile_handler *handler;
+	unsigned long *aggregate = data;
+
+	lockdep_assert_held(&profile_lock);
+	handler = dev_get_drvdata(dev);
+	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
+		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
+	else
+		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
+
+	return 0;
+}
+
+/**
+ * platform_profile_choices_show - Show the available profile choices for legacy sysfs interface
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to write to
+ * Return: The number of bytes written
+ */
 static ssize_t platform_profile_choices_show(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+					     struct device_attribute *attr,
+					     char *buf)
 {
-	int len = 0;
-	int i;
+	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	int err;
 
+	set_bit(PLATFORM_PROFILE_LAST, aggregate);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
-			return -ENODEV;
-		for_each_set_bit(i, cur_profile->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]);
-		}
+		err = class_for_each_device(&platform_profile_class, NULL,
+					    aggregate, _aggregate_choices);
+		if (err)
+			return err;
 	}
-	len += sysfs_emit_at(buf, len, "\n");
 
-	return len;
+	/* no profile handler registered any more */
+	if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
+		return -EINVAL;
+
+	return _commmon_choices_show(aggregate, buf);
 }
 
 static ssize_t platform_profile_show(struct device *dev,
-- 
2.43.0

[PATCH v6 16/22] ACPI: platform_profile: Add concept of a "custom" profile

[Mario Limonciello]

When two profile handlers don't agree on the current profile it's ambiguous
what to show to the legacy sysfs interface.

Add a "custom" profile string that userspace will be able to distinguish
this situation when using the legacy sysfs interface.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c  | 1 +
 include/linux/platform_profile.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c4e2c75116988..089ac73b3ec97 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -20,6 +20,7 @@ static const char * const profile_names[] = {
 	[PLATFORM_PROFILE_BALANCED] = "balanced",
 	[PLATFORM_PROFILE_BALANCED_PERFORMANCE] = "balanced-performance",
 	[PLATFORM_PROFILE_PERFORMANCE] = "performance",
+	[PLATFORM_PROFILE_CUSTOM] = "custom",
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index a888fd085c513..0682bb4c57e5d 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -23,6 +23,7 @@ enum platform_profile_option {
 	PLATFORM_PROFILE_BALANCED,
 	PLATFORM_PROFILE_BALANCED_PERFORMANCE,
 	PLATFORM_PROFILE_PERFORMANCE,
+	PLATFORM_PROFILE_CUSTOM,
 	PLATFORM_PROFILE_LAST, /*must always be last */
 };
 
-- 
2.43.0

[PATCH v6 17/22] 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
return the custom profile.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Fix logic error with PLATFORM_PROFILE_CUSTOM
v5:
 * Notify class profile of change to legacy interface
 * Don't show warning when writing custom string, document in last patch
   instead.
 * Adjust mutex use
---
 drivers/acpi/platform_profile.c | 120 ++++++++++++++++++++++++++------
 1 file changed, 97 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 089ac73b3ec97..2676f4a13689e 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -71,6 +71,22 @@ static int _store_class_profile(struct device *dev, void *data)
 	return 0;
 }
 
+/**
+ * _notify_class_profile - Notify the class device of a profile change
+ * @dev: The class device
+ * @data: Unused
+ */
+static int _notify_class_profile(struct device *dev, void *data)
+{
+	struct platform_profile_handler *handler = dev_get_drvdata(dev);
+
+	lockdep_assert_held(&profile_lock);
+	sysfs_notify(&handler->class_dev->kobj, NULL, "profile");
+	kobject_uevent(&handler->class_dev->kobj, KOBJ_CHANGE);
+
+	return 0;
+}
+
 /**
  * get_class_profile - Show the current profile for a class device
  * @dev: The class device
@@ -257,51 +273,109 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 	return _commmon_choices_show(aggregate, buf);
 }
 
+/**
+ * _aggregate_profiles - Aggregate the profiles for legacy sysfs interface
+ * @dev: The device
+ * @data: The profile to return
+ * Return: 0 on success, -errno on failure
+ */
+static int _aggregate_profiles(struct device *dev, void *data)
+{
+	enum platform_profile_option *profile = data;
+	enum platform_profile_option val;
+	int err;
+
+	err = get_class_profile(dev, &val);
+	if (err)
+		return err;
+
+	if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
+		*profile = PLATFORM_PROFILE_CUSTOM;
+	else
+		*profile = val;
+
+	return 0;
+}
+
+
+/**
+ * _store_and_notify - Atomically store and notify a class from legacy sysfs interface
+ * @dev: The device
+ * @data: The profile to return
+ * Return: 0 on success, -errno on failure
+ */
+static int _store_and_notify(struct device *dev, void *data)
+{
+	enum platform_profile_option *profile = data;
+	int err;
+
+	err = _store_class_profile(dev, profile);
+	if (err)
+		return err;
+	return _notify_class_profile(dev, NULL);
+}
+
+/**
+ * platform_profile_show - Show the current profile for legacy sysfs interface
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to write to
+ * Return: The number of bytes written
+ */
 static ssize_t platform_profile_show(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+				     struct device_attribute *attr,
+				     char *buf)
 {
-	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
+	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
 	int err;
 
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
-			return -ENODEV;
-
-		err = cur_profile->profile_get(cur_profile, &profile);
+		err = class_for_each_device(&platform_profile_class, NULL,
+					    &profile, _aggregate_profiles);
 		if (err)
 			return err;
 	}
 
-	/* Check that profile is valid index */
-	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
-		return -EIO;
+	/* no profile handler registered any more */
+	if (profile == PLATFORM_PROFILE_LAST)
+		return -EINVAL;
 
 	return sysfs_emit(buf, "%s\n", profile_names[profile]);
 }
 
+/**
+ * platform_profile_store - Set the profile for legacy sysfs interface
+ * @dev: The device
+ * @attr: The attribute
+ * @buf: The buffer to read from
+ * @count: The number of bytes to read
+ * Return: The number of bytes read
+ */
 static ssize_t platform_profile_store(struct device *dev,
-			    struct device_attribute *attr,
-			    const char *buf, size_t count)
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
 {
-	int err, i;
+	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	int ret;
+	int i;
 
 	/* Scan for a matching profile */
 	i = sysfs_match_string(profile_names, buf);
-	if (i < 0)
+	if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
 		return -EINVAL;
-
+	set_bit(PLATFORM_PROFILE_LAST, choices);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
-			return -ENODEV;
-
-		/* Check that platform supports this profile choice */
-		if (!test_bit(i, cur_profile->choices))
+		ret = class_for_each_device(&platform_profile_class, NULL,
+					    choices, _aggregate_choices);
+		if (ret)
+			return ret;
+		if (!test_bit(i, choices))
 			return -EOPNOTSUPP;
 
-		err = cur_profile->profile_set(cur_profile, i);
-		if (err)
-			return err;
+		ret = class_for_each_device(&platform_profile_class, NULL, &i,
+					    _store_and_notify);
+		if (ret)
+			return ret;
 	}
 
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
-- 
2.43.0

[PATCH v6 18/22] 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: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * Handle cases of inconsistent profiles or all profile handlers
   supporting custom.
v5:
 * Adjust mutex use
---
 drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 2676f4a13689e..c574483be4fd1 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -423,28 +423,40 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
 
 int platform_profile_cycle(void)
 {
-	enum platform_profile_option profile;
-	enum platform_profile_option next;
+	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
+	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
+	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
 	int err;
 
 	if (!class_is_registered(&platform_profile_class))
 		return -ENODEV;
 
+	set_bit(PLATFORM_PROFILE_LAST, choices);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
-			return -ENODEV;
+		err = class_for_each_device(&platform_profile_class, NULL,
+					    &profile, _aggregate_profiles);
+		if (err)
+			return err;
 
-		err = cur_profile->profile_get(cur_profile, &profile);
+		if (profile == PLATFORM_PROFILE_CUSTOM ||
+		    profile == PLATFORM_PROFILE_LAST)
+			return -EINVAL;
+
+		err = class_for_each_device(&platform_profile_class, NULL,
+					    choices, _aggregate_choices);
 		if (err)
 			return err;
 
-		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
+		/* never iterate into a custom if all drivers supported it */
+		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
+
+		next = find_next_bit_wrap(choices,
+					  PLATFORM_PROFILE_LAST,
 					  profile + 1);
 
-		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
-			return -EINVAL;
+		err = class_for_each_device(&platform_profile_class, NULL, &next,
+					    _store_class_profile);
 
-		err = cur_profile->profile_set(cur_profile, next);
 		if (err)
 			return err;
 	}
-- 
2.43.0

[PATCH v6 19/22] ACPI: platform_profile: Notify class device from platform_profile_notify()

[Mario Limonciello]

When a driver has called platform_profile_notify() both the legacy sysfs
interface and the class device should be notified as userspace may listen
to either.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c574483be4fd1..7ad473982ab11 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -417,6 +417,7 @@ void platform_profile_notify(struct platform_profile_handler *pprof)
 {
 	if (!cur_profile)
 		return;
+	_notify_class_profile(NULL, pprof);
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 }
 EXPORT_SYMBOL_GPL(platform_profile_notify);
-- 
2.43.0

[PATCH v6 20/22] 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 the
drivers that were already changed remain changed and the legacy sysfs
handler will report 'custom'.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6:
 * rebase
v5:
 * reword commit message
---
 drivers/acpi/platform_profile.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 7ad473982ab11..0a8b86b44e161 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -10,7 +10,6 @@
 #include <linux/platform_profile.h>
 #include <linux/sysfs.h>
 
-static struct platform_profile_handler *cur_profile;
 static DEFINE_MUTEX(profile_lock);
 
 static const char * const profile_names[] = {
@@ -415,8 +414,7 @@ static const struct attribute_group platform_profile_group = {
 
 void platform_profile_notify(struct platform_profile_handler *pprof)
 {
-	if (!cur_profile)
-		return;
+	guard(mutex)(&profile_lock);
 	_notify_class_profile(NULL, pprof);
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 }
@@ -480,9 +478,6 @@ 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;
 
 	/* create class interface for individual handler */
 	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
@@ -498,8 +493,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	cur_profile = pprof;
-
 	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
 	if (err)
 		goto cleanup_cur;
@@ -507,7 +500,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	return 0;
 
 cleanup_cur:
-	cur_profile = NULL;
 	device_unregister(pprof->class_dev);
 
 cleanup_ida:
@@ -528,8 +520,6 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	cur_profile = NULL;
-
 	sysfs_update_group(acpi_kobj, &platform_profile_group);
 
 	return 0;
-- 
2.43.0

[PATCH v6 21/22] 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.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
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

[PATCH v6 22/22] Documentation: Add documentation about class interface for platform profiles

[Mario Limonciello]

The class interface allows changing multiple platform profiles on a system
to different values. The semantics of it are similar to the legacy
interface.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5:
 * Fix some typos
---
 .../ABI/testing/sysfs-platform_profile        |  5 ++++
 .../userspace-api/sysfs-platform_profile.rst  | 28 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
index baf1d125f9f83..125324ab53a96 100644
--- a/Documentation/ABI/testing/sysfs-platform_profile
+++ b/Documentation/ABI/testing/sysfs-platform_profile
@@ -33,3 +33,8 @@ Description:	Reading this file gives the current selected profile for this
 		source such as e.g. a hotkey triggered profile change handled
 		either directly by the embedded-controller or fully handled
 		inside the kernel.
+
+		This file may also emit the string 'custom' to indicate
+		that multiple platform profiles drivers are in use but
+		have different values.  This string can not be written to
+		this interface and is solely for informational purposes.
diff --git a/Documentation/userspace-api/sysfs-platform_profile.rst b/Documentation/userspace-api/sysfs-platform_profile.rst
index 4fccde2e45639..b746c30432753 100644
--- a/Documentation/userspace-api/sysfs-platform_profile.rst
+++ b/Documentation/userspace-api/sysfs-platform_profile.rst
@@ -40,3 +40,31 @@ added. Drivers which wish to introduce new profile names must:
  1. Explain why the existing profile names cannot be used.
  2. Add the new profile name, along with a clear description of the
     expected behaviour, to the sysfs-platform_profile ABI documentation.
+
+Multiple driver support
+=======================
+When multiple drivers on a system advertise a platform profile handler, the
+platform profile handler core will only advertise the profiles that are
+common between all drivers to the ``/sys/firmware/acpi`` interfaces.
+
+This is to ensure there is no ambiguity on what the profile names mean when
+all handlers don't support a profile.
+
+Individual drivers will register a 'platform_profile' class device that has
+similar semantics as the ``/sys/firmware/acpi/platform_profile`` interface.
+
+To discover available profiles from the class interface the user can read the
+``choices`` attribute.
+
+If a user wants to select a profile for a specific driver, they can do so
+by writing to the ``profile`` attribute of the driver's class device.
+
+This will allow users to set different profiles for different drivers on the
+same system. If the selected profile by individual drivers differs the
+platform profile handler core will display the profile 'custom' to indicate
+that the profiles are not the same.
+
+While the ``platform_profile`` attribute has the value ``custom``, writing a
+common profile from ``platform_profile_choices`` to the platform_profile
+attribute of the platform profile handler core will set the profile for all
+drivers.
-- 
2.43.0

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

[Rafael J. Wysocki]

On Sat, Nov 9, 2024 at 5:42 AM Mario Limonciello
<mario.limonciello@amd.com> 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 when the legacy /sys/firmware/acpi/platform_profile interface
> is used.
>
> This series also adds a new concept of a "custom" profile.  This allows
> userspace to discover that there are multiple driver handlers that are
> configured differently.
>
> This series also allows dropping all of the PMF quirks from amd-pmf.
>
> ---
> v6:
>  * Add patch dev patch but don't make mandatory

Probably a typo?

Which patch is it, BTW?

In any case, if the merge window for 6.13 starts on the upcoming
weekend, which is likely to happen AFAICS, I'll defer applying this
series until 6.13-rc1 is out.

It's larger and it's been changing too often recently for me to catch
up and I'll be much more comfortable if it spends some time in
linux-next before going into the mainline (and not during a merge
window for that matter).

>  * See other patches changelogs for individualized changes
>
> Mario Limonciello (22):
>   ACPI: platform-profile: Add a name member to handlers
>   platform/x86/dell: dell-pc: Create platform device
>   ACPI: platform_profile: Add device pointer into platform profile
>     handler
>   ACPI: platform_profile: Add platform handler argument to
>     platform_profile_remove()
>   ACPI: platform_profile: Pass the profile handler into
>     platform_profile_notify()
>   ACPI: platform_profile: Move sanity check out of the mutex
>   ACPI: platform_profile: Move matching string for new profile out of
>     mutex
>   ACPI: platform_profile: Use guard(mutex) for register/unregister
>   ACPI: platform_profile: Use `scoped_cond_guard`
>   ACPI: platform_profile: Create class for ACPI platform profile
>   ACPI: platform_profile: Add name attribute to class interface
>   ACPI: platform_profile: Add choices attribute for class interface
>   ACPI: platform_profile: Add profile attribute for class interface
>   ACPI: platform_profile: Notify change events on register and
>     unregister
>   ACPI: platform_profile: Only show profiles common for all handlers
>   ACPI: platform_profile: Add concept of a "custom" profile
>   ACPI: platform_profile: Make sure all profile handlers agree on
>     profile
>   ACPI: platform_profile: Check all profile handler to calculate next
>   ACPI: platform_profile: Notify class device from
>     platform_profile_notify()
>   ACPI: platform_profile: Allow multiple handlers
>   platform/x86/amd: pmf: Drop all quirks
>   Documentation: Add documentation about class interface for platform
>     profiles
>
>  .../ABI/testing/sysfs-platform_profile        |   5 +
>  .../userspace-api/sysfs-platform_profile.rst  |  28 +
>  drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
>  .../surface/surface_platform_profile.c        |   8 +-
>  drivers/platform/x86/acer-wmi.c               |  12 +-
>  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            |   4 +-
>  drivers/platform/x86/asus-wmi.c               |  10 +-
>  drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>  drivers/platform/x86/dell/dell-pc.c           |  36 +-
>  drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>  drivers/platform/x86/ideapad-laptop.c         |   6 +-
>  .../platform/x86/inspur_platform_profile.c    |   7 +-
>  drivers/platform/x86/thinkpad_acpi.c          |  16 +-
>  include/linux/platform_profile.h              |   9 +-
>  18 files changed, 553 insertions(+), 213 deletions(-)
>  delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>
>
> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
> --
> 2.43.0
>

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

[Mario Limonciello]

On 11/12/2024 14:16, Rafael J. Wysocki wrote:
> On Sat, Nov 9, 2024 at 5:42 AM Mario Limonciello
> <mario.limonciello@amd.com> 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 when the legacy /sys/firmware/acpi/platform_profile interface
>> is used.
>>
>> This series also adds a new concept of a "custom" profile.  This allows
>> userspace to discover that there are multiple driver handlers that are
>> configured differently.
>>
>> This series also allows dropping all of the PMF quirks from amd-pmf.
>>
>> ---
>> v6:
>>   * Add patch dev patch but don't make mandatory
> 
> Probably a typo?

Ah whoops, yes.

> 
> Which patch is it, BTW?

Patch 3.

> 
> In any case, if the merge window for 6.13 starts on the upcoming
> weekend, which is likely to happen AFAICS, I'll defer applying this
> series until 6.13-rc1 is out.
> 
> It's larger and it's been changing too often recently for me to catch
> up and I'll be much more comfortable if it spends some time in
> linux-next before going into the mainline (and not during a merge
> window for that matter).
> 

I'm thankful; Armin ended up having a lot of very valuable feedback.

Yeah, it makes sense to defer to next cycle.

Would you prefer me to rebase and resend as v7 after the merge window or 
will you just add it to a TODO?

>>   * See other patches changelogs for individualized changes
>>
>> Mario Limonciello (22):
>>    ACPI: platform-profile: Add a name member to handlers
>>    platform/x86/dell: dell-pc: Create platform device
>>    ACPI: platform_profile: Add device pointer into platform profile
>>      handler
>>    ACPI: platform_profile: Add platform handler argument to
>>      platform_profile_remove()
>>    ACPI: platform_profile: Pass the profile handler into
>>      platform_profile_notify()
>>    ACPI: platform_profile: Move sanity check out of the mutex
>>    ACPI: platform_profile: Move matching string for new profile out of
>>      mutex
>>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>>    ACPI: platform_profile: Use `scoped_cond_guard`
>>    ACPI: platform_profile: Create class for ACPI platform profile
>>    ACPI: platform_profile: Add name attribute to class interface
>>    ACPI: platform_profile: Add choices attribute for class interface
>>    ACPI: platform_profile: Add profile attribute for class interface
>>    ACPI: platform_profile: Notify change events on register and
>>      unregister
>>    ACPI: platform_profile: Only show profiles common for all handlers
>>    ACPI: platform_profile: Add concept of a "custom" profile
>>    ACPI: platform_profile: Make sure all profile handlers agree on
>>      profile
>>    ACPI: platform_profile: Check all profile handler to calculate next
>>    ACPI: platform_profile: Notify class device from
>>      platform_profile_notify()
>>    ACPI: platform_profile: Allow multiple handlers
>>    platform/x86/amd: pmf: Drop all quirks
>>    Documentation: Add documentation about class interface for platform
>>      profiles
>>
>>   .../ABI/testing/sysfs-platform_profile        |   5 +
>>   .../userspace-api/sysfs-platform_profile.rst  |  28 +
>>   drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
>>   .../surface/surface_platform_profile.c        |   8 +-
>>   drivers/platform/x86/acer-wmi.c               |  12 +-
>>   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            |   4 +-
>>   drivers/platform/x86/asus-wmi.c               |  10 +-
>>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>>   drivers/platform/x86/dell/dell-pc.c           |  36 +-
>>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>>   drivers/platform/x86/ideapad-laptop.c         |   6 +-
>>   .../platform/x86/inspur_platform_profile.c    |   7 +-
>>   drivers/platform/x86/thinkpad_acpi.c          |  16 +-
>>   include/linux/platform_profile.h              |   9 +-
>>   18 files changed, 553 insertions(+), 213 deletions(-)
>>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>>
>>
>> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
>> --
>> 2.43.0
>>

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

[Rafael J. Wysocki]

On Tue, Nov 12, 2024 at 9:20 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 11/12/2024 14:16, Rafael J. Wysocki wrote:
> > On Sat, Nov 9, 2024 at 5:42 AM Mario Limonciello
> > <mario.limonciello@amd.com> 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 when the legacy /sys/firmware/acpi/platform_profile interface
> >> is used.
> >>
> >> This series also adds a new concept of a "custom" profile.  This allows
> >> userspace to discover that there are multiple driver handlers that are
> >> configured differently.
> >>
> >> This series also allows dropping all of the PMF quirks from amd-pmf.
> >>
> >> ---
> >> v6:
> >>   * Add patch dev patch but don't make mandatory
> >
> > Probably a typo?
>
> Ah whoops, yes.
>
> >
> > Which patch is it, BTW?
>
> Patch 3.
>
> >
> > In any case, if the merge window for 6.13 starts on the upcoming
> > weekend, which is likely to happen AFAICS, I'll defer applying this
> > series until 6.13-rc1 is out.
> >
> > It's larger and it's been changing too often recently for me to catch
> > up and I'll be much more comfortable if it spends some time in
> > linux-next before going into the mainline (and not during a merge
> > window for that matter).
> >
>
> I'm thankful; Armin ended up having a lot of very valuable feedback.
>
> Yeah, it makes sense to defer to next cycle.
>
> Would you prefer me to rebase and resend as v7 after the merge window or
> will you just add it to a TODO?

If rebasing is needed, it will be welcome.  Also if you need/want to
make any changes in the meantime, please respin.  Otherwise I can just
pick up the current series.

> >>   * See other patches changelogs for individualized changes
> >>
> >> Mario Limonciello (22):
> >>    ACPI: platform-profile: Add a name member to handlers
> >>    platform/x86/dell: dell-pc: Create platform device
> >>    ACPI: platform_profile: Add device pointer into platform profile
> >>      handler
> >>    ACPI: platform_profile: Add platform handler argument to
> >>      platform_profile_remove()
> >>    ACPI: platform_profile: Pass the profile handler into
> >>      platform_profile_notify()
> >>    ACPI: platform_profile: Move sanity check out of the mutex
> >>    ACPI: platform_profile: Move matching string for new profile out of
> >>      mutex
> >>    ACPI: platform_profile: Use guard(mutex) for register/unregister
> >>    ACPI: platform_profile: Use `scoped_cond_guard`
> >>    ACPI: platform_profile: Create class for ACPI platform profile
> >>    ACPI: platform_profile: Add name attribute to class interface
> >>    ACPI: platform_profile: Add choices attribute for class interface
> >>    ACPI: platform_profile: Add profile attribute for class interface
> >>    ACPI: platform_profile: Notify change events on register and
> >>      unregister
> >>    ACPI: platform_profile: Only show profiles common for all handlers
> >>    ACPI: platform_profile: Add concept of a "custom" profile
> >>    ACPI: platform_profile: Make sure all profile handlers agree on
> >>      profile
> >>    ACPI: platform_profile: Check all profile handler to calculate next
> >>    ACPI: platform_profile: Notify class device from
> >>      platform_profile_notify()
> >>    ACPI: platform_profile: Allow multiple handlers
> >>    platform/x86/amd: pmf: Drop all quirks
> >>    Documentation: Add documentation about class interface for platform
> >>      profiles
> >>
> >>   .../ABI/testing/sysfs-platform_profile        |   5 +
> >>   .../userspace-api/sysfs-platform_profile.rst  |  28 +
> >>   drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
> >>   .../surface/surface_platform_profile.c        |   8 +-
> >>   drivers/platform/x86/acer-wmi.c               |  12 +-
> >>   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            |   4 +-
> >>   drivers/platform/x86/asus-wmi.c               |  10 +-
> >>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
> >>   drivers/platform/x86/dell/dell-pc.c           |  36 +-
> >>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
> >>   drivers/platform/x86/ideapad-laptop.c         |   6 +-
> >>   .../platform/x86/inspur_platform_profile.c    |   7 +-
> >>   drivers/platform/x86/thinkpad_acpi.c          |  16 +-
> >>   include/linux/platform_profile.h              |   9 +-
> >>   18 files changed, 553 insertions(+), 213 deletions(-)
> >>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
> >>
> >>
> >> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
> >> --
> >> 2.43.0
> >>
>

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

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb 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 when the legacy /sys/firmware/acpi/platform_profile interface
> is used.
>
> This series also adds a new concept of a "custom" profile.  This allows
> userspace to discover that there are multiple driver handlers that are
> configured differently.
>
> This series also allows dropping all of the PMF quirks from amd-pmf.

Sorry for taking a bit long to respond, i am currently quite busy. I will try to review this series
in the coming days.

Thanks,
Armin Wolf

> ---
> v6:
>   * Add patch dev patch but don't make mandatory
>   * See other patches changelogs for individualized changes
>
> Mario Limonciello (22):
>    ACPI: platform-profile: Add a name member to handlers
>    platform/x86/dell: dell-pc: Create platform device
>    ACPI: platform_profile: Add device pointer into platform profile
>      handler
>    ACPI: platform_profile: Add platform handler argument to
>      platform_profile_remove()
>    ACPI: platform_profile: Pass the profile handler into
>      platform_profile_notify()
>    ACPI: platform_profile: Move sanity check out of the mutex
>    ACPI: platform_profile: Move matching string for new profile out of
>      mutex
>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>    ACPI: platform_profile: Use `scoped_cond_guard`
>    ACPI: platform_profile: Create class for ACPI platform profile
>    ACPI: platform_profile: Add name attribute to class interface
>    ACPI: platform_profile: Add choices attribute for class interface
>    ACPI: platform_profile: Add profile attribute for class interface
>    ACPI: platform_profile: Notify change events on register and
>      unregister
>    ACPI: platform_profile: Only show profiles common for all handlers
>    ACPI: platform_profile: Add concept of a "custom" profile
>    ACPI: platform_profile: Make sure all profile handlers agree on
>      profile
>    ACPI: platform_profile: Check all profile handler to calculate next
>    ACPI: platform_profile: Notify class device from
>      platform_profile_notify()
>    ACPI: platform_profile: Allow multiple handlers
>    platform/x86/amd: pmf: Drop all quirks
>    Documentation: Add documentation about class interface for platform
>      profiles
>
>   .../ABI/testing/sysfs-platform_profile        |   5 +
>   .../userspace-api/sysfs-platform_profile.rst  |  28 +
>   drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
>   .../surface/surface_platform_profile.c        |   8 +-
>   drivers/platform/x86/acer-wmi.c               |  12 +-
>   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            |   4 +-
>   drivers/platform/x86/asus-wmi.c               |  10 +-
>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>   drivers/platform/x86/dell/dell-pc.c           |  36 +-
>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>   drivers/platform/x86/ideapad-laptop.c         |   6 +-
>   .../platform/x86/inspur_platform_profile.c    |   7 +-
>   drivers/platform/x86/thinkpad_acpi.c          |  16 +-
>   include/linux/platform_profile.h              |   9 +-
>   18 files changed, 553 insertions(+), 213 deletions(-)
>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>
>
> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2

Re: [PATCH v6 03/22] ACPI: platform_profile: Add device pointer into platform profile handler

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> In order to let platform profile handlers manage platform profile
> for their driver the core code will need a pointer to the device.
>
> Add this to the structure and use it in the trivial driver cases.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6:
>   * Don't require to be set
> v4:
>   * add alienware-wmi too
> ---
>   drivers/platform/surface/surface_platform_profile.c | 1 +
>   drivers/platform/x86/acer-wmi.c                     | 5 +++--
>   drivers/platform/x86/amd/pmf/sps.c                  | 1 +
>   drivers/platform/x86/asus-wmi.c                     | 1 +
>   drivers/platform/x86/dell/alienware-wmi.c           | 5 +++--
>   drivers/platform/x86/dell/dell-pc.c                 | 1 +
>   drivers/platform/x86/hp/hp-wmi.c                    | 5 +++--
>   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 +
>   11 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index 61aa488a80eb5..5f45f8e8cd69b 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -212,6 +212,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
>   	tpd->sdev = sdev;
>
>   	tpd->handler.name = "Surface Platform Profile";
> +	tpd->handler.dev = &sdev->dev;
>   	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 53fbc9b4d3df7..aca4a5746bee1 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1873,12 +1873,13 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
>   	return 0;
>   }
>
> -static int acer_platform_profile_setup(void)
> +static int acer_platform_profile_setup(struct platform_device *device)
>   {
>   	if (quirks->predator_v4) {
>   		int err;
>
>   		platform_profile_handler.name = "acer-wmi";
> +		platform_profile_handler.dev = &device->dev;
>   		platform_profile_handler.profile_get =
>   			acer_predator_v4_platform_profile_get;
>   		platform_profile_handler.profile_set =
> @@ -2531,7 +2532,7 @@ static int acer_platform_probe(struct platform_device *device)
>   		goto error_rfkill;
>
>   	if (has_cap(ACER_CAP_PLATFORM_PROFILE)) {
> -		err = acer_platform_profile_setup();
> +		err = acer_platform_profile_setup(device);
>   		if (err)
>   			goto error_platform_profile;
>   	}
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index e2d0cc92c4396..1b94af7c0e0c4 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -406,6 +406,7 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>   	}
>
>   	dev->pprof.name = "amd-pmf";
> +	dev->pprof.dev = dev->dev;
>   	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 c7c104c65a85a..78621b2af0ddb 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3911,6 +3911,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.dev = dev;
>   	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/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index ac0038afd98fa..c03b1aa7dfb5f 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -1017,7 +1017,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
>   	return wmax_thermal_control(supported_thermal_profiles[profile]);
>   }
>
> -static int create_thermal_profile(void)
> +static int create_thermal_profile(struct platform_device *platform_device)
>   {
>   	u32 out_data;
>   	enum wmax_thermal_mode mode;
> @@ -1057,6 +1057,7 @@ static int create_thermal_profile(void)
>   	pp_handler.profile_get = thermal_profile_get;
>   	pp_handler.profile_set = thermal_profile_set;
>   	pp_handler.name = "alienware-wmi";
> +	pp_handler.dev = &platform_device->dev;
>
>   	return platform_profile_register(&pp_handler);
>   }
> @@ -1125,7 +1126,7 @@ static int __init alienware_wmi_init(void)
>   	}
>
>   	if (quirks->thermal) {
> -		ret = create_thermal_profile();
> +		ret = create_thermal_profile(platform_device);
>   		if (ret)
>   			goto fail_prep_thermal_profile;
>   	}
> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
> index 805497e44b3a5..e5724e76a2082 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -257,6 +257,7 @@ static int thermal_init(void)
>   		goto cleanup_platform_device;
>   	}
>   	thermal_handler->name = "dell-pc";
> +	thermal_handler->dev = &platform_device->dev;
>   	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 26cac73caf2b9..ffb09799142bc 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -1565,7 +1565,7 @@ static inline void omen_unregister_powersource_event_handler(void)
>   	unregister_acpi_notifier(&platform_power_source_nb);
>   }
>
> -static int thermal_profile_setup(void)
> +static int thermal_profile_setup(struct platform_device *device)
>   {
>   	int err, tp;
>
> @@ -1625,6 +1625,7 @@ static int thermal_profile_setup(void)
>   	}
>
>   	platform_profile_handler.name = "hp-wmi";
> +	platform_profile_handler.dev = &device->dev;
>   	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
>   	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
>
> @@ -1664,7 +1665,7 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>   	if (err < 0)
>   		return err;
>
> -	thermal_profile_setup();
> +	thermal_profile_setup(device);
>
>   	return 0;
>   }
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 1f94c14c3b832..24367c3590c99 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1103,6 +1103,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
>   	mutex_init(&priv->dytc->mutex);
>
>   	priv->dytc->pprof.name = "ideapad-laptop";
> +	priv->dytc->pprof.dev = &priv->platform_device->dev;
>   	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 03da2c8cf6789..5a53949bbbf5f 100644
> --- a/drivers/platform/x86/inspur_platform_profile.c
> +++ b/drivers/platform/x86/inspur_platform_profile.c
> @@ -178,6 +178,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
>   	dev_set_drvdata(&wdev->dev, priv);
>
>   	priv->handler.name = "inspur-wmi";
> +	priv->handler.dev = &wdev->dev;
>   	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 c8c316b8507a5..222fba97d79a7 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10616,6 +10616,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>   	dbg_printk(TPACPI_DBG_INIT,
>   			"DYTC version %d: thermal mode available\n", dytc_version);
>
> +	dytc_profile.dev = &tpacpi_pdev->dev;
>   	/* Create platform_profile structure and register */
>   	err = platform_profile_register(&dytc_profile);
>   	/*
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 6fa988e417428..daec6b9bad81f 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -28,6 +28,7 @@ enum platform_profile_option {
>
>   struct platform_profile_handler {
>   	const char *name;
> +	struct device *dev;
>   	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>   	int (*profile_get)(struct platform_profile_handler *pprof,
>   				enum platform_profile_option *profile);

Re: [PATCH v6 05/22] ACPI: platform_profile: Pass the profile handler into platform_profile_notify()

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> The profile handler will be used to notify the appropriate class
> devices.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c       |  2 +-
>   drivers/platform/x86/acer-wmi.c       |  2 +-
>   drivers/platform/x86/asus-wmi.c       |  4 ++--
>   drivers/platform/x86/ideapad-laptop.c |  2 +-
>   drivers/platform/x86/thinkpad_acpi.c  | 14 +++++++-------
>   include/linux/platform_profile.h      |  2 +-
>   6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index c24744da20916..927a2f7456c9a 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -128,7 +128,7 @@ static const struct attribute_group platform_profile_group = {
>   	.attrs = platform_profile_attrs
>   };
>
> -void platform_profile_notify(void)
> +void platform_profile_notify(struct platform_profile_handler *pprof)
>   {
>   	if (!cur_profile)
>   		return;
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index b12965d9fcdb7..0018818258070 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1988,7 +1988,7 @@ static int acer_thermal_profile_change(void)
>   		if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
>   			last_non_turbo_profile = tp;
>
> -		platform_profile_notify();
> +		platform_profile_notify(&platform_profile_handler);
>   	}
>
>   	return 0;
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 0750e2fe65325..2a8d789ee05cf 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3754,7 +3754,7 @@ static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>   	 * Ensure that platform_profile updates userspace with the change to ensure
>   	 * that platform_profile and throttle_thermal_policy_mode are in sync.
>   	 */
> -	platform_profile_notify();
> +	platform_profile_notify(&asus->platform_profile_handler);
>
>   	return 0;
>   }
> @@ -3793,7 +3793,7 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>   	 * Ensure that platform_profile updates userspace with the change to ensure
>   	 * that platform_profile and throttle_thermal_policy_mode are in sync.
>   	 */
> -	platform_profile_notify();
> +	platform_profile_notify(&asus->platform_profile_handler);
>
>   	return count;
>   }
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 80797c6ae8b0b..79c65c24b433a 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1041,7 +1041,7 @@ static void dytc_profile_refresh(struct ideapad_private *priv)
>
>   	if (profile != priv->dytc->current_profile) {
>   		priv->dytc->current_profile = profile;
> -		platform_profile_notify();
> +		platform_profile_notify(&priv->dytc->pprof);
>   	}
>   }
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 13798c6d5fcf3..8539cac042be8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10516,6 +10516,12 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>   	return err;
>   }
>
> +static struct platform_profile_handler dytc_profile = {
> +	.name = "thinkpad-acpi",
> +	.profile_get = dytc_profile_get,
> +	.profile_set = dytc_profile_set,
> +};
> +
>   static void dytc_profile_refresh(void)
>   {
>   	enum platform_profile_option profile;
> @@ -10544,16 +10550,10 @@ static void dytc_profile_refresh(void)
>   	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
>   	if (!err && profile != dytc_current_profile) {
>   		dytc_current_profile = profile;
> -		platform_profile_notify();
> +		platform_profile_notify(&dytc_profile);
>   	}
>   }
>
> -static struct platform_profile_handler dytc_profile = {
> -	.name = "thinkpad-acpi",
> -	.profile_get = dytc_profile_get,
> -	.profile_set = dytc_profile_set,
> -};
> -
>   static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>   {
>   	int err, output;
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index bcaf3aa39160f..8ec0b8da56db5 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -39,6 +39,6 @@ struct platform_profile_handler {
>   int platform_profile_register(struct platform_profile_handler *pprof);
>   int platform_profile_remove(struct platform_profile_handler *pprof);
>   int platform_profile_cycle(void);
> -void platform_profile_notify(void);
> +void platform_profile_notify(struct platform_profile_handler *pprof);
>
>   #endif  /*_PLATFORM_PROFILE_H_*/

Re: [PATCH v6 10/22] ACPI: platform_profile: Create class for ACPI platform profile

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> When registering a platform profile handler create a class device
> that will allow changing a single platform profile handler.
>
> The class and sysfs group are no longer needed when the platform profile
> core is a module and unloaded, so remove them at that time as well.
>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6:
>   * Catch failures in ida_alloc
>   * Use 4th argument of device_create instead of dev_set_drvdata()
>   * Squash unregister patch
>   * Add module init callback
>   * Move class creation to module init
>   * Update visibility based on group presence
>   * Add back parent device
> v5:
>   * Use ida instead of idr
>   * Use device_unregister instead of device_destroy()
>   * MKDEV (0, 0)
> ---
>   drivers/acpi/platform_profile.c  | 88 ++++++++++++++++++++++++++++++--
>   include/linux/platform_profile.h |  2 +
>   2 files changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 32affb75e782d..ef6af2c655524 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -5,6 +5,7 @@
>   #include <linux/acpi.h>
>   #include <linux/bits.h>
>   #include <linux/init.h>
> +#include <linux/kdev_t.h>
>   #include <linux/mutex.h>
>   #include <linux/platform_profile.h>
>   #include <linux/sysfs.h>
> @@ -22,6 +23,12 @@ static const char * const profile_names[] = {
>   };
>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> +static DEFINE_IDA(platform_profile_ida);
> +
> +static const struct class platform_profile_class = {
> +	.name = "platform-profile",
> +};
> +
>   static ssize_t platform_profile_choices_show(struct device *dev,
>   					struct device_attribute *attr,
>   					char *buf)
> @@ -105,8 +112,25 @@ static struct attribute *platform_profile_attrs[] = {
>   	NULL
>   };
>
> +static int profile_class_registered(struct device *dev, const void *data)
> +{
> +	return 1;
> +}
> +
> +static umode_t profile_class_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +{
> +	if (!class_find_device(&platform_profile_class, NULL, NULL, profile_class_registered))
> +		return 0;
> +	if (attr == &dev_attr_platform_profile_choices.attr)
> +		return 0444;
> +	if (attr == &dev_attr_platform_profile.attr)
> +		return 0644;
> +	return 0;
> +}
> +
>   static const struct attribute_group platform_profile_group = {
> -	.attrs = platform_profile_attrs
> +	.attrs = platform_profile_attrs,
> +	.is_visible = profile_class_is_visible,
>   };
>
>   void platform_profile_notify(struct platform_profile_handler *pprof)
> @@ -123,6 +147,9 @@ int platform_profile_cycle(void)
>   	enum platform_profile_option next;
>   	int err;
>
> +	if (!class_is_registered(&platform_profile_class))
> +		return -ENODEV;

This check is pointless since the platform profile class will always be registered during module initialization.
Please remove it.

> +
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>   		if (!cur_profile)
>   			return -ENODEV;
> @@ -164,25 +191,76 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   	if (cur_profile)
>   		return -EEXIST;
>
> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> -	if (err)
> -		return err;
> +	/* create class interface for individual handler */
> +	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> +	if (pprof->minor < 0)
> +		return pprof->minor;
> +	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
> +					 MKDEV(0, 0), pprof, "platform-profile-%d",
> +					 pprof->minor);
> +	if (IS_ERR(pprof->class_dev)) {
> +		err = PTR_ERR(pprof->class_dev);
> +		goto cleanup_ida;
> +	}
>
>   	cur_profile = pprof;
> +
> +	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
> +	if (err)
> +		goto cleanup_cur;
> +
>   	return 0;
> +
> +cleanup_cur:
> +	cur_profile = NULL;
> +	device_unregister(pprof->class_dev);
> +
> +cleanup_ida:
> +	ida_free(&platform_profile_ida, pprof->minor);
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_register);
>
>   int platform_profile_remove(struct platform_profile_handler *pprof)
>   {
> +	int id;
>   	guard(mutex)(&profile_lock);
>
> -	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +	id = pprof->minor;
> +	device_unregister(pprof->class_dev);
> +	ida_free(&platform_profile_ida, id);
> +
>   	cur_profile = NULL;
> +
> +	sysfs_update_group(acpi_kobj, &platform_profile_group);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_remove);
>
> +static int __init platform_profile_init(void)
> +{
> +	int err;
> +
> +	err = class_register(&platform_profile_class);
> +	if (err)
> +		return err;
> +
> +	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +	if (err)
> +		class_unregister(&platform_profile_class);
> +
> +	return err;
> +}

Please use a blank line after function/struct/union/enum declarations.

Apart from those minor issues the patch looks quite nice, so with those issues being fixed:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> +static void __exit platform_profile_exit(void)
> +{
> +	class_unregister(&platform_profile_class);
> +	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +}
> +module_init(platform_profile_init);
> +module_exit(platform_profile_exit);
> +
>   MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
>   MODULE_DESCRIPTION("ACPI platform profile sysfs interface");
>   MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 8ec0b8da56db5..a888fd085c513 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -29,6 +29,8 @@ enum platform_profile_option {
>   struct platform_profile_handler {
>   	const char *name;
>   	struct device *dev;
> +	struct device *class_dev;
> +	int minor;
>   	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>   	int (*profile_get)(struct platform_profile_handler *pprof,
>   				enum platform_profile_option *profile);

Re: [PATCH v6 14/22] ACPI: platform_profile: Notify change events on register and unregister

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb 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.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index c5487d3928c1b..c4d451782ff18 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -380,6 +380,8 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   		goto cleanup_ida;
>   	}
>
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
>   	cur_profile = pprof;
>
>   	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
> @@ -408,6 +410,8 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>   	device_unregister(pprof->class_dev);
>   	ida_free(&platform_profile_ida, id);
>
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
>   	cur_profile = NULL;
>
>   	sysfs_update_group(acpi_kobj, &platform_profile_group);

Re: [PATCH v6 19/22] ACPI: platform_profile: Notify class device from platform_profile_notify()

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> When a driver has called platform_profile_notify() both the legacy sysfs
> interface and the class device should be notified as userspace may listen
> to either.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index c574483be4fd1..7ad473982ab11 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -417,6 +417,7 @@ void platform_profile_notify(struct platform_profile_handler *pprof)
>   {
>   	if (!cur_profile)
>   		return;
> +	_notify_class_profile(NULL, pprof);

You will need to supply the first argument ("dev") here or _notify_class_profile() will crash.

Thanks,
Armin Wolf

>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_notify);

Re: [PATCH v6 20/22] ACPI: platform_profile: Allow multiple handlers

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb 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 the
> drivers that were already changed remain changed and the legacy sysfs
> handler will report 'custom'.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6:
>   * rebase
> v5:
>   * reword commit message
> ---
>   drivers/acpi/platform_profile.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 7ad473982ab11..0a8b86b44e161 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -10,7 +10,6 @@
>   #include <linux/platform_profile.h>
>   #include <linux/sysfs.h>
>
> -static struct platform_profile_handler *cur_profile;
>   static DEFINE_MUTEX(profile_lock);
>
>   static const char * const profile_names[] = {
> @@ -415,8 +414,7 @@ static const struct attribute_group platform_profile_group = {
>
>   void platform_profile_notify(struct platform_profile_handler *pprof)
>   {
> -	if (!cur_profile)
> -		return;
> +	guard(mutex)(&profile_lock);
>   	_notify_class_profile(NULL, pprof);
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>   }
> @@ -480,9 +478,6 @@ 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;
>
>   	/* create class interface for individual handler */
>   	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> @@ -498,8 +493,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>
> -	cur_profile = pprof;
> -
>   	err = sysfs_update_group(acpi_kobj, &platform_profile_group);
>   	if (err)
>   		goto cleanup_cur;
> @@ -507,7 +500,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   	return 0;
>
>   cleanup_cur:
> -	cur_profile = NULL;
>   	device_unregister(pprof->class_dev);
>
>   cleanup_ida:
> @@ -528,8 +520,6 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>
> -	cur_profile = NULL;
> -
>   	sysfs_update_group(acpi_kobj, &platform_profile_group);
>
>   	return 0;

Re: [PATCH v6 22/22] Documentation: Add documentation about class interface for platform profiles

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> The class interface allows changing multiple platform profiles on a system
> to different values. The semantics of it are similar to the legacy
> interface.
>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v5:
>   * Fix some typos
> ---
>   .../ABI/testing/sysfs-platform_profile        |  5 ++++
>   .../userspace-api/sysfs-platform_profile.rst  | 28 +++++++++++++++++++
>   2 files changed, 33 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
> index baf1d125f9f83..125324ab53a96 100644
> --- a/Documentation/ABI/testing/sysfs-platform_profile
> +++ b/Documentation/ABI/testing/sysfs-platform_profile
> @@ -33,3 +33,8 @@ Description:	Reading this file gives the current selected profile for this
>   		source such as e.g. a hotkey triggered profile change handled
>   		either directly by the embedded-controller or fully handled
>   		inside the kernel.
> +
> +		This file may also emit the string 'custom' to indicate
> +		that multiple platform profiles drivers are in use but
> +		have different values.  This string can not be written to
> +		this interface and is solely for informational purposes.
> diff --git a/Documentation/userspace-api/sysfs-platform_profile.rst b/Documentation/userspace-api/sysfs-platform_profile.rst
> index 4fccde2e45639..b746c30432753 100644
> --- a/Documentation/userspace-api/sysfs-platform_profile.rst
> +++ b/Documentation/userspace-api/sysfs-platform_profile.rst
> @@ -40,3 +40,31 @@ added. Drivers which wish to introduce new profile names must:
>    1. Explain why the existing profile names cannot be used.
>    2. Add the new profile name, along with a clear description of the
>       expected behaviour, to the sysfs-platform_profile ABI documentation.
> +
> +Multiple driver support
> +=======================
> +When multiple drivers on a system advertise a platform profile handler, the
> +platform profile handler core will only advertise the profiles that are
> +common between all drivers to the ``/sys/firmware/acpi`` interfaces.
> +
> +This is to ensure there is no ambiguity on what the profile names mean when
> +all handlers don't support a profile.
> +
> +Individual drivers will register a 'platform_profile' class device that has
> +similar semantics as the ``/sys/firmware/acpi/platform_profile`` interface.
> +

Please add a short description of the 'name' attribute of the class device.

With that being addressed:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> +To discover available profiles from the class interface the user can read the
> +``choices`` attribute.
> +
> +If a user wants to select a profile for a specific driver, they can do so
> +by writing to the ``profile`` attribute of the driver's class device.
> +
> +This will allow users to set different profiles for different drivers on the
> +same system. If the selected profile by individual drivers differs the
> +platform profile handler core will display the profile 'custom' to indicate
> +that the profiles are not the same.
> +
> +While the ``platform_profile`` attribute has the value ``custom``, writing a
> +common profile from ``platform_profile_choices`` to the platform_profile
> +attribute of the platform profile handler core will set the profile for all
> +drivers.

Re: [PATCH v6 09/22] ACPI: platform_profile: Use `scoped_cond_guard`

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> Migrate away from using an interruptible mutex to scoped_cond_guard
> in all functions. While changing, move the sysfs notification
> used in platform_profile_store() outside of mutex scope.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c | 111 +++++++++++++-------------------
>   1 file changed, 43 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 9729543df6333..32affb75e782d 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -27,25 +27,20 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>   					char *buf)
>   {
>   	int len = 0;
> -	int err, i;
> -
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> -
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> -
> -	for_each_set_bit(i, cur_profile->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]);
> +	int i;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		if (!cur_profile)
> +			return -ENODEV;
> +		for_each_set_bit(i, cur_profile->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;
>   }
>
> @@ -56,20 +51,15 @@ 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;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		if (!cur_profile)
> +			return -ENODEV;
>
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> +		err = cur_profile->profile_get(cur_profile, &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;
> @@ -88,28 +78,21 @@ static ssize_t platform_profile_store(struct device *dev,
>   	if (i < 0)
>   		return -EINVAL;
>
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		if (!cur_profile)
> +			return -ENODEV;
>
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> +		/* Check that platform supports this profile choice */
> +		if (!test_bit(i, cur_profile->choices))
> +			return -EOPNOTSUPP;
>
> -	/* Check that platform supports this profile choice */
> -	if (!test_bit(i, cur_profile->choices)) {
> -		mutex_unlock(&profile_lock);
> -		return -EOPNOTSUPP;
> +		err = cur_profile->profile_set(cur_profile, i);
> +		if (err)
> +			return err;
>   	}
>
> -	err = cur_profile->profile_set(cur_profile, i);
> -	if (!err)
> -		sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>
> -	mutex_unlock(&profile_lock);
> -	if (err)
> -		return err;
>   	return count;
>   }
>
> @@ -140,36 +123,28 @@ int platform_profile_cycle(void)
>   	enum platform_profile_option next;
>   	int err;
>
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		if (!cur_profile)
> +			return -ENODEV;
>
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> +		err = cur_profile->profile_get(cur_profile, &profile);
> +		if (err)
> +			return err;
>
> -	err = cur_profile->profile_get(cur_profile, &profile);
> -	if (err) {
> -		mutex_unlock(&profile_lock);
> -		return err;
> -	}
> +		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
> +					  profile + 1);
>
> -	next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
> -				  profile + 1);
> +		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
> +			return -EINVAL;
>
> -	if (WARN_ON(next == PLATFORM_PROFILE_LAST)) {
> -		mutex_unlock(&profile_lock);
> -		return -EINVAL;
> +		err = cur_profile->profile_set(cur_profile, next);
> +		if (err)
> +			return err;
>   	}
>
> -	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);
>

Re: [PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> The name attribute shows the name of the associated platform profile
> handler.
>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index ef6af2c655524..4e2eda18f7f5f 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
>   static DEFINE_IDA(platform_profile_ida);
>
> +/**
> + * name_show - Show the name of the profile handler
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to write to
> + * Return: The number of bytes written
> + */
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct platform_profile_handler *handler = dev_get_drvdata(dev);
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		return sysfs_emit(buf, "%s\n", handler->name);
> +	}
> +	return -ERESTARTSYS;

I still have a bad feeling about the locking inside the class attributes...

Can we assume that no sysfs accesses occur after unregistering the class device?

Even if this is not the case then the locking fails to protect the platform_profile_handler here.
If the device is unregistered right after dev_get_drvdata() was called, then we would sill operate
on possibly stale data once we take the profile_lock.

Does someone have any clue how sysfs attributes act during removal?

Thanks,
Armin Wolf

> +}
> +
> +static DEVICE_ATTR_RO(name);
> +static struct attribute *profile_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(profile);
> +
>   static const struct class platform_profile_class = {
>   	.name = "platform-profile",
> +	.dev_groups = profile_groups,
>   };
>
>   static ssize_t platform_profile_choices_show(struct device *dev,

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

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

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

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v5:
>   * adjust mutex use
> ---
>   drivers/acpi/platform_profile.c | 56 ++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index c4d451782ff18..c4e2c75116988 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -206,26 +206,54 @@ static const struct class platform_profile_class = {
>   	.dev_groups = profile_groups,
>   };
>
> +/**
> + * _aggregate_choices - Aggregate the available profile choices
> + * @dev: The device
> + * @data: The available profile choices
> + * Return: 0 on success, -errno on failure
> + */
> +static int _aggregate_choices(struct device *dev, void *data)
> +{
> +	struct platform_profile_handler *handler;
> +	unsigned long *aggregate = data;
> +
> +	lockdep_assert_held(&profile_lock);
> +	handler = dev_get_drvdata(dev);
> +	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> +		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> +	else
> +		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> +
> +	return 0;
> +}
> +
> +/**
> + * platform_profile_choices_show - Show the available profile choices for legacy sysfs interface
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to write to
> + * Return: The number of bytes written
> + */
>   static ssize_t platform_profile_choices_show(struct device *dev,
> -					struct device_attribute *attr,
> -					char *buf)
> +					     struct device_attribute *attr,
> +					     char *buf)
>   {
> -	int len = 0;
> -	int i;
> +	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	int err;
>
> +	set_bit(PLATFORM_PROFILE_LAST, aggregate);
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> -		for_each_set_bit(i, cur_profile->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]);
> -		}
> +		err = class_for_each_device(&platform_profile_class, NULL,
> +					    aggregate, _aggregate_choices);
> +		if (err)
> +			return err;
>   	}
> -	len += sysfs_emit_at(buf, len, "\n");
>
> -	return len;
> +	/* no profile handler registered any more */
> +	if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
> +		return -EINVAL;
> +
> +	return _commmon_choices_show(aggregate, buf);
>   }
>
>   static ssize_t platform_profile_show(struct device *dev,

Re: [PATCH v6 17/22] ACPI: platform_profile: Make sure all profile handlers agree on profile

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb Mario Limonciello:

> If for any reason multiple profile handlers don't agree on the profile
> return the custom profile.
>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6:
>   * Fix logic error with PLATFORM_PROFILE_CUSTOM
> v5:
>   * Notify class profile of change to legacy interface
>   * Don't show warning when writing custom string, document in last patch
>     instead.
>   * Adjust mutex use
> ---
>   drivers/acpi/platform_profile.c | 120 ++++++++++++++++++++++++++------
>   1 file changed, 97 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 089ac73b3ec97..2676f4a13689e 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -71,6 +71,22 @@ static int _store_class_profile(struct device *dev, void *data)
>   	return 0;
>   }
>
> +/**
> + * _notify_class_profile - Notify the class device of a profile change
> + * @dev: The class device
> + * @data: Unused
> + */
> +static int _notify_class_profile(struct device *dev, void *data)
> +{
> +	struct platform_profile_handler *handler = dev_get_drvdata(dev);
> +
> +	lockdep_assert_held(&profile_lock);
> +	sysfs_notify(&handler->class_dev->kobj, NULL, "profile");
> +	kobject_uevent(&handler->class_dev->kobj, KOBJ_CHANGE);
> +
> +	return 0;
> +}
> +
>   /**
>    * get_class_profile - Show the current profile for a class device
>    * @dev: The class device
> @@ -257,51 +273,109 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>   	return _commmon_choices_show(aggregate, buf);
>   }
>
> +/**
> + * _aggregate_profiles - Aggregate the profiles for legacy sysfs interface
> + * @dev: The device
> + * @data: The profile to return
> + * Return: 0 on success, -errno on failure
> + */
> +static int _aggregate_profiles(struct device *dev, void *data)
> +{
> +	enum platform_profile_option *profile = data;
> +	enum platform_profile_option val;
> +	int err;
> +
> +	err = get_class_profile(dev, &val);
> +	if (err)
> +		return err;
> +
> +	if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> +		*profile = PLATFORM_PROFILE_CUSTOM;
> +	else
> +		*profile = val;
> +
> +	return 0;
> +}
> +

Please don't use multiple blank lines. With that fixed:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> +
> +/**
> + * _store_and_notify - Atomically store and notify a class from legacy sysfs interface
> + * @dev: The device
> + * @data: The profile to return
> + * Return: 0 on success, -errno on failure
> + */
> +static int _store_and_notify(struct device *dev, void *data)
> +{
> +	enum platform_profile_option *profile = data;
> +	int err;
> +
> +	err = _store_class_profile(dev, profile);
> +	if (err)
> +		return err;
> +	return _notify_class_profile(dev, NULL);
> +}
> +
> +/**
> + * platform_profile_show - Show the current profile for legacy sysfs interface
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to write to
> + * Return: The number of bytes written
> + */
>   static ssize_t platform_profile_show(struct device *dev,
> -					struct device_attribute *attr,
> -					char *buf)
> +				     struct device_attribute *attr,
> +				     char *buf)
>   {
> -	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
> +	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>   	int err;
>
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> -
> -		err = cur_profile->profile_get(cur_profile, &profile);
> +		err = class_for_each_device(&platform_profile_class, NULL,
> +					    &profile, _aggregate_profiles);
>   		if (err)
>   			return err;
>   	}
>
> -	/* Check that profile is valid index */
> -	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> -		return -EIO;
> +	/* no profile handler registered any more */
> +	if (profile == PLATFORM_PROFILE_LAST)
> +		return -EINVAL;
>
>   	return sysfs_emit(buf, "%s\n", profile_names[profile]);
>   }
>
> +/**
> + * platform_profile_store - Set the profile for legacy sysfs interface
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to read from
> + * @count: The number of bytes to read
> + * Return: The number of bytes read
> + */
>   static ssize_t platform_profile_store(struct device *dev,
> -			    struct device_attribute *attr,
> -			    const char *buf, size_t count)
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
>   {
> -	int err, i;
> +	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	int ret;
> +	int i;
>
>   	/* Scan for a matching profile */
>   	i = sysfs_match_string(profile_names, buf);
> -	if (i < 0)
> +	if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
>   		return -EINVAL;
> -
> +	set_bit(PLATFORM_PROFILE_LAST, choices);
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> -
> -		/* Check that platform supports this profile choice */
> -		if (!test_bit(i, cur_profile->choices))
> +		ret = class_for_each_device(&platform_profile_class, NULL,
> +					    choices, _aggregate_choices);
> +		if (ret)
> +			return ret;
> +		if (!test_bit(i, choices))
>   			return -EOPNOTSUPP;
>
> -		err = cur_profile->profile_set(cur_profile, i);
> -		if (err)
> -			return err;
> +		ret = class_for_each_device(&platform_profile_class, NULL, &i,
> +					    _store_and_notify);
> +		if (ret)
> +			return ret;
>   	}
>
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");

Re: [PATCH v6 18/22] ACPI: platform_profile: Check all profile handler to calculate next

[Armin Wolf]

Am 09.11.24 um 05:41 schrieb 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.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6:
>   * Handle cases of inconsistent profiles or all profile handlers
>     supporting custom.
> v5:
>   * Adjust mutex use
> ---
>   drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 2676f4a13689e..c574483be4fd1 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -423,28 +423,40 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>
>   int platform_profile_cycle(void)
>   {
> -	enum platform_profile_option profile;
> -	enum platform_profile_option next;
> +	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
> +	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> +	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>   	int err;
>
>   	if (!class_is_registered(&platform_profile_class))
>   		return -ENODEV;
>
> +	set_bit(PLATFORM_PROFILE_LAST, choices);
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> +		err = class_for_each_device(&platform_profile_class, NULL,
> +					    &profile, _aggregate_profiles);
> +		if (err)
> +			return err;
>
> -		err = cur_profile->profile_get(cur_profile, &profile);
> +		if (profile == PLATFORM_PROFILE_CUSTOM ||
> +		    profile == PLATFORM_PROFILE_LAST)
> +			return -EINVAL;
> +
> +		err = class_for_each_device(&platform_profile_class, NULL,
> +					    choices, _aggregate_choices);
>   		if (err)
>   			return err;
>
> -		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
> +		/* never iterate into a custom if all drivers supported it */
> +		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> +
> +		next = find_next_bit_wrap(choices,
> +					  PLATFORM_PROFILE_LAST,
>   					  profile + 1);
>
> -		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
> -			return -EINVAL;
> +		err = class_for_each_device(&platform_profile_class, NULL, &next,
> +					    _store_class_profile);
>
> -		err = cur_profile->profile_set(cur_profile, next);
>   		if (err)
>   			return err;
>   	}

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

[Armin Wolf]

Am 14.11.24 um 22:57 schrieb Armin Wolf:

> Am 09.11.24 um 05:41 schrieb 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 when the legacy /sys/firmware/acpi/platform_profile interface
>> is used.
>>
>> This series also adds a new concept of a "custom" profile.  This allows
>> userspace to discover that there are multiple driver handlers that are
>> configured differently.
>>
>> This series also allows dropping all of the PMF quirks from amd-pmf.
>
> Sorry for taking a bit long to respond, i am currently quite busy. I
> will try to review this series
> in the coming days.
>
> Thanks,
> Armin Wolf
>
So far the patch series looks quite good, but a single issue remains: the locking around the class attributes.
Maybe someone with some knowledge about sysfs can help us here.

Also can you please rebase the patch series onto the current for-net branch? This would solve a merge conflict
inside the asus-wmi driver.

Thanks,
Armin WOlf

>> ---
>> v6:
>>   * Add patch dev patch but don't make mandatory
>>   * See other patches changelogs for individualized changes
>>
>> Mario Limonciello (22):
>>    ACPI: platform-profile: Add a name member to handlers
>>    platform/x86/dell: dell-pc: Create platform device
>>    ACPI: platform_profile: Add device pointer into platform profile
>>      handler
>>    ACPI: platform_profile: Add platform handler argument to
>>      platform_profile_remove()
>>    ACPI: platform_profile: Pass the profile handler into
>>      platform_profile_notify()
>>    ACPI: platform_profile: Move sanity check out of the mutex
>>    ACPI: platform_profile: Move matching string for new profile out of
>>      mutex
>>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>>    ACPI: platform_profile: Use `scoped_cond_guard`
>>    ACPI: platform_profile: Create class for ACPI platform profile
>>    ACPI: platform_profile: Add name attribute to class interface
>>    ACPI: platform_profile: Add choices attribute for class interface
>>    ACPI: platform_profile: Add profile attribute for class interface
>>    ACPI: platform_profile: Notify change events on register and
>>      unregister
>>    ACPI: platform_profile: Only show profiles common for all handlers
>>    ACPI: platform_profile: Add concept of a "custom" profile
>>    ACPI: platform_profile: Make sure all profile handlers agree on
>>      profile
>>    ACPI: platform_profile: Check all profile handler to calculate next
>>    ACPI: platform_profile: Notify class device from
>>      platform_profile_notify()
>>    ACPI: platform_profile: Allow multiple handlers
>>    platform/x86/amd: pmf: Drop all quirks
>>    Documentation: Add documentation about class interface for platform
>>      profiles
>>
>>   .../ABI/testing/sysfs-platform_profile        |   5 +
>>   .../userspace-api/sysfs-platform_profile.rst  |  28 +
>>   drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
>>   .../surface/surface_platform_profile.c        |   8 +-
>>   drivers/platform/x86/acer-wmi.c               |  12 +-
>>   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            |   4 +-
>>   drivers/platform/x86/asus-wmi.c               |  10 +-
>>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>>   drivers/platform/x86/dell/dell-pc.c           |  36 +-
>>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>>   drivers/platform/x86/ideapad-laptop.c         |   6 +-
>>   .../platform/x86/inspur_platform_profile.c    |   7 +-
>>   drivers/platform/x86/thinkpad_acpi.c          |  16 +-
>>   include/linux/platform_profile.h              |   9 +-
>>   18 files changed, 553 insertions(+), 213 deletions(-)
>>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>>
>>
>> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
>

Re: [PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Armin Wolf]

Am 18.11.24 um 20:43 schrieb Armin Wolf:

> Am 09.11.24 um 05:41 schrieb Mario Limonciello:
>
>> The name attribute shows the name of the associated platform profile
>> handler.
>>
>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/acpi/platform_profile.c
>> b/drivers/acpi/platform_profile.c
>> index ef6af2c655524..4e2eda18f7f5f 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) ==
>> PLATFORM_PROFILE_LAST);
>>
>>   static DEFINE_IDA(platform_profile_ida);
>>
>> +/**
>> + * name_show - Show the name of the profile handler
>> + * @dev: The device
>> + * @attr: The attribute
>> + * @buf: The buffer to write to
>> + * Return: The number of bytes written
>> + */
>> +static ssize_t name_show(struct device *dev,
>> +             struct device_attribute *attr,
>> +             char *buf)
>> +{
>> +    struct platform_profile_handler *handler = dev_get_drvdata(dev);
>> +
>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> +        return sysfs_emit(buf, "%s\n", handler->name);
>> +    }
>> +    return -ERESTARTSYS;
>
> I still have a bad feeling about the locking inside the class
> attributes...
>
> Can we assume that no sysfs accesses occur after unregistering the
> class device?
>
> Even if this is not the case then the locking fails to protect the
> platform_profile_handler here.
> If the device is unregistered right after dev_get_drvdata() was
> called, then we would sill operate
> on possibly stale data once we take the profile_lock.
>
> Does someone have any clue how sysfs attributes act during removal?
>
I think i found the answer to my questions inside this patch series:
https://lore.kernel.org/linux-kernel/1390951311-15325-1-git-send-email-tj@kernel.org

It says that:

	kernfs / sysfs implement the "sever" semantic for userland accesses.
	When a node is removed, no further userland operations are allowed and
	the in-flight ones are drained before removal is finished.  This makes
	policing post-mortem userland accesses trivial for its users.

In this case taking the profile_lock when reading/writing class attributes seems to be unnecessary.
Please remove the unnecessary locking inside the class attributes.

Thanks,
Armin Wolf

> Thanks,
> Armin Wolf
>
>> +}
>> +
>> +static DEVICE_ATTR_RO(name);
>> +static struct attribute *profile_attrs[] = {
>> +    &dev_attr_name.attr,
>> +    NULL
>> +};
>> +ATTRIBUTE_GROUPS(profile);
>> +
>>   static const struct class platform_profile_class = {
>>       .name = "platform-profile",
>> +    .dev_groups = profile_groups,
>>   };
>>
>>   static ssize_t platform_profile_choices_show(struct device *dev,
>

Re: [PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Mario Limonciello]

On 11/18/2024 18:28, Armin Wolf wrote:
> Am 18.11.24 um 20:43 schrieb Armin Wolf:
> 
>> Am 09.11.24 um 05:41 schrieb Mario Limonciello:
>>
>>> The name attribute shows the name of the associated platform profile
>>> handler.
>>>
>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
>>>   1 file changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c
>>> b/drivers/acpi/platform_profile.c
>>> index ef6af2c655524..4e2eda18f7f5f 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) ==
>>> PLATFORM_PROFILE_LAST);
>>>
>>>   static DEFINE_IDA(platform_profile_ida);
>>>
>>> +/**
>>> + * name_show - Show the name of the profile handler
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to write to
>>> + * Return: The number of bytes written
>>> + */
>>> +static ssize_t name_show(struct device *dev,
>>> +             struct device_attribute *attr,
>>> +             char *buf)
>>> +{
>>> +    struct platform_profile_handler *handler = dev_get_drvdata(dev);
>>> +
>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>> +        return sysfs_emit(buf, "%s\n", handler->name);
>>> +    }
>>> +    return -ERESTARTSYS;
>>
>> I still have a bad feeling about the locking inside the class
>> attributes...
>>
>> Can we assume that no sysfs accesses occur after unregistering the
>> class device?
>>
>> Even if this is not the case then the locking fails to protect the
>> platform_profile_handler here.
>> If the device is unregistered right after dev_get_drvdata() was
>> called, then we would sill operate
>> on possibly stale data once we take the profile_lock.
>>
>> Does someone have any clue how sysfs attributes act during removal?
>>
> I think i found the answer to my questions inside this patch series:
> https://lore.kernel.org/linux-kernel/1390951311-15325-1-git-send-email- 
> tj@kernel.org
> 
> It says that:
> 
>      kernfs / sysfs implement the "sever" semantic for userland accesses.
>      When a node is removed, no further userland operations are allowed and
>      the in-flight ones are drained before removal is finished.  This makes
>      policing post-mortem userland accesses trivial for its users.
> 
> In this case taking the profile_lock when reading/writing class 
> attributes seems to be unnecessary.
> Please remove the unnecessary locking inside the class attributes.
> 

Before I respin a v7, let's make sure we're agreed on which things need 
locking and which don't.

Functions that check if a lock is held:
_store_class_profile()
_notify_class_profile()
get_class_profile()
_aggregate_choices()

Functions that take a lock:
name_show()
choices_show()
profile_show()
profile_store()
platform_profile_choices_show()
platform_profile_show()
platform_profile_store()
platform_profile_cycle()
platform_profile_register()
platform_profile_remove()

Functions that don't take or check for a lock (these are intermediary 
and things they call check for one):
_aggregate_profiles()
_store_and_notify()

Are you suggesting that basically these 4 can drop taking the lock?
name_show()
choices_show()
profile_show()
profile_store()

I think the show() ones I can get behind, but I'm worried about 
profile_store(), particularly as it pertains to the other callers of 
_store_class_profile() because it's incongruent how the other callers 
would use it then.

Can we perhaps just drop it for the 3 class attribute show() ones?

LMK.

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

[Mario Limonciello]

On 11/18/2024 15:05, Armin Wolf wrote:
> Am 14.11.24 um 22:57 schrieb Armin Wolf:
> 
>> Am 09.11.24 um 05:41 schrieb 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 when the legacy /sys/firmware/acpi/platform_profile interface
>>> is used.
>>>
>>> This series also adds a new concept of a "custom" profile.  This allows
>>> userspace to discover that there are multiple driver handlers that are
>>> configured differently.
>>>
>>> This series also allows dropping all of the PMF quirks from amd-pmf.
>>
>> Sorry for taking a bit long to respond, i am currently quite busy. I
>> will try to review this series
>> in the coming days.
>>
>> Thanks,
>> Armin Wolf
>>
> So far the patch series looks quite good, but a single issue remains: 
> the locking around the class attributes.
> Maybe someone with some knowledge about sysfs can help us here.
> 
> Also can you please rebase the patch series onto the current for-net 
> branch? This would solve a merge conflict
> inside the asus-wmi driver.

Yeah; I've done this locally.  I was going to wait until 6.13-rc1 to 
send it out, but I guess if we don't have any other merge conflicts 
coming in this code I'll send it after we are in agreement on the 
locking and I do some more testing.

> 
> Thanks,
> Armin WOlf
> 
>>> ---
>>> v6:
>>>   * Add patch dev patch but don't make mandatory
>>>   * See other patches changelogs for individualized changes
>>>
>>> Mario Limonciello (22):
>>>    ACPI: platform-profile: Add a name member to handlers
>>>    platform/x86/dell: dell-pc: Create platform device
>>>    ACPI: platform_profile: Add device pointer into platform profile
>>>      handler
>>>    ACPI: platform_profile: Add platform handler argument to
>>>      platform_profile_remove()
>>>    ACPI: platform_profile: Pass the profile handler into
>>>      platform_profile_notify()
>>>    ACPI: platform_profile: Move sanity check out of the mutex
>>>    ACPI: platform_profile: Move matching string for new profile out of
>>>      mutex
>>>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>>>    ACPI: platform_profile: Use `scoped_cond_guard`
>>>    ACPI: platform_profile: Create class for ACPI platform profile
>>>    ACPI: platform_profile: Add name attribute to class interface
>>>    ACPI: platform_profile: Add choices attribute for class interface
>>>    ACPI: platform_profile: Add profile attribute for class interface
>>>    ACPI: platform_profile: Notify change events on register and
>>>      unregister
>>>    ACPI: platform_profile: Only show profiles common for all handlers
>>>    ACPI: platform_profile: Add concept of a "custom" profile
>>>    ACPI: platform_profile: Make sure all profile handlers agree on
>>>      profile
>>>    ACPI: platform_profile: Check all profile handler to calculate next
>>>    ACPI: platform_profile: Notify class device from
>>>      platform_profile_notify()
>>>    ACPI: platform_profile: Allow multiple handlers
>>>    platform/x86/amd: pmf: Drop all quirks
>>>    Documentation: Add documentation about class interface for platform
>>>      profiles
>>>
>>>   .../ABI/testing/sysfs-platform_profile        |   5 +
>>>   .../userspace-api/sysfs-platform_profile.rst  |  28 +
>>>   drivers/acpi/platform_profile.c               | 537 ++++++++++++++----
>>>   .../surface/surface_platform_profile.c        |   8 +-
>>>   drivers/platform/x86/acer-wmi.c               |  12 +-
>>>   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            |   4 +-
>>>   drivers/platform/x86/asus-wmi.c               |  10 +-
>>>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>>>   drivers/platform/x86/dell/dell-pc.c           |  36 +-
>>>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>>>   drivers/platform/x86/ideapad-laptop.c         |   6 +-
>>>   .../platform/x86/inspur_platform_profile.c    |   7 +-
>>>   drivers/platform/x86/thinkpad_acpi.c          |  16 +-
>>>   include/linux/platform_profile.h              |   9 +-
>>>   18 files changed, 553 insertions(+), 213 deletions(-)
>>>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>>>
>>>
>>> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
>>

Re: [PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Armin Wolf]

Am 19.11.24 um 05:09 schrieb Mario Limonciello:

> On 11/18/2024 18:28, Armin Wolf wrote:
>> Am 18.11.24 um 20:43 schrieb Armin Wolf:
>>
>>> Am 09.11.24 um 05:41 schrieb Mario Limonciello:
>>>
>>>> The name attribute shows the name of the associated platform profile
>>>> handler.
>>>>
>>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>   drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
>>>>   1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c
>>>> b/drivers/acpi/platform_profile.c
>>>> index ef6af2c655524..4e2eda18f7f5f 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) ==
>>>> PLATFORM_PROFILE_LAST);
>>>>
>>>>   static DEFINE_IDA(platform_profile_ida);
>>>>
>>>> +/**
>>>> + * name_show - Show the name of the profile handler
>>>> + * @dev: The device
>>>> + * @attr: The attribute
>>>> + * @buf: The buffer to write to
>>>> + * Return: The number of bytes written
>>>> + */
>>>> +static ssize_t name_show(struct device *dev,
>>>> +             struct device_attribute *attr,
>>>> +             char *buf)
>>>> +{
>>>> +    struct platform_profile_handler *handler = dev_get_drvdata(dev);
>>>> +
>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>>> &profile_lock) {
>>>> +        return sysfs_emit(buf, "%s\n", handler->name);
>>>> +    }
>>>> +    return -ERESTARTSYS;
>>>
>>> I still have a bad feeling about the locking inside the class
>>> attributes...
>>>
>>> Can we assume that no sysfs accesses occur after unregistering the
>>> class device?
>>>
>>> Even if this is not the case then the locking fails to protect the
>>> platform_profile_handler here.
>>> If the device is unregistered right after dev_get_drvdata() was
>>> called, then we would sill operate
>>> on possibly stale data once we take the profile_lock.
>>>
>>> Does someone have any clue how sysfs attributes act during removal?
>>>
>> I think i found the answer to my questions inside this patch series:
>> https://lore.kernel.org/linux-kernel/1390951311-15325-1-git-send-email-
>> tj@kernel.org
>>
>> It says that:
>>
>>      kernfs / sysfs implement the "sever" semantic for userland
>> accesses.
>>      When a node is removed, no further userland operations are
>> allowed and
>>      the in-flight ones are drained before removal is finished. This
>> makes
>>      policing post-mortem userland accesses trivial for its users.
>>
>> In this case taking the profile_lock when reading/writing class
>> attributes seems to be unnecessary.
>> Please remove the unnecessary locking inside the class attributes.
>>
>
> Before I respin a v7, let's make sure we're agreed on which things
> need locking and which don't.
>
> Functions that check if a lock is held:
> _store_class_profile()
> _notify_class_profile()
> get_class_profile()
> _aggregate_choices()
>
> Functions that take a lock:
> name_show()
> choices_show()
> profile_show()
> profile_store()
> platform_profile_choices_show()
> platform_profile_show()
> platform_profile_store()
> platform_profile_cycle()
> platform_profile_register()
> platform_profile_remove()
>
> Functions that don't take or check for a lock (these are intermediary
> and things they call check for one):
> _aggregate_profiles()
> _store_and_notify()
>
> Are you suggesting that basically these 4 can drop taking the lock?
> name_show()
> choices_show()
> profile_show()
> profile_store()
>
> I think the show() ones I can get behind, but I'm worried about
> profile_store(), particularly as it pertains to the other callers of
> _store_class_profile() because it's incongruent how the other callers
> would use it then.
>
> Can we perhaps just drop it for the 3 class attribute show() ones?

I think so, i also remembered that profile_store() needs to keep taking the lock in case platform_profile_cycle() is currently
running.

Can you also remove the second call to dev_get_drvdata() in _store_class_profile()?

Thanks,
Armin Wolf

>
> LMK.
>
>
>

Re: [PATCH v6 10/22] ACPI: platform_profile: Create class for ACPI platform profile

[Armin Wolf]

Am 17.11.24 um 20:11 schrieb Armin Wolf:

> Am 09.11.24 um 05:41 schrieb Mario Limonciello:
>
>> When registering a platform profile handler create a class device
>> that will allow changing a single platform profile handler.
>>
>> The class and sysfs group are no longer needed when the platform profile
>> core is a module and unloaded, so remove them at that time as well.
>>
>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v6:
>>   * Catch failures in ida_alloc
>>   * Use 4th argument of device_create instead of dev_set_drvdata()
>>   * Squash unregister patch
>>   * Add module init callback
>>   * Move class creation to module init
>>   * Update visibility based on group presence
>>   * Add back parent device
>> v5:
>>   * Use ida instead of idr
>>   * Use device_unregister instead of device_destroy()
>>   * MKDEV (0, 0)
>> ---
>>   drivers/acpi/platform_profile.c  | 88 ++++++++++++++++++++++++++++++--
>>   include/linux/platform_profile.h |  2 +
>>   2 files changed, 85 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c
>> b/drivers/acpi/platform_profile.c
>> index 32affb75e782d..ef6af2c655524 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -5,6 +5,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/bits.h>
>>   #include <linux/init.h>
>> +#include <linux/kdev_t.h>
>>   #include <linux/mutex.h>
>>   #include <linux/platform_profile.h>
>>   #include <linux/sysfs.h>
>> @@ -22,6 +23,12 @@ static const char * const profile_names[] = {
>>   };
>>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>
>> +static DEFINE_IDA(platform_profile_ida);
>> +
>> +static const struct class platform_profile_class = {
>> +    .name = "platform-profile",
>> +};
>> +
>>   static ssize_t platform_profile_choices_show(struct device *dev,
>>                       struct device_attribute *attr,
>>                       char *buf)
>> @@ -105,8 +112,25 @@ static struct attribute
>> *platform_profile_attrs[] = {
>>       NULL
>>   };
>>
>> +static int profile_class_registered(struct device *dev, const void
>> *data)
>> +{
>> +    return 1;
>> +}
>> +
>> +static umode_t profile_class_is_visible(struct kobject *kobj, struct
>> attribute *attr, int idx)
>> +{
>> +    if (!class_find_device(&platform_profile_class, NULL, NULL,
>> profile_class_registered))
>> +        return 0;
>> +    if (attr == &dev_attr_platform_profile_choices.attr)
>> +        return 0444;
>> +    if (attr == &dev_attr_platform_profile.attr)
>> +        return 0644;
>> +    return 0;
>> +}
>> +
>>   static const struct attribute_group platform_profile_group = {
>> -    .attrs = platform_profile_attrs
>> +    .attrs = platform_profile_attrs,
>> +    .is_visible = profile_class_is_visible,
>>   };
>>
>>   void platform_profile_notify(struct platform_profile_handler *pprof)
>> @@ -123,6 +147,9 @@ int platform_profile_cycle(void)
>>       enum platform_profile_option next;
>>       int err;
>>
>> +    if (!class_is_registered(&platform_profile_class))
>> +        return -ENODEV;
>
> This check is pointless since the platform profile class will always
> be registered during module initialization.
> Please remove it.
>
>> +
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>> &profile_lock) {
>>           if (!cur_profile)
>>               return -ENODEV;
>> @@ -164,25 +191,76 @@ int platform_profile_register(struct
>> platform_profile_handler *pprof)
>>       if (cur_profile)
>>           return -EEXIST;
>>
>> -    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> -    if (err)
>> -        return err;
>> +    /* create class interface for individual handler */
>> +    pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
>> +    if (pprof->minor < 0)
>> +        return pprof->minor;
>> +    pprof->class_dev = device_create(&platform_profile_class,
>> pprof->dev,
>> +                     MKDEV(0, 0), pprof, "platform-profile-%d",
>> +                     pprof->minor);
>> +    if (IS_ERR(pprof->class_dev)) {
>> +        err = PTR_ERR(pprof->class_dev);
>> +        goto cleanup_ida;
>> +    }
>>
>>       cur_profile = pprof;
>> +
>> +    err = sysfs_update_group(acpi_kobj, &platform_profile_group);
>> +    if (err)
>> +        goto cleanup_cur;
>> +
>>       return 0;
>> +
>> +cleanup_cur:
>> +    cur_profile = NULL;
>> +    device_unregister(pprof->class_dev);
>> +
>> +cleanup_ida:
>> +    ida_free(&platform_profile_ida, pprof->minor);
>> +
>> +    return err;
>>   }
>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>
>>   int platform_profile_remove(struct platform_profile_handler *pprof)
>>   {
>> +    int id;
>>       guard(mutex)(&profile_lock);
>>
>> -    sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +    id = pprof->minor;
>> +    device_unregister(pprof->class_dev);
>> +    ida_free(&platform_profile_ida, id);
>> +
>>       cur_profile = NULL;
>> +
>> +    sysfs_update_group(acpi_kobj, &platform_profile_group);
>> +
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(platform_profile_remove);
>>
>> +static int __init platform_profile_init(void)
>> +{
>> +    int err;
>> +
>> +    err = class_register(&platform_profile_class);
>> +    if (err)
>> +        return err;
>> +
>> +    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +    if (err)
>> +        class_unregister(&platform_profile_class);
>> +
>> +    return err;
>> +}
>
> Please use a blank line after function/struct/union/enum declarations.
>
> Apart from those minor issues the patch looks quite nice, so with
> those issues being fixed:
>
> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>
>> +static void __exit platform_profile_exit(void)
>> +{
>> +    class_unregister(&platform_profile_class);
>> +    sysfs_remove_group(acpi_kobj, &platform_profile_group);

I just noticed that the legacy sysfs group needs to be removed before the class, otherwise
there might be a short time window during module exit where the legacy sysfs interface might
try to use the already unregistered class.

Thanks,
Armin Wolf

>> +}
>> +module_init(platform_profile_init);
>> +module_exit(platform_profile_exit);
>> +
>>   MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
>>   MODULE_DESCRIPTION("ACPI platform profile sysfs interface");
>>   MODULE_LICENSE("GPL");
>> diff --git a/include/linux/platform_profile.h
>> b/include/linux/platform_profile.h
>> index 8ec0b8da56db5..a888fd085c513 100644
>> --- a/include/linux/platform_profile.h
>> +++ b/include/linux/platform_profile.h
>> @@ -29,6 +29,8 @@ enum platform_profile_option {
>>   struct platform_profile_handler {
>>       const char *name;
>>       struct device *dev;
>> +    struct device *class_dev;
>> +    int minor;
>>       unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>       int (*profile_get)(struct platform_profile_handler *pprof,
>>                   enum platform_profile_option *profile);
>

Re: [PATCH v6 18/22] ACPI: platform_profile: Check all profile handler to calculate next

[Armin Wolf]

Am 18.11.24 um 20:53 schrieb Armin Wolf:

> Am 09.11.24 um 05:41 schrieb 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.
>
> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>
>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v6:
>>   * Handle cases of inconsistent profiles or all profile handlers
>>     supporting custom.
>> v5:
>>   * Adjust mutex use
>> ---
>>   drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c 
>> b/drivers/acpi/platform_profile.c
>> index 2676f4a13689e..c574483be4fd1 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -423,28 +423,40 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>
>>   int platform_profile_cycle(void)
>>   {
>> -    enum platform_profile_option profile;
>> -    enum platform_profile_option next;
>> +    enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>> +    enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>> +    unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>       int err;
>>
>>       if (!class_is_registered(&platform_profile_class))
>>           return -ENODEV;
>>
>> +    set_bit(PLATFORM_PROFILE_LAST, choices);
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, 
>> &profile_lock) {
>> -        if (!cur_profile)
>> -            return -ENODEV;
>> +        err = class_for_each_device(&platform_profile_class, NULL,
>> +                        &profile, _aggregate_profiles);
>> +        if (err)
>> +            return err;
>>
>> -        err = cur_profile->profile_get(cur_profile, &profile);
>> +        if (profile == PLATFORM_PROFILE_CUSTOM ||
>> +            profile == PLATFORM_PROFILE_LAST)
>> +            return -EINVAL;
>> +
>> +        err = class_for_each_device(&platform_profile_class, NULL,
>> +                        choices, _aggregate_choices);
>>           if (err)
>>               return err;
>>
>> -        next = find_next_bit_wrap(cur_profile->choices, 
>> PLATFORM_PROFILE_LAST,
>> +        /* never iterate into a custom if all drivers supported it */
>> +        clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
>> +
>> +        next = find_next_bit_wrap(choices,
>> +                      PLATFORM_PROFILE_LAST,
>>                         profile + 1);
>>
>> -        if (WARN_ON(next == PLATFORM_PROFILE_LAST))
>> -            return -EINVAL;
>> +        err = class_for_each_device(&platform_profile_class, NULL, 
>> &next,
>> +                        _store_class_profile);

I just noticed that the class device is not notified here. Please use _store_and_notify() insted of _store_class_profile()
here.

Thanks,
Armin Wolf

>>
>> -        err = cur_profile->profile_set(cur_profile, next);
>>           if (err)
>>               return err;
>>       }
>

Re: [PATCH v6 11/22] ACPI: platform_profile: Add name attribute to class interface

[Mario Limonciello]

On 11/19/2024 06:26, Armin Wolf wrote:
> Am 19.11.24 um 05:09 schrieb Mario Limonciello:
> 
>> On 11/18/2024 18:28, Armin Wolf wrote:
>>> Am 18.11.24 um 20:43 schrieb Armin Wolf:
>>>
>>>> Am 09.11.24 um 05:41 schrieb Mario Limonciello:
>>>>
>>>>> The name attribute shows the name of the associated platform profile
>>>>> handler.
>>>>>
>>>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>>   drivers/acpi/platform_profile.c | 27 +++++++++++++++++++++++++++
>>>>>   1 file changed, 27 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/platform_profile.c
>>>>> b/drivers/acpi/platform_profile.c
>>>>> index ef6af2c655524..4e2eda18f7f5f 100644
>>>>> --- a/drivers/acpi/platform_profile.c
>>>>> +++ b/drivers/acpi/platform_profile.c
>>>>> @@ -25,8 +25,35 @@ static_assert(ARRAY_SIZE(profile_names) ==
>>>>> PLATFORM_PROFILE_LAST);
>>>>>
>>>>>   static DEFINE_IDA(platform_profile_ida);
>>>>>
>>>>> +/**
>>>>> + * name_show - Show the name of the profile handler
>>>>> + * @dev: The device
>>>>> + * @attr: The attribute
>>>>> + * @buf: The buffer to write to
>>>>> + * Return: The number of bytes written
>>>>> + */
>>>>> +static ssize_t name_show(struct device *dev,
>>>>> +             struct device_attribute *attr,
>>>>> +             char *buf)
>>>>> +{
>>>>> +    struct platform_profile_handler *handler = dev_get_drvdata(dev);
>>>>> +
>>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>>>> &profile_lock) {
>>>>> +        return sysfs_emit(buf, "%s\n", handler->name);
>>>>> +    }
>>>>> +    return -ERESTARTSYS;
>>>>
>>>> I still have a bad feeling about the locking inside the class
>>>> attributes...
>>>>
>>>> Can we assume that no sysfs accesses occur after unregistering the
>>>> class device?
>>>>
>>>> Even if this is not the case then the locking fails to protect the
>>>> platform_profile_handler here.
>>>> If the device is unregistered right after dev_get_drvdata() was
>>>> called, then we would sill operate
>>>> on possibly stale data once we take the profile_lock.
>>>>
>>>> Does someone have any clue how sysfs attributes act during removal?
>>>>
>>> I think i found the answer to my questions inside this patch series:
>>> https://lore.kernel.org/linux-kernel/1390951311-15325-1-git-send-email-
>>> tj@kernel.org
>>>
>>> It says that:
>>>
>>>      kernfs / sysfs implement the "sever" semantic for userland
>>> accesses.
>>>      When a node is removed, no further userland operations are
>>> allowed and
>>>      the in-flight ones are drained before removal is finished. This
>>> makes
>>>      policing post-mortem userland accesses trivial for its users.
>>>
>>> In this case taking the profile_lock when reading/writing class
>>> attributes seems to be unnecessary.
>>> Please remove the unnecessary locking inside the class attributes.
>>>
>>
>> Before I respin a v7, let's make sure we're agreed on which things
>> need locking and which don't.
>>
>> Functions that check if a lock is held:
>> _store_class_profile()
>> _notify_class_profile()
>> get_class_profile()
>> _aggregate_choices()
>>
>> Functions that take a lock:
>> name_show()
>> choices_show()
>> profile_show()
>> profile_store()
>> platform_profile_choices_show()
>> platform_profile_show()
>> platform_profile_store()
>> platform_profile_cycle()
>> platform_profile_register()
>> platform_profile_remove()
>>
>> Functions that don't take or check for a lock (these are intermediary
>> and things they call check for one):
>> _aggregate_profiles()
>> _store_and_notify()
>>
>> Are you suggesting that basically these 4 can drop taking the lock?
>> name_show()
>> choices_show()
>> profile_show()
>> profile_store()
>>
>> I think the show() ones I can get behind, but I'm worried about
>> profile_store(), particularly as it pertains to the other callers of
>> _store_class_profile() because it's incongruent how the other callers
>> would use it then.
>>
>> Can we perhaps just drop it for the 3 class attribute show() ones?
> 
> I think so, i also remembered that profile_store() needs to keep taking 
> the lock in case platform_profile_cycle() is currently
> running.

Actually considering this, we need to keep it on profile_show() too then 
for the exact same reason.

I will drop it for choices and name though.

> 
> Can you also remove the second call to dev_get_drvdata() in 
> _store_class_profile()?
> 

Sure.

> Thanks,
> Armin Wolf
> 
>>
>> LMK.
>>
>>
>>