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

[PATCH v3 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.

v3:
 * Split to even more patches
 * Add concept of class device for platform profile handlers
 * Add concept of custom profile for legacy sysfs interface
 * Pick up tags for some patches
v2:
 * Split to many more patches
 * Account for feedback from M/L

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: Add a list to platform profile handler
  ACPI: platform_profile: Move sanity check out of the mutex
  ACPI: platform_profile: Use guard(mutex) for register/unregister
  ACPI: platform_profile: Use `scoped_cond_guard` for
    platform_profile_choices_show()
  ACPI: platform_profile: Use `scoped_cond_guard` for
    platform_profile_show()
  ACPI: platform_profile: Use `scoped_cond_guard` for
    platform_profile_show()
  ACPI: platform_profile: Use `scoped_cond_guard` for
    platform_profile_cycle()
  ACPI: platform_profile: Only remove group when no more handler
    registered
  ACPI: platform_profile: Require handlers to support balanced profile
  ACPI: platform_profile: Notify change events on register and
    unregister
  ACPI: platform_profile: Only show profiles common for all handlers
  ACPI: platform_profile: Set profile for all registered handlers
  ACPI: platform_profile: 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: Register class device for platform profile
    handlers
  ACPI: platform_profile: Allow multiple handlers
  platform/x86/amd: pmf: Drop all quirks

 drivers/acpi/platform_profile.c               | 330 ++++++++++++------
 .../surface/surface_platform_profile.c        |   8 +-
 drivers/platform/x86/acer-wmi.c               |  10 +-
 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               |   6 +-
 drivers/platform/x86/dell/dell-pc.c           |  39 ++-
 drivers/platform/x86/hp/hp-wmi.c              |   8 +-
 drivers/platform/x86/ideapad-laptop.c         |   4 +-
 .../platform/x86/inspur_platform_profile.c    |   7 +-
 drivers/platform/x86/thinkpad_acpi.c          |   4 +-
 include/linux/platform_profile.h              |   8 +-
 15 files changed, 303 insertions(+), 197 deletions(-)
 delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c

-- 
2.43.0

[PATCH v3 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: 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: Lyndon Sanche <lsanche@lyndeno.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/surface/surface_platform_profile.c | 1 +
 drivers/platform/x86/acer-wmi.c                     | 1 +
 drivers/platform/x86/amd/pmf/sps.c                  | 1 +
 drivers/platform/x86/asus-wmi.c                     | 1 +
 drivers/platform/x86/dell/dell-pc.c                 | 1 +
 drivers/platform/x86/hp/hp-wmi.c                    | 1 +
 drivers/platform/x86/ideapad-laptop.c               | 1 +
 drivers/platform/x86/inspur_platform_profile.c      | 1 +
 drivers/platform/x86/thinkpad_acpi.c                | 1 +
 include/linux/platform_profile.h                    | 1 +
 10 files changed, 10 insertions(+)

diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 08db878f1d7d4..9d3e3f9458186 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 7169b84ccdb6e..13a97afe0112d 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1883,6 +1883,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 abdca3f05c5c1..6177fbee60573 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3920,6 +3920,7 @@ static int platform_profile_setup(struct asus_wmi *asus)
 
 	dev_info(dev, "Using throttle_thermal_policy for platform_profile support\n");
 
+	asus->platform_profile_handler.name = "asus-wmi";
 	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
 	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
 
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 972385ca1990b..3cf79e55e3129 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -247,6 +247,7 @@ static int thermal_init(void)
 	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
 	if (!thermal_handler)
 		return -ENOMEM;
+	thermal_handler->name = "dell-pc";
 	thermal_handler->profile_get = thermal_platform_profile_get;
 	thermal_handler->profile_set = thermal_platform_profile_set;
 
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 8c05e0dd2a218..10a853b6b0514 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 c64dfc56651d3..6c72d1b6a2aff 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1102,6 +1102,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 
 	mutex_init(&priv->dytc->mutex);
 
+	priv->dytc->pprof.name = "ideapad-laptop";
 	priv->dytc->priv = priv;
 	priv->dytc->pprof.profile_get = dytc_profile_get;
 	priv->dytc->pprof.profile_set = dytc_profile_set;
diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 8440defa67886..03da2c8cf6789 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -177,6 +177,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
+	priv->handler.name = "inspur-wmi";
 	priv->handler.profile_get = inspur_platform_profile_get;
 	priv->handler.profile_set = inspur_platform_profile_set;
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4c1b0553f8720..c8c316b8507a5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10549,6 +10549,7 @@ static void dytc_profile_refresh(void)
 }
 
 static struct platform_profile_handler dytc_profile = {
+	.name = "thinkpad-acpi",
 	.profile_get = dytc_profile_get,
 	.profile_set = dytc_profile_set,
 };
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index f5492ed413f36..6fa988e417428 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -27,6 +27,7 @@ enum platform_profile_option {
 };
 
 struct platform_profile_handler {
+	const char *name;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
 	int (*profile_get)(struct platform_profile_handler *pprof,
 				enum platform_profile_option *profile);
-- 
2.43.0

[PATCH v3 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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/dell/dell-pc.c | 35 +++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 3cf79e55e3129..b145fedb6b710 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,18 @@ 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_alloc("dell-pc", PLATFORM_DEVID_NONE);
+	if (!platform_device)
 		return -ENOMEM;
+	ret = platform_device_add(platform_device);
+	if (ret)
+		goto cleanup_platform_device;
+
+	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 +274,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_del(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 v3 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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c                     | 5 +++++
 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/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, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d2f7fd7743a13..5d9f3f7ba71c5 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -179,6 +179,11 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
 
+	if (!pprof->dev) {
+		pr_err("platform_profile: handler device is not set\n");
+		return -EINVAL;
+	}
+
 	mutex_lock(&profile_lock);
 	/* We can only have one active profile */
 	if (cur_profile) {
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 9d3e3f9458186..b73cfdd920c66 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 13a97afe0112d..a5caa529351ea 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1878,12 +1878,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 =
@@ -2536,7 +2537,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 6177fbee60573..1a8c29aafe892 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3921,6 +3921,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/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index b145fedb6b710..730f97aab70cd 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -260,6 +260,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 10a853b6b0514..1b6677e176769 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 6c72d1b6a2aff..feaf98819dc82 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 v3 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: 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>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 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/dell-pc.c                 | 2 +-
 drivers/platform/x86/hp/hp-wmi.c                    | 2 +-
 drivers/platform/x86/ideapad-laptop.c               | 2 +-
 drivers/platform/x86/inspur_platform_profile.c      | 5 ++++-
 drivers/platform/x86/thinkpad_acpi.c                | 2 +-
 include/linux/platform_profile.h                    | 2 +-
 11 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 5d9f3f7ba71c5..c76b8e3fdcde6 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -210,7 +210,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 b73cfdd920c66..6c87e982bfc8f 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 a5caa529351ea..44992d5da4768 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -2552,7 +2552,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:
@@ -2575,7 +2575,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 1a8c29aafe892..90c20d02e1638 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4896,7 +4896,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);
@@ -4923,7 +4923,7 @@ static void asus_wmi_remove(struct platform_device *device)
 	asus_wmi_battery_exit(asus);
 
 	if (asus->platform_profile_support)
-		platform_profile_remove();
+		platform_profile_remove(&asus->platform_profile_handler);
 
 	kfree(asus);
 }
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 730f97aab70cd..65989af2e4196 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -292,7 +292,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 1b6677e176769..22a7fc64d230a 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 feaf98819dc82..73030dc6b579b 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 v3 05/22] ACPI: platform_profile: Add a list to platform profile handler

[Mario Limonciello]

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

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index c76b8e3fdcde6..d0198d2ccb551 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -10,6 +10,7 @@
 #include <linux/sysfs.h>
 
 static struct platform_profile_handler *cur_profile;
+static LIST_HEAD(platform_profile_handler_list);
 static DEFINE_MUTEX(profile_lock);
 
 static const char * const profile_names[] = {
@@ -203,6 +204,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		mutex_unlock(&profile_lock);
 		return err;
 	}
+	list_add_tail(&pprof->list, &platform_profile_handler_list);
 
 	cur_profile = pprof;
 	mutex_unlock(&profile_lock);
@@ -212,8 +214,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register);
 
 int platform_profile_remove(struct platform_profile_handler *pprof)
 {
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	list_del(&pprof->list);
 
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 	mutex_lock(&profile_lock);
 	cur_profile = NULL;
 	mutex_unlock(&profile_lock);
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index bcaf3aa39160f..6aad98f4abaf4 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -30,6 +30,7 @@ struct platform_profile_handler {
 	const char *name;
 	struct device *dev;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	struct list_head list;
 	int (*profile_get)(struct platform_profile_handler *pprof,
 				enum platform_profile_option *profile);
 	int (*profile_set)(struct platform_profile_handler *pprof,
-- 
2.43.0

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d0198d2ccb551..f2f2274e4d83e 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
 
+	/* Sanity check the profile handler */
+	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
+	    !pprof->profile_set || !pprof->profile_get) {
+		pr_err("platform_profile: handler is invalid\n");
+		return -EINVAL;
+	}
 	if (!pprof->dev) {
 		pr_err("platform_profile: handler device is not set\n");
 		return -EINVAL;
@@ -192,13 +198,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 v3 07/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: Matthew Schwartz <matthew.schwartz@linux.dev>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

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

[PATCH v3 08/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_choices_show()

[Mario Limonciello]

Migrate away from using an interruptible mutex to scoped_cond_guard.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index cc2037147c0fd..79feb273c1def 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -28,25 +28,21 @@ 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;
 }
 
-- 
2.43.0

[PATCH v3 09/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_show()

[Mario Limonciello]

Use scoped_cond_guard for the interruptible mutex in
platform_profile_show().

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 79feb273c1def..b48dd34301f13 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -53,20 +53,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;
-- 
2.43.0

[PATCH v3 10/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_show()

[Mario Limonciello]

Migrate away from using an interruptible mutex to scoped_cond_guard.
Also move the sysfs string match out of the mutex as it's not needed.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index b48dd34301f13..63a5f5ac33898 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -75,35 +75,25 @@ static ssize_t platform_profile_store(struct device *dev,
 {
 	int err, i;
 
-	err = mutex_lock_interruptible(&profile_lock);
-	if (err)
-		return err;
-
-	if (!cur_profile) {
-		mutex_unlock(&profile_lock);
-		return -ENODEV;
-	}
-
 	/* Scan for a matching profile */
 	i = sysfs_match_string(profile_names, buf);
-	if (i < 0) {
-		mutex_unlock(&profile_lock);
+	if (i < 0)
 		return -EINVAL;
-	}
 
-	/* Check that platform supports this profile choice */
-	if (!test_bit(i, cur_profile->choices)) {
-		mutex_unlock(&profile_lock);
-		return -EOPNOTSUPP;
-	}
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		if (!cur_profile)
+			return -ENODEV;
 
-	err = cur_profile->profile_set(cur_profile, i);
-	if (!err)
-		sysfs_notify(acpi_kobj, NULL, "platform_profile");
+		/* Check that platform supports this profile choice */
+		if (!test_bit(i, cur_profile->choices))
+			return -EOPNOTSUPP;
 
-	mutex_unlock(&profile_lock);
-	if (err)
-		return err;
+		err = cur_profile->profile_set(cur_profile, i);
+		if (err)
+			return err;
+	}
+
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 	return count;
 }
 
-- 
2.43.0

[PATCH v3 11/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_cycle()

[Mario Limonciello]

Migrate away from using an interruptible mutex to scoped_cond_guard.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 63a5f5ac33898..2d971dba2d917 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -124,36 +124,27 @@ 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");
-
-	return err;
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_cycle);
 
-- 
2.43.0

[PATCH v3 12/22] ACPI: platform_profile: Only remove group when no more handler registered

[Mario Limonciello]

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

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 2d971dba2d917..b70ceb11947d0 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -23,6 +23,12 @@ static const char * const profile_names[] = {
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
+static bool platform_profile_is_registered(void)
+{
+	lockdep_assert_held(&profile_lock);
+	return !list_empty(&platform_profile_handler_list);
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -184,8 +190,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 
 	list_del(&pprof->list);
 
-	sysfs_remove_group(acpi_kobj, &platform_profile_group);
 	cur_profile = NULL;
+	if (!platform_profile_is_registered())
+		sysfs_remove_group(acpi_kobj, &platform_profile_group);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
-- 
2.43.0

[PATCH v3 13/22] ACPI: platform_profile: Require handlers to support balanced profile

[Mario Limonciello]

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

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

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index b70ceb11947d0..57c66d7dbf827 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -164,6 +164,10 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 		pr_err("platform_profile: handler is invalid\n");
 		return -EINVAL;
 	}
+	if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
+		pr_err("platform_profile: handler does not support balanced profile\n");
+		return -EINVAL;
+	}
 	if (!pprof->dev) {
 		pr_err("platform_profile: handler device is not set\n");
 		return -EINVAL;
-- 
2.43.0

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

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

[PATCH v3 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: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c | 38 +++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 7bd32f1e8d834..90cbc0de4d5bc 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -29,23 +29,43 @@ static bool platform_profile_is_registered(void)
 	return !list_empty(&platform_profile_handler_list);
 }
 
+static unsigned long platform_profile_get_choices(void)
+{
+	struct platform_profile_handler *handler;
+	unsigned long aggregate = 0;
+	int i;
+
+	lockdep_assert_held(&profile_lock);
+	list_for_each_entry(handler, &platform_profile_handler_list, list) {
+		unsigned long individual = 0;
+
+		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
+			individual |= BIT(i);
+		if (!aggregate)
+			aggregate = individual;
+		else
+			aggregate &= individual;
+	}
+
+	return aggregate;
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
 {
+	unsigned long choices;
 	int len = 0;
 	int i;
 
-	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
-			return -ENODEV;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
+		choices = platform_profile_get_choices();
 
-		for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) {
-			if (len == 0)
-				len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
-			else
-				len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
-		}
+	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");
 
-- 
2.43.0

[PATCH v3 16/22] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

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

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

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

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

[PATCH v3 17/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.

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 c2bb325ba531c..3128bd16615b6 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 6aad98f4abaf4..da009c8a402c9 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 v3 18/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
report the custom profile to userspace.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 3128bd16615b6..5baac1f9a9c0e 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -51,6 +51,36 @@ static unsigned long platform_profile_get_choices(void)
 	return aggregate;
 }
 
+static int platform_profile_get_active(enum platform_profile_option *profile)
+{
+	struct platform_profile_handler *handler;
+	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
+	enum platform_profile_option val;
+	int err;
+
+	lockdep_assert_held(&profile_lock);
+	list_for_each_entry(handler, &platform_profile_handler_list, list) {
+		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;
+
+		if (active != val && active != PLATFORM_PROFILE_LAST) {
+			*profile = PLATFORM_PROFILE_CUSTOM;
+			return 0;
+		}
+		active = val;
+	}
+
+	*profile = active;
+
+	return 0;
+}
+
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -81,18 +111,13 @@ static ssize_t platform_profile_show(struct device *dev,
 	int err;
 
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		if (!cur_profile)
+		if (!platform_profile_is_registered())
 			return -ENODEV;
-
-		err = cur_profile->profile_get(cur_profile, &profile);
+		err = platform_profile_get_active(&profile);
 		if (err)
 			return err;
 	}
 
-	/* Check that profile is valid index */
-	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
-		return -EIO;
-
 	return sysfs_emit(buf, "%s\n", profile_names[profile]);
 }
 
-- 
2.43.0

[PATCH v3 19/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.

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

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

[PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Mario Limonciello]

The "platform_profile" class device has the exact same semantics as the
platform profile files in /sys/firmware/acpi/ but it reflects values only
present for a single platform profile handler.

The expectation is that legacy userspace can change the profile for all
handlers in /sys/firmware/acpi/platform_profile and can change it for
individual handlers by /sys/class/platform_profile/*.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
 include/linux/platform_profile.h |  2 +
 2 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 9b681884ae324..1cc8182930dde 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -24,13 +24,24 @@ static const char * const profile_names[] = {
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
 
+static DEFINE_IDR(platform_profile_minor_idr);
+
+static const struct class platform_profile_class = {
+	.name = "platform-profile",
+};
+
 static bool platform_profile_is_registered(void)
 {
 	lockdep_assert_held(&profile_lock);
 	return !list_empty(&platform_profile_handler_list);
 }
 
-static unsigned long platform_profile_get_choices(void)
+static bool platform_profile_is_class_device(struct device *dev)
+{
+	return dev && dev->class == &platform_profile_class;
+}
+
+static unsigned long platform_profile_get_choices(struct device *dev)
 {
 	struct platform_profile_handler *handler;
 	unsigned long aggregate = 0;
@@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
 	list_for_each_entry(handler, &platform_profile_handler_list, list) {
 		unsigned long individual = 0;
 
+		/* if called from a class attribute then only match that one */
+		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
+			continue;
 		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
 			individual |= BIT(i);
 		if (!aggregate)
@@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
 	return aggregate;
 }
 
-static int platform_profile_get_active(enum platform_profile_option *profile)
+static int platform_profile_get_active(struct device *dev, enum platform_profile_option *profile)
 {
 	struct platform_profile_handler *handler;
 	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
@@ -60,6 +74,8 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
 
 	lockdep_assert_held(&profile_lock);
 	list_for_each_entry(handler, &platform_profile_handler_list, list) {
+		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
+			continue;
 		err = handler->profile_get(handler, &val);
 		if (err) {
 			pr_err("Failed to get profile for handler %s\n", handler->name);
@@ -69,6 +85,10 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
 		if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
 			return -EINVAL;
 
+		/*
+		 * If the profiles are different for class devices then this must
+		 * show "custom" to legacy sysfs interface
+		 */
 		if (active != val && active != PLATFORM_PROFILE_LAST) {
 			*profile = PLATFORM_PROFILE_CUSTOM;
 			return 0;
@@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 	int i;
 
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
-		choices = platform_profile_get_choices();
+		choices = platform_profile_get_choices(dev);
 
 	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
 		if (len == 0)
@@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device *dev,
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		if (!platform_profile_is_registered())
 			return -ENODEV;
-		err = platform_profile_get_active(&profile);
+		err = platform_profile_get_active(dev, &profile);
 		if (err)
 			return err;
 	}
@@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct device *dev,
 		if (!platform_profile_is_registered())
 			return -ENODEV;
 
-		/* Check that all handlers support this profile choice */
-		choices = platform_profile_get_choices();
+		/* don't allow setting custom to legacy sysfs interface */
+		if (!platform_profile_is_class_device(dev) &&
+		     i == PLATFORM_PROFILE_CUSTOM) {
+			pr_warn("Custom profile not supported for legacy sysfs interface\n");
+			return -EINVAL;
+		}
+
+		/* Check that applicable handlers support this profile choice */
+		choices = platform_profile_get_choices(dev);
 		if (!test_bit(i, &choices))
 			return -EOPNOTSUPP;
 
 		list_for_each_entry(handler, &platform_profile_handler_list, list) {
+			if (platform_profile_is_class_device(dev) &&
+			    handler->dev != dev->parent)
+				continue;
 			err = handler->profile_set(handler, i);
 			if (err) {
 				pr_err("Failed to set profile for handler %s\n", handler->name);
@@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct device *dev,
 		}
 		if (err) {
 			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
+				if (platform_profile_is_class_device(dev) &&
+				    handler->dev != dev->parent)
+					continue;
 				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
 					pr_err("Failed to revert profile for handler %s\n",
 					       handler->name);
@@ -194,11 +227,11 @@ int platform_profile_cycle(void)
 	int err;
 
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		err = platform_profile_get_active(&profile);
+		err = platform_profile_get_active(NULL, &profile);
 		if (err)
 			return err;
 
-		choices = platform_profile_get_choices();
+		choices = platform_profile_get_choices(NULL);
 
 		next = find_next_bit_wrap(&choices,
 					  PLATFORM_PROFILE_LAST,
@@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
 
 int platform_profile_register(struct platform_profile_handler *pprof)
 {
+	bool registered;
 	int err;
 
 	/* Sanity check the profile handler */
@@ -250,14 +284,49 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	if (cur_profile)
 		return -EEXIST;
 
-	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
+	registered = platform_profile_is_registered();
+	if (!registered) {
+		/* class for individual handlers */
+		err = class_register(&platform_profile_class);
+		if (err)
+			return err;
+		/* legacy sysfs files */
+		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
+		if (err)
+			goto cleanup_class;
+
+	}
+
+	/* create class interface for individual handler */
+	pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
+	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
+					 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
+					 pprof->name);
+	if (IS_ERR(pprof->class_dev)) {
+		err = PTR_ERR(pprof->class_dev);
+		goto cleanup_legacy;
+	}
+	err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
 	if (err)
-		return err;
+		goto cleanup_device;
+
 	list_add_tail(&pprof->list, &platform_profile_handler_list);
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
 	cur_profile = pprof;
 	return 0;
+
+cleanup_device:
+	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
+
+cleanup_legacy:
+	if (!registered)
+		sysfs_remove_group(acpi_kobj, &platform_profile_group);
+cleanup_class:
+	if (!registered)
+		class_unregister(&platform_profile_class);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(platform_profile_register);
 
@@ -270,6 +339,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 	cur_profile = NULL;
 
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
+	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
+	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
+
 	if (!platform_profile_is_registered())
 		sysfs_remove_group(acpi_kobj, &platform_profile_group);
 
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index da009c8a402c9..764c4812ef759 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -30,6 +30,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)];
 	struct list_head list;
 	int (*profile_get)(struct platform_profile_handler *pprof,
-- 
2.43.0

[PATCH v3 21/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 revert
back to the balanced profile, which is now required.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 1cc8182930dde..a845524a248b9 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -9,7 +9,6 @@
 #include <linux/platform_profile.h>
 #include <linux/sysfs.h>
 
-static struct platform_profile_handler *cur_profile;
 static LIST_HEAD(platform_profile_handler_list);
 static DEFINE_MUTEX(profile_lock);
 
@@ -212,7 +211,8 @@ static const struct attribute_group platform_profile_group = {
 
 void platform_profile_notify(void)
 {
-	if (!cur_profile)
+	guard(mutex)(&profile_lock);
+	if (!platform_profile_is_registered())
 		return;
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 }
@@ -280,9 +280,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;
 
 	registered = platform_profile_is_registered();
 	if (!registered) {
@@ -313,7 +310,6 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	list_add_tail(&pprof->list, &platform_profile_handler_list);
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
-	cur_profile = pprof;
 	return 0;
 
 cleanup_device:
@@ -336,8 +332,6 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 
 	list_del(&pprof->list);
 
-	cur_profile = NULL;
-
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
 
 	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
-- 
2.43.0

[PATCH v3 22/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.

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

Re: [PATCH v3 10/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_show()

[Ilpo Järvinen]

On Wed, 30 Oct 2024, Mario Limonciello wrote:

> Migrate away from using an interruptible mutex to scoped_cond_guard.
> Also move the sysfs string match out of the mutex as it's not needed.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

I'd have expected all the mutex_lock_interruptible() -> 
scoped_cond_guard() changes in the same patch. Although this also moves 
the sysfs stuff out which in this smaller form is kind of okay but if it's 
part of larger patch merging all scoped guard conversions, it should be 
in a different change.

-- 
 i.

> ---
>  drivers/acpi/platform_profile.c | 36 ++++++++++++---------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index b48dd34301f13..63a5f5ac33898 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -75,35 +75,25 @@ static ssize_t platform_profile_store(struct device *dev,
>  {
>  	int err, i;
>  
> -	err = mutex_lock_interruptible(&profile_lock);
> -	if (err)
> -		return err;
> -
> -	if (!cur_profile) {
> -		mutex_unlock(&profile_lock);
> -		return -ENODEV;
> -	}
> -
>  	/* Scan for a matching profile */
>  	i = sysfs_match_string(profile_names, buf);
> -	if (i < 0) {
> -		mutex_unlock(&profile_lock);
> +	if (i < 0)
>  		return -EINVAL;
> -	}
>  
> -	/* Check that platform supports this profile choice */
> -	if (!test_bit(i, cur_profile->choices)) {
> -		mutex_unlock(&profile_lock);
> -		return -EOPNOTSUPP;
> -	}
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		if (!cur_profile)
> +			return -ENODEV;
>  
> -	err = cur_profile->profile_set(cur_profile, i);
> -	if (!err)
> -		sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +		/* Check that platform supports this profile choice */
> +		if (!test_bit(i, cur_profile->choices))
> +			return -EOPNOTSUPP;
>  
> -	mutex_unlock(&profile_lock);
> -	if (err)
> -		return err;
> +		err = cur_profile->profile_set(cur_profile, i);
> +		if (err)
> +			return err;
> +	}
> +
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>  	return count;
>  }
>  
> 

Re: [PATCH v3 07/22] ACPI: platform_profile: Use guard(mutex) for register/unregister

[Ilpo Järvinen]

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

On Wed, 30 Oct 2024, Mario Limonciello wrote:

> guard(mutex) can be used to automatically release mutexes when going
> out of scope.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index f2f2274e4d83e..cc2037147c0fd 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -191,34 +191,29 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>  		return -EINVAL;
>  	}
>  
> -	mutex_lock(&profile_lock);
> +	guard(mutex)(&profile_lock);
>  	/* We can only have one active profile */
> -	if (cur_profile) {
> -		mutex_unlock(&profile_lock);
> +	if (cur_profile)
>  		return -EEXIST;
> -	}
>  
>  	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> -	if (err) {
> -		mutex_unlock(&profile_lock);
> +	if (err)
>  		return err;
> -	}
>  	list_add_tail(&pprof->list, &platform_profile_handler_list);
>  
>  	cur_profile = pprof;
> -	mutex_unlock(&profile_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(platform_profile_register);
>  
>  int platform_profile_remove(struct platform_profile_handler *pprof)
>  {
> +	guard(mutex)(&profile_lock);
> +
>  	list_del(&pprof->list);
>  
>  	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> -	mutex_lock(&profile_lock);
>  	cur_profile = NULL;
> -	mutex_unlock(&profile_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(platform_profile_remove);
> 

For patches 1-7,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3 16/22] ACPI: platform_profile: Set profile for all registered handlers

[Ilpo Järvinen]

On Wed, 30 Oct 2024, Mario Limonciello wrote:

> If multiple platform profile handlers have been registered then when
> setting a profile verify that all profile handlers support the requested
> profile and set it to each handler.
> 
> If this fails for any given handler, revert all profile handlers back to
> balanced and log an error into the kernel ring buffer.
> 
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 90cbc0de4d5bc..c2bb325ba531c 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -99,6 +99,8 @@ static ssize_t platform_profile_store(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf, size_t count)
>  {
> +	struct platform_profile_handler *handler;
> +	unsigned long choices;
>  	int err, i;
>  
>  	/* Scan for a matching profile */
> @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device *dev,
>  		return -EINVAL;
>  
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> +		if (!platform_profile_is_registered())
>  			return -ENODEV;
>  
> -		/* Check that platform supports this profile choice */
> -		if (!test_bit(i, cur_profile->choices))
> +		/* Check that all handlers support this profile choice */
> +		choices = platform_profile_get_choices();
> +		if (!test_bit(i, &choices))
>  			return -EOPNOTSUPP;
>  
> -		err = cur_profile->profile_set(cur_profile, i);
> -		if (err)
> +		list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +			err = handler->profile_set(handler, i);
> +			if (err) {
> +				pr_err("Failed to set profile for handler %s\n", handler->name);
> +				break;
> +			}
> +		}
> +		if (err) {
> +			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {

Too long line.

This looks an error rollback though so instead of break inside the loop 
you could goto into a label at the end of the function and have much less 
indentation to begin with.

> +				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
> +					pr_err("Failed to revert profile for handler %s\n",
> +					       handler->name);
> +			}
>  			return err;
> +		}
>  	}
>  
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> 

-- 
 i.

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

[Ilpo Järvinen]

On Wed, 30 Oct 2024, Mario Limonciello wrote:

> 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.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 35 ++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 5baac1f9a9c0e..9b681884ae324 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -187,30 +187,41 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>  
>  int platform_profile_cycle(void)
>  {
> +	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
> +	struct platform_profile_handler *handler;
>  	enum platform_profile_option profile;
> -	enum platform_profile_option next;
> +	unsigned long choices;
>  	int err;
>  
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> -
> -		err = cur_profile->profile_get(cur_profile, &profile);
> +		err = platform_profile_get_active(&profile);

If platform_profile_cycle() is called without any profiles registered, 
platform_profile_get_active() will return success.

I suppose you'd want check first that there is one registered and return 
-EINVAL if none are, especially since this is an exported function.

>  		if (err)
>  			return err;
>  
> -		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
> -					  profile + 1);
> +		choices = platform_profile_get_choices();
>  
> -		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
> -			return -EINVAL;
> +		next = find_next_bit_wrap(&choices,
> +					  PLATFORM_PROFILE_LAST,
> +					  profile + 1);
>  
> -		err = cur_profile->profile_set(cur_profile, next);
> -		if (err)
> -			return err;
> +		list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +			err = handler->profile_set(handler, next);
> +			if (err) {
> +				pr_err("Failed to set profile for handler %s\n", handler->name);
> +				break;
> +			}
> +		}
> +		if (err) {

Same here, instead of break you could goto to a rollback label to 
deindent the rollback loop a lot and make the code flow more obvious too.

> +			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
> +				err = handler->profile_set(handler, PLATFORM_PROFILE_BALANCED);
> +				if (err)
> +					pr_err("Failed to revert profile for handler %s\n", handler->name);
> +			}
> +		}
>  	}
>  
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(platform_profile_cycle);
> 

-- 
 i.

Re: [PATCH v3 10/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_show()

[Mario Limonciello]

On 10/31/2024 05:15, Ilpo Järvinen wrote:
> On Wed, 30 Oct 2024, Mario Limonciello wrote:
> 
>> Migrate away from using an interruptible mutex to scoped_cond_guard.
>> Also move the sysfs string match out of the mutex as it's not needed.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> I'd have expected all the mutex_lock_interruptible() ->
> scoped_cond_guard() changes in the same patch. Although this also moves
> the sysfs stuff out which in this smaller form is kind of okay but if it's
> part of larger patch merging all scoped guard conversions, it should be
> in a different change.
> 

OK for the next version I'll merge all the mutex changes to the same 
patch but split the sysfs change to it's own.

Re: [PATCH v3 16/22] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

On 10/31/2024 05:25, Ilpo Järvinen wrote:
> On Wed, 30 Oct 2024, Mario Limonciello wrote:
> 
>> If multiple platform profile handlers have been registered then when
>> setting a profile verify that all profile handlers support the requested
>> profile and set it to each handler.
>>
>> If this fails for any given handler, revert all profile handlers back to
>> balanced and log an error into the kernel ring buffer.
>>
>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index 90cbc0de4d5bc..c2bb325ba531c 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -99,6 +99,8 @@ static ssize_t platform_profile_store(struct device *dev,
>>   			    struct device_attribute *attr,
>>   			    const char *buf, size_t count)
>>   {
>> +	struct platform_profile_handler *handler;
>> +	unsigned long choices;
>>   	int err, i;
>>   
>>   	/* Scan for a matching profile */
>> @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device *dev,
>>   		return -EINVAL;
>>   
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> -		if (!cur_profile)
>> +		if (!platform_profile_is_registered())
>>   			return -ENODEV;
>>   
>> -		/* Check that platform supports this profile choice */
>> -		if (!test_bit(i, cur_profile->choices))
>> +		/* Check that all handlers support this profile choice */
>> +		choices = platform_profile_get_choices();
>> +		if (!test_bit(i, &choices))
>>   			return -EOPNOTSUPP;
>>   
>> -		err = cur_profile->profile_set(cur_profile, i);
>> -		if (err)
>> +		list_for_each_entry(handler, &platform_profile_handler_list, list) {
>> +			err = handler->profile_set(handler, i);
>> +			if (err) {
>> +				pr_err("Failed to set profile for handler %s\n", handler->name);
>> +				break;
>> +			}
>> +		}
>> +		if (err) {
>> +			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
> 
> Too long line.
> 
> This looks an error rollback though so instead of break inside the loop
> you could goto into a label at the end of the function and have much less
> indentation to begin with.

How does the scoped_cond_guard interact with a goto?  With the jump I 
had guessed it goes out of scope, but I wasn't really sure what the 
compiler does.

I guess in the goto label I'll need another scoped_cond_guard()?

> 
>> +				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
>> +					pr_err("Failed to revert profile for handler %s\n",
>> +					       handler->name);
>> +			}
>>   			return err;
>> +		}
>>   	}
>>   
>>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
> 

Re: [PATCH v3 16/22] ACPI: platform_profile: Set profile for all registered handlers

[Ilpo Järvinen]

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

On Thu, 31 Oct 2024, Mario Limonciello wrote:

> On 10/31/2024 05:25, Ilpo Järvinen wrote:
> > On Wed, 30 Oct 2024, Mario Limonciello wrote:
> > 
> > > If multiple platform profile handlers have been registered then when
> > > setting a profile verify that all profile handlers support the requested
> > > profile and set it to each handler.
> > > 
> > > If this fails for any given handler, revert all profile handlers back to
> > > balanced and log an error into the kernel ring buffer.
> > > 
> > > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/acpi/platform_profile.c | 25 ++++++++++++++++++++-----
> > >   1 file changed, 20 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/platform_profile.c
> > > b/drivers/acpi/platform_profile.c
> > > index 90cbc0de4d5bc..c2bb325ba531c 100644
> > > --- a/drivers/acpi/platform_profile.c
> > > +++ b/drivers/acpi/platform_profile.c
> > > @@ -99,6 +99,8 @@ static ssize_t platform_profile_store(struct device
> > > *dev,
> > >   			    struct device_attribute *attr,
> > >   			    const char *buf, size_t count)
> > >   {
> > > +	struct platform_profile_handler *handler;
> > > +	unsigned long choices;
> > >   	int err, i;
> > >     	/* Scan for a matching profile */
> > > @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device
> > > *dev,
> > >   		return -EINVAL;
> > >     	scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
> > > &profile_lock) {
> > > -		if (!cur_profile)
> > > +		if (!platform_profile_is_registered())
> > >   			return -ENODEV;
> > >   -		/* Check that platform supports this profile choice */
> > > -		if (!test_bit(i, cur_profile->choices))
> > > +		/* Check that all handlers support this profile choice */
> > > +		choices = platform_profile_get_choices();
> > > +		if (!test_bit(i, &choices))
> > >   			return -EOPNOTSUPP;
> > >   -		err = cur_profile->profile_set(cur_profile, i);
> > > -		if (err)
> > > +		list_for_each_entry(handler, &platform_profile_handler_list,
> > > list) {
> > > +			err = handler->profile_set(handler, i);
> > > +			if (err) {
> > > +				pr_err("Failed to set profile for handler
> > > %s\n", handler->name);
> > > +				break;
> > > +			}
> > > +		}
> > > +		if (err) {
> > > +			list_for_each_entry_continue_reverse(handler,
> > > &platform_profile_handler_list, list) {
> > 
> > Too long line.
> > 
> > This looks an error rollback though so instead of break inside the loop
> > you could goto into a label at the end of the function and have much less
> > indentation to begin with.
> 
> How does the scoped_cond_guard interact with a goto?  With the jump I had
> guessed it goes out of scope, but I wasn't really sure what the compiler does.
> 
> I guess in the goto label I'll need another scoped_cond_guard()?

Ah, the scope problem is a good point.

Perhaps you could instead add e.g. platform_profile_reset_default() and 
call that before break, both patches that had the rollback did the same 
thing anyway so it can be reused too.

-- 
 i.

Re: [PATCH v3 16/22] ACPI: platform_profile: Set profile for all registered handlers

[Mario Limonciello]

On 10/31/2024 09:37, Ilpo Järvinen wrote:
> On Thu, 31 Oct 2024, Mario Limonciello wrote:
> 
>> On 10/31/2024 05:25, Ilpo Järvinen wrote:
>>> On Wed, 30 Oct 2024, Mario Limonciello wrote:
>>>
>>>> If multiple platform profile handlers have been registered then when
>>>> setting a profile verify that all profile handlers support the requested
>>>> profile and set it to each handler.
>>>>
>>>> If this fails for any given handler, revert all profile handlers back to
>>>> balanced and log an error into the kernel ring buffer.
>>>>
>>>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/acpi/platform_profile.c | 25 ++++++++++++++++++++-----
>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c
>>>> b/drivers/acpi/platform_profile.c
>>>> index 90cbc0de4d5bc..c2bb325ba531c 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -99,6 +99,8 @@ static ssize_t platform_profile_store(struct device
>>>> *dev,
>>>>    			    struct device_attribute *attr,
>>>>    			    const char *buf, size_t count)
>>>>    {
>>>> +	struct platform_profile_handler *handler;
>>>> +	unsigned long choices;
>>>>    	int err, i;
>>>>      	/* Scan for a matching profile */
>>>> @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device
>>>> *dev,
>>>>    		return -EINVAL;
>>>>      	scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>>> &profile_lock) {
>>>> -		if (!cur_profile)
>>>> +		if (!platform_profile_is_registered())
>>>>    			return -ENODEV;
>>>>    -		/* Check that platform supports this profile choice */
>>>> -		if (!test_bit(i, cur_profile->choices))
>>>> +		/* Check that all handlers support this profile choice */
>>>> +		choices = platform_profile_get_choices();
>>>> +		if (!test_bit(i, &choices))
>>>>    			return -EOPNOTSUPP;
>>>>    -		err = cur_profile->profile_set(cur_profile, i);
>>>> -		if (err)
>>>> +		list_for_each_entry(handler, &platform_profile_handler_list,
>>>> list) {
>>>> +			err = handler->profile_set(handler, i);
>>>> +			if (err) {
>>>> +				pr_err("Failed to set profile for handler
>>>> %s\n", handler->name);
>>>> +				break;
>>>> +			}
>>>> +		}
>>>> +		if (err) {
>>>> +			list_for_each_entry_continue_reverse(handler,
>>>> &platform_profile_handler_list, list) {
>>>
>>> Too long line.
>>>
>>> This looks an error rollback though so instead of break inside the loop
>>> you could goto into a label at the end of the function and have much less
>>> indentation to begin with.
>>
>> How does the scoped_cond_guard interact with a goto?  With the jump I had
>> guessed it goes out of scope, but I wasn't really sure what the compiler does.
>>
>> I guess in the goto label I'll need another scoped_cond_guard()?
> 
> Ah, the scope problem is a good point.
> 
> Perhaps you could instead add e.g. platform_profile_reset_default() and
> call that before break, both patches that had the rollback did the same
> thing anyway so it can be reused too.
> 

Sounds like a good idea, thanks.

Re: [PATCH v3 13/22] ACPI: platform_profile: Require handlers to support balanced profile

[Armin Wolf]

Am 31.10.24 um 05:09 schrieb Mario Limonciello:

> As support for multiple simultaneous platform handers is introduced it's
> important they have at least the balanced profile in common.
>
> This will be used as a fallback in case setting the profile across one of the
> handlers happens to fail.

Do we actually need this patch anymore now that we have the "custom" platform profile?
If setting the platform profile fails for some handlers, then we simply display the current
platform profile as "custom".

Thanks,
Armin Wolf

> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index b70ceb11947d0..57c66d7dbf827 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -164,6 +164,10 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   		pr_err("platform_profile: handler is invalid\n");
>   		return -EINVAL;
>   	}
> +	if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
> +		pr_err("platform_profile: handler does not support balanced profile\n");
> +		return -EINVAL;
> +	}
>   	if (!pprof->dev) {
>   		pr_err("platform_profile: handler device is not set\n");
>   		return -EINVAL;

Re: [PATCH v3 13/22] ACPI: platform_profile: Require handlers to support balanced profile

[Mario Limonciello]

On 10/31/2024 15:39, Armin Wolf wrote:
> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
> 
>> As support for multiple simultaneous platform handers is introduced it's
>> important they have at least the balanced profile in common.
>>
>> This will be used as a fallback in case setting the profile across one 
>> of the
>> handlers happens to fail.
> 
> Do we actually need this patch anymore now that we have the "custom" 
> platform profile?
> If setting the platform profile fails for some handlers, then we simply 
> display the current
> platform profile as "custom".

Yes; it's still needed because 'balanced' is used as the fallback of 
something failed.  If you fail to write to a handler it gets you back to 
a known place for all GPUs.

Now I suppose it's up for discussion if that's really the right thing to do.

Maybe because of custom we don't even need that.

If I have 3 profile handlers in
low-power
balanced
balanced

IE I'm already in 'custom'.

If I try to write performance and the first two succeed but the third 
fails what's better:

performance
performance
balanced

Or

balanced
balanced
balanced


> 
> Thanks,
> Armin Wolf
> 
>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/ 
>> platform_profile.c
>> index b70ceb11947d0..57c66d7dbf827 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -164,6 +164,10 @@ int platform_profile_register(struct 
>> platform_profile_handler *pprof)
>>           pr_err("platform_profile: handler is invalid\n");
>>           return -EINVAL;
>>       }
>> +    if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
>> +        pr_err("platform_profile: handler does not support balanced 
>> profile\n");
>> +        return -EINVAL;
>> +    }
>>       if (!pprof->dev) {
>>           pr_err("platform_profile: handler device is not set\n");
>>           return -EINVAL;

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Armin Wolf]

Am 31.10.24 um 05:09 schrieb Mario Limonciello:

> The "platform_profile" class device has the exact same semantics as the
> platform profile files in /sys/firmware/acpi/ but it reflects values only
> present for a single platform profile handler.
>
> The expectation is that legacy userspace can change the profile for all
> handlers in /sys/firmware/acpi/platform_profile and can change it for
> individual handlers by /sys/class/platform_profile/*.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
>   include/linux/platform_profile.h |  2 +
>   2 files changed, 85 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 9b681884ae324..1cc8182930dde 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>   };
>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> +static DEFINE_IDR(platform_profile_minor_idr);
> +
> +static const struct class platform_profile_class = {
> +	.name = "platform-profile",
> +};
> +
>   static bool platform_profile_is_registered(void)
>   {
>   	lockdep_assert_held(&profile_lock);
>   	return !list_empty(&platform_profile_handler_list);
>   }
>
> -static unsigned long platform_profile_get_choices(void)
> +static bool platform_profile_is_class_device(struct device *dev)
> +{
> +	return dev && dev->class == &platform_profile_class;
> +}
> +
> +static unsigned long platform_profile_get_choices(struct device *dev)
>   {
>   	struct platform_profile_handler *handler;
>   	unsigned long aggregate = 0;
> @@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
>   	list_for_each_entry(handler, &platform_profile_handler_list, list) {
>   		unsigned long individual = 0;
>
> +		/* if called from a class attribute then only match that one */
> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
> +			continue;

I do not like how the sysfs attributes for the platform-profile class are handled:

1. We should use .dev_groups instead of manually registering the sysfs attributes.
2. Can we name the sysfs attributes for the class a bit differently ("profile_choices" and "profile")
    and use separate store/show functions for those?
3. Why do we still need platform_profile_handler_list?

This would allow us to get rid of platform_profile_is_class_device().

>   		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>   			individual |= BIT(i);
>   		if (!aggregate)
> @@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
>   	return aggregate;
>   }
>
> -static int platform_profile_get_active(enum platform_profile_option *profile)
> +static int platform_profile_get_active(struct device *dev, enum platform_profile_option *profile)
>   {
>   	struct platform_profile_handler *handler;
>   	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>
>   	lockdep_assert_held(&profile_lock);
>   	list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
> +			continue;
>   		err = handler->profile_get(handler, &val);
>   		if (err) {
>   			pr_err("Failed to get profile for handler %s\n", handler->name);
> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>   		if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>   			return -EINVAL;
>
> +		/*
> +		 * If the profiles are different for class devices then this must
> +		 * show "custom" to legacy sysfs interface
> +		 */
>   		if (active != val && active != PLATFORM_PROFILE_LAST) {
>   			*profile = PLATFORM_PROFILE_CUSTOM;
>   			return 0;
> @@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>   	int i;
>
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
> -		choices = platform_profile_get_choices();
> +		choices = platform_profile_get_choices(dev);
>
>   	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>   		if (len == 0)
> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device *dev,
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>   		if (!platform_profile_is_registered())
>   			return -ENODEV;
> -		err = platform_profile_get_active(&profile);
> +		err = platform_profile_get_active(dev, &profile);
>   		if (err)
>   			return err;
>   	}
> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct device *dev,
>   		if (!platform_profile_is_registered())
>   			return -ENODEV;
>
> -		/* Check that all handlers support this profile choice */
> -		choices = platform_profile_get_choices();
> +		/* don't allow setting custom to legacy sysfs interface */
> +		if (!platform_profile_is_class_device(dev) &&
> +		     i == PLATFORM_PROFILE_CUSTOM) {
> +			pr_warn("Custom profile not supported for legacy sysfs interface\n");
> +			return -EINVAL;
> +		}
> +
> +		/* Check that applicable handlers support this profile choice */
> +		choices = platform_profile_get_choices(dev);
>   		if (!test_bit(i, &choices))
>   			return -EOPNOTSUPP;
>
>   		list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +			if (platform_profile_is_class_device(dev) &&
> +			    handler->dev != dev->parent)
> +				continue;
>   			err = handler->profile_set(handler, i);
>   			if (err) {
>   				pr_err("Failed to set profile for handler %s\n", handler->name);
> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct device *dev,
>   		}
>   		if (err) {
>   			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
> +				if (platform_profile_is_class_device(dev) &&
> +				    handler->dev != dev->parent)
> +					continue;
>   				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
>   					pr_err("Failed to revert profile for handler %s\n",
>   					       handler->name);
> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>   	int err;
>
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		err = platform_profile_get_active(&profile);
> +		err = platform_profile_get_active(NULL, &profile);
>   		if (err)
>   			return err;
>
> -		choices = platform_profile_get_choices();
> +		choices = platform_profile_get_choices(NULL);
>
>   		next = find_next_bit_wrap(&choices,
>   					  PLATFORM_PROFILE_LAST,
> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>
>   int platform_profile_register(struct platform_profile_handler *pprof)
>   {
> +	bool registered;
>   	int err;
>
>   	/* Sanity check the profile handler */
> @@ -250,14 +284,49 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   	if (cur_profile)
>   		return -EEXIST;
>
> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +	registered = platform_profile_is_registered();
> +	if (!registered) {
> +		/* class for individual handlers */
> +		err = class_register(&platform_profile_class);
> +		if (err)
> +			return err;

Why do we need to unregister the class here? From my point of view, having a empty class if no
platform profiles are registered is totally fine.

> +		/* legacy sysfs files */
> +		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +		if (err)
> +			goto cleanup_class;
> +
> +	}
> +
> +	/* create class interface for individual handler */
> +	pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
> +	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
> +					 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
> +					 pprof->name);

I would suggest that the name of the class devices should not contain the platform profile name,
as this would mean that two platform profile handlers cannot have the same name.

Maybe using "platform-profile-<minor>" would be a better solution here? The name can instead be
read using an additional sysfs property.

Thanks,
Armin Wolf

> +	if (IS_ERR(pprof->class_dev)) {
> +		err = PTR_ERR(pprof->class_dev);
> +		goto cleanup_legacy;
> +	}
> +	err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
>   	if (err)
> -		return err;
> +		goto cleanup_device;
> +
>   	list_add_tail(&pprof->list, &platform_profile_handler_list);
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>
>   	cur_profile = pprof;
>   	return 0;
> +
> +cleanup_device:
> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
> +
> +cleanup_legacy:
> +	if (!registered)
> +		sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +cleanup_class:
> +	if (!registered)
> +		class_unregister(&platform_profile_class);
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_register);
>
> @@ -270,6 +339,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>   	cur_profile = NULL;
>
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
> +	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
> +
>   	if (!platform_profile_is_registered())
>   		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index da009c8a402c9..764c4812ef759 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -30,6 +30,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)];
>   	struct list_head list;
>   	int (*profile_get)(struct platform_profile_handler *pprof,

Re: [PATCH v3 13/22] ACPI: platform_profile: Require handlers to support balanced profile

[Armin Wolf]

Am 31.10.24 um 21:43 schrieb Mario Limonciello:

> On 10/31/2024 15:39, Armin Wolf wrote:
>> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
>>
>>> As support for multiple simultaneous platform handers is introduced
>>> it's
>>> important they have at least the balanced profile in common.
>>>
>>> This will be used as a fallback in case setting the profile across
>>> one of the
>>> handlers happens to fail.
>>
>> Do we actually need this patch anymore now that we have the "custom"
>> platform profile?
>> If setting the platform profile fails for some handlers, then we
>> simply display the current
>> platform profile as "custom".
>
> Yes; it's still needed because 'balanced' is used as the fallback of
> something failed.  If you fail to write to a handler it gets you back
> to a known place for all GPUs.
>
> Now I suppose it's up for discussion if that's really the right thing
> to do.
>
> Maybe because of custom we don't even need that.
>
> If I have 3 profile handlers in
> low-power
> balanced
> balanced
>
> IE I'm already in 'custom'.
>
> If I try to write performance and the first two succeed but the third
> fails what's better:
>
> performance
> performance
> balanced
>
> Or
>
> balanced
> balanced
> balanced
>
I think the first is better, as we cannot really guarantee that setting "balanced" for all handlers
will always work.

Thanks,
Armin Wolf

>>
>> Thanks,
>> Armin Wolf
>>
>>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/acpi/platform_profile.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index b70ceb11947d0..57c66d7dbf827 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -164,6 +164,10 @@ int platform_profile_register(struct
>>> platform_profile_handler *pprof)
>>>           pr_err("platform_profile: handler is invalid\n");
>>>           return -EINVAL;
>>>       }
>>> +    if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
>>> +        pr_err("platform_profile: handler does not support balanced
>>> profile\n");
>>> +        return -EINVAL;
>>> +    }
>>>       if (!pprof->dev) {
>>>           pr_err("platform_profile: handler device is not set\n");
>>>           return -EINVAL;
>

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Mario Limonciello]

On 10/31/2024 15:55, Armin Wolf wrote:
> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
> 
>> The "platform_profile" class device has the exact same semantics as the
>> platform profile files in /sys/firmware/acpi/ but it reflects values only
>> present for a single platform profile handler.
>>
>> The expectation is that legacy userspace can change the profile for all
>> handlers in /sys/firmware/acpi/platform_profile and can change it for
>> individual handlers by /sys/class/platform_profile/*.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
>>   include/linux/platform_profile.h |  2 +
>>   2 files changed, 85 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/ 
>> platform_profile.c
>> index 9b681884ae324..1cc8182930dde 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>>   };
>>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>
>> +static DEFINE_IDR(platform_profile_minor_idr);
>> +
>> +static const struct class platform_profile_class = {
>> +    .name = "platform-profile",
>> +};
>> +
>>   static bool platform_profile_is_registered(void)
>>   {
>>       lockdep_assert_held(&profile_lock);
>>       return !list_empty(&platform_profile_handler_list);
>>   }
>>
>> -static unsigned long platform_profile_get_choices(void)
>> +static bool platform_profile_is_class_device(struct device *dev)
>> +{
>> +    return dev && dev->class == &platform_profile_class;
>> +}
>> +
>> +static unsigned long platform_profile_get_choices(struct device *dev)
>>   {
>>       struct platform_profile_handler *handler;
>>       unsigned long aggregate = 0;
>> @@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
>>       list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>>           unsigned long individual = 0;
>>
>> +        /* if called from a class attribute then only match that one */
>> +        if (platform_profile_is_class_device(dev) && handler->dev != 
>> dev->parent)
>> +            continue;
> 
> I do not like how the sysfs attributes for the platform-profile class 
> are handled:
> 
> 1. We should use .dev_groups instead of manually registering the sysfs 
> attributes.
> 2. Can we name the sysfs attributes for the class a bit differently 
> ("profile_choices" and "profile")
>     and use separate store/show functions for those?

Sure.

> 3. Why do we still need platform_profile_handler_list?

The main reason for the list is for iteration and checking if it's empty.
I guess class_for_each_device() could serve the same purpose, but this 
patch probably needs to be way earlier in the series then.

> 
> This would allow us to get rid of platform_profile_is_class_device().
> 
>>           for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>>               individual |= BIT(i);
>>           if (!aggregate)
>> @@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
>>       return aggregate;
>>   }
>>
>> -static int platform_profile_get_active(enum platform_profile_option 
>> *profile)
>> +static int platform_profile_get_active(struct device *dev, enum 
>> platform_profile_option *profile)
>>   {
>>       struct platform_profile_handler *handler;
>>       enum platform_profile_option active = PLATFORM_PROFILE_LAST;
>> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum 
>> platform_profile_option *profile)
>>
>>       lockdep_assert_held(&profile_lock);
>>       list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>> +        if (platform_profile_is_class_device(dev) && handler->dev != 
>> dev->parent)
>> +            continue;
>>           err = handler->profile_get(handler, &val);
>>           if (err) {
>>               pr_err("Failed to get profile for handler %s\n", 
>> handler->name);
>> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum 
>> platform_profile_option *profile)
>>           if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>               return -EINVAL;
>>
>> +        /*
>> +         * If the profiles are different for class devices then this 
>> must
>> +         * show "custom" to legacy sysfs interface
>> +         */
>>           if (active != val && active != PLATFORM_PROFILE_LAST) {
>>               *profile = PLATFORM_PROFILE_CUSTOM;
>>               return 0;
>> @@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct 
>> device *dev,
>>       int i;
>>
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
>> -        choices = platform_profile_get_choices();
>> +        choices = platform_profile_get_choices(dev);
>>
>>       for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>>           if (len == 0)
>> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device 
>> *dev,
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>           if (!platform_profile_is_registered())
>>               return -ENODEV;
>> -        err = platform_profile_get_active(&profile);
>> +        err = platform_profile_get_active(dev, &profile);
>>           if (err)
>>               return err;
>>       }
>> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct 
>> device *dev,
>>           if (!platform_profile_is_registered())
>>               return -ENODEV;
>>
>> -        /* Check that all handlers support this profile choice */
>> -        choices = platform_profile_get_choices();
>> +        /* don't allow setting custom to legacy sysfs interface */
>> +        if (!platform_profile_is_class_device(dev) &&
>> +             i == PLATFORM_PROFILE_CUSTOM) {
>> +            pr_warn("Custom profile not supported for legacy sysfs 
>> interface\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        /* Check that applicable handlers support this profile choice */
>> +        choices = platform_profile_get_choices(dev);
>>           if (!test_bit(i, &choices))
>>               return -EOPNOTSUPP;
>>
>>           list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>> +            if (platform_profile_is_class_device(dev) &&
>> +                handler->dev != dev->parent)
>> +                continue;
>>               err = handler->profile_set(handler, i);
>>               if (err) {
>>                   pr_err("Failed to set profile for handler %s\n", 
>> handler->name);
>> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct 
>> device *dev,
>>           }
>>           if (err) {
>>               list_for_each_entry_continue_reverse(handler, 
>> &platform_profile_handler_list, list) {
>> +                if (platform_profile_is_class_device(dev) &&
>> +                    handler->dev != dev->parent)
>> +                    continue;
>>                   if (handler->profile_set(handler, 
>> PLATFORM_PROFILE_BALANCED))
>>                       pr_err("Failed to revert profile for handler %s\n",
>>                              handler->name);
>> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>>       int err;
>>
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> -        err = platform_profile_get_active(&profile);
>> +        err = platform_profile_get_active(NULL, &profile);
>>           if (err)
>>               return err;
>>
>> -        choices = platform_profile_get_choices();
>> +        choices = platform_profile_get_choices(NULL);
>>
>>           next = find_next_bit_wrap(&choices,
>>                         PLATFORM_PROFILE_LAST,
>> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>
>>   int platform_profile_register(struct platform_profile_handler *pprof)
>>   {
>> +    bool registered;
>>       int err;
>>
>>       /* Sanity check the profile handler */
>> @@ -250,14 +284,49 @@ int platform_profile_register(struct 
>> platform_profile_handler *pprof)
>>       if (cur_profile)
>>           return -EEXIST;
>>
>> -    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +    registered = platform_profile_is_registered();
>> +    if (!registered) {
>> +        /* class for individual handlers */
>> +        err = class_register(&platform_profile_class);
>> +        if (err)
>> +            return err;
> 
> Why do we need to unregister the class here? From my point of view, 
> having a empty class if no
> platform profiles are registered is totally fine.

Hmm, OK.

> 
>> +        /* legacy sysfs files */
>> +        err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +        if (err)
>> +            goto cleanup_class;
>> +
>> +    }
>> +
>> +    /* create class interface for individual handler */
>> +    pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 
>> 0, GFP_KERNEL);
>> +    pprof->class_dev = device_create(&platform_profile_class, pprof- 
>> >dev,
>> +                     MKDEV(0, pprof->minor), NULL, "platform-profile- 
>> %s",
>> +                     pprof->name);
> 
> I would suggest that the name of the class devices should not contain 
> the platform profile name,
> as this would mean that two platform profile handlers cannot have the 
> same name.
> 
> Maybe using "platform-profile-<minor>" would be a better solution here? 
> The name can instead be
> read using an additional sysfs property.

Sure makes sense.

> 
> Thanks,
> Armin Wolf
> 
>> +    if (IS_ERR(pprof->class_dev)) {
>> +        err = PTR_ERR(pprof->class_dev);
>> +        goto cleanup_legacy;
>> +    }
>> +    err = sysfs_create_group(&pprof->class_dev->kobj, 
>> &platform_profile_group);
>>       if (err)
>> -        return err;
>> +        goto cleanup_device;
>> +
>>       list_add_tail(&pprof->list, &platform_profile_handler_list);
>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
>>       cur_profile = pprof;
>>       return 0;
>> +
>> +cleanup_device:
>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>> +cleanup_legacy:
>> +    if (!registered)
>> +        sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +cleanup_class:
>> +    if (!registered)
>> +        class_unregister(&platform_profile_class);
>> +
>> +    return err;
>>   }
>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>
>> @@ -270,6 +339,10 @@ int platform_profile_remove(struct 
>> platform_profile_handler *pprof)
>>       cur_profile = NULL;
>>
>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +
>> +    sysfs_remove_group(&pprof->class_dev->kobj, 
>> &platform_profile_group);
>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>>       if (!platform_profile_is_registered())
>>           sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>
>> diff --git a/include/linux/platform_profile.h b/include/linux/ 
>> platform_profile.h
>> index da009c8a402c9..764c4812ef759 100644
>> --- a/include/linux/platform_profile.h
>> +++ b/include/linux/platform_profile.h
>> @@ -30,6 +30,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)];
>>       struct list_head list;
>>       int (*profile_get)(struct platform_profile_handler *pprof,

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Armin Wolf]

Am 31.10.24 um 22:54 schrieb Mario Limonciello:

> On 10/31/2024 15:55, Armin Wolf wrote:
>> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
>>
>>> The "platform_profile" class device has the exact same semantics as the
>>> platform profile files in /sys/firmware/acpi/ but it reflects values
>>> only
>>> present for a single platform profile handler.
>>>
>>> The expectation is that legacy userspace can change the profile for all
>>> handlers in /sys/firmware/acpi/platform_profile and can change it for
>>> individual handlers by /sys/class/platform_profile/*.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/acpi/platform_profile.c  | 93
>>> ++++++++++++++++++++++++++++----
>>>   include/linux/platform_profile.h |  2 +
>>>   2 files changed, 85 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index 9b681884ae324..1cc8182930dde 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>>>   };
>>>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>>
>>> +static DEFINE_IDR(platform_profile_minor_idr);
>>> +
>>> +static const struct class platform_profile_class = {
>>> +    .name = "platform-profile",
>>> +};
>>> +
>>>   static bool platform_profile_is_registered(void)
>>>   {
>>>       lockdep_assert_held(&profile_lock);
>>>       return !list_empty(&platform_profile_handler_list);
>>>   }
>>>
>>> -static unsigned long platform_profile_get_choices(void)
>>> +static bool platform_profile_is_class_device(struct device *dev)
>>> +{
>>> +    return dev && dev->class == &platform_profile_class;
>>> +}
>>> +
>>> +static unsigned long platform_profile_get_choices(struct device *dev)
>>>   {
>>>       struct platform_profile_handler *handler;
>>>       unsigned long aggregate = 0;
>>> @@ -40,6 +51,9 @@ static unsigned long
>>> platform_profile_get_choices(void)
>>>       list_for_each_entry(handler, &platform_profile_handler_list,
>>> list) {
>>>           unsigned long individual = 0;
>>>
>>> +        /* if called from a class attribute then only match that
>>> one */
>>> +        if (platform_profile_is_class_device(dev) && handler->dev
>>> != dev->parent)
>>> +            continue;
>>
>> I do not like how the sysfs attributes for the platform-profile class
>> are handled:
>>
>> 1. We should use .dev_groups instead of manually registering the
>> sysfs attributes.
>> 2. Can we name the sysfs attributes for the class a bit differently
>> ("profile_choices" and "profile")
>>     and use separate store/show functions for those?
>
> Sure.
>
>> 3. Why do we still need platform_profile_handler_list?
>
> The main reason for the list is for iteration and checking if it's empty.
> I guess class_for_each_device() could serve the same purpose, but this
> patch probably needs to be way earlier in the series then.
>
Maybe we can introduce the class earlier. Basically we could:

1. Extend the public API.
2. Introduce the class infrastructure (but still block multiple handlers).
3. Introduce the ability to register multiple handlers.

This would allow for relying more on the class infrastructure for things like
device iteration, etc.

Thanks,
Armin Wolf

>>
>> This would allow us to get rid of platform_profile_is_class_device().
>>
>>>           for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>>>               individual |= BIT(i);
>>>           if (!aggregate)
>>> @@ -51,7 +65,7 @@ static unsigned long
>>> platform_profile_get_choices(void)
>>>       return aggregate;
>>>   }
>>>
>>> -static int platform_profile_get_active(enum platform_profile_option
>>> *profile)
>>> +static int platform_profile_get_active(struct device *dev, enum
>>> platform_profile_option *profile)
>>>   {
>>>       struct platform_profile_handler *handler;
>>>       enum platform_profile_option active = PLATFORM_PROFILE_LAST;
>>> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum
>>> platform_profile_option *profile)
>>>
>>>       lockdep_assert_held(&profile_lock);
>>>       list_for_each_entry(handler, &platform_profile_handler_list,
>>> list) {
>>> +        if (platform_profile_is_class_device(dev) && handler->dev
>>> != dev->parent)
>>> +            continue;
>>>           err = handler->profile_get(handler, &val);
>>>           if (err) {
>>>               pr_err("Failed to get profile for handler %s\n",
>>> handler->name);
>>> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum
>>> platform_profile_option *profile)
>>>           if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>>               return -EINVAL;
>>>
>>> +        /*
>>> +         * If the profiles are different for class devices then
>>> this must
>>> +         * show "custom" to legacy sysfs interface
>>> +         */
>>>           if (active != val && active != PLATFORM_PROFILE_LAST) {
>>>               *profile = PLATFORM_PROFILE_CUSTOM;
>>>               return 0;
>>> @@ -90,7 +110,7 @@ static ssize_t
>>> platform_profile_choices_show(struct device *dev,
>>>       int i;
>>>
>>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
>>> -        choices = platform_profile_get_choices();
>>> +        choices = platform_profile_get_choices(dev);
>>>
>>>       for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>>>           if (len == 0)
>>> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct
>>> device *dev,
>>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>>           if (!platform_profile_is_registered())
>>>               return -ENODEV;
>>> -        err = platform_profile_get_active(&profile);
>>> +        err = platform_profile_get_active(dev, &profile);
>>>           if (err)
>>>               return err;
>>>       }
>>> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct
>>> device *dev,
>>>           if (!platform_profile_is_registered())
>>>               return -ENODEV;
>>>
>>> -        /* Check that all handlers support this profile choice */
>>> -        choices = platform_profile_get_choices();
>>> +        /* don't allow setting custom to legacy sysfs interface */
>>> +        if (!platform_profile_is_class_device(dev) &&
>>> +             i == PLATFORM_PROFILE_CUSTOM) {
>>> +            pr_warn("Custom profile not supported for legacy sysfs
>>> interface\n");
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        /* Check that applicable handlers support this profile
>>> choice */
>>> +        choices = platform_profile_get_choices(dev);
>>>           if (!test_bit(i, &choices))
>>>               return -EOPNOTSUPP;
>>>
>>>           list_for_each_entry(handler,
>>> &platform_profile_handler_list, list) {
>>> +            if (platform_profile_is_class_device(dev) &&
>>> +                handler->dev != dev->parent)
>>> +                continue;
>>>               err = handler->profile_set(handler, i);
>>>               if (err) {
>>>                   pr_err("Failed to set profile for handler %s\n",
>>> handler->name);
>>> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct
>>> device *dev,
>>>           }
>>>           if (err) {
>>>               list_for_each_entry_continue_reverse(handler,
>>> &platform_profile_handler_list, list) {
>>> +                if (platform_profile_is_class_device(dev) &&
>>> +                    handler->dev != dev->parent)
>>> +                    continue;
>>>                   if (handler->profile_set(handler,
>>> PLATFORM_PROFILE_BALANCED))
>>>                       pr_err("Failed to revert profile for handler
>>> %s\n",
>>>                              handler->name);
>>> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>>>       int err;
>>>
>>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> -        err = platform_profile_get_active(&profile);
>>> +        err = platform_profile_get_active(NULL, &profile);
>>>           if (err)
>>>               return err;
>>>
>>> -        choices = platform_profile_get_choices();
>>> +        choices = platform_profile_get_choices(NULL);
>>>
>>>           next = find_next_bit_wrap(&choices,
>>>                         PLATFORM_PROFILE_LAST,
>>> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>>
>>>   int platform_profile_register(struct platform_profile_handler *pprof)
>>>   {
>>> +    bool registered;
>>>       int err;
>>>
>>>       /* Sanity check the profile handler */
>>> @@ -250,14 +284,49 @@ int platform_profile_register(struct
>>> platform_profile_handler *pprof)
>>>       if (cur_profile)
>>>           return -EEXIST;
>>>
>>> -    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>> +    registered = platform_profile_is_registered();
>>> +    if (!registered) {
>>> +        /* class for individual handlers */
>>> +        err = class_register(&platform_profile_class);
>>> +        if (err)
>>> +            return err;
>>
>> Why do we need to unregister the class here? From my point of view,
>> having a empty class if no
>> platform profiles are registered is totally fine.
>
> Hmm, OK.
>
>>
>>> +        /* legacy sysfs files */
>>> +        err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>> +        if (err)
>>> +            goto cleanup_class;
>>> +
>>> +    }
>>> +
>>> +    /* create class interface for individual handler */
>>> +    pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0,
>>> 0, GFP_KERNEL);
>>> +    pprof->class_dev = device_create(&platform_profile_class,
>>> pprof- >dev,
>>> +                     MKDEV(0, pprof->minor), NULL,
>>> "platform-profile- %s",
>>> +                     pprof->name);
>>
>> I would suggest that the name of the class devices should not contain
>> the platform profile name,
>> as this would mean that two platform profile handlers cannot have the
>> same name.
>>
>> Maybe using "platform-profile-<minor>" would be a better solution
>> here? The name can instead be
>> read using an additional sysfs property.
>
> Sure makes sense.
>
>>
>> Thanks,
>> Armin Wolf
>>
>>> +    if (IS_ERR(pprof->class_dev)) {
>>> +        err = PTR_ERR(pprof->class_dev);
>>> +        goto cleanup_legacy;
>>> +    }
>>> +    err = sysfs_create_group(&pprof->class_dev->kobj,
>>> &platform_profile_group);
>>>       if (err)
>>> -        return err;
>>> +        goto cleanup_device;
>>> +
>>>       list_add_tail(&pprof->list, &platform_profile_handler_list);
>>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>
>>>       cur_profile = pprof;
>>>       return 0;
>>> +
>>> +cleanup_device:
>>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>>> +
>>> +cleanup_legacy:
>>> +    if (!registered)
>>> +        sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>> +cleanup_class:
>>> +    if (!registered)
>>> +        class_unregister(&platform_profile_class);
>>> +
>>> +    return err;
>>>   }
>>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>>
>>> @@ -270,6 +339,10 @@ int platform_profile_remove(struct
>>> platform_profile_handler *pprof)
>>>       cur_profile = NULL;
>>>
>>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> +
>>> +    sysfs_remove_group(&pprof->class_dev->kobj,
>>> &platform_profile_group);
>>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>>> +
>>>       if (!platform_profile_is_registered())
>>>           sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>>
>>> diff --git a/include/linux/platform_profile.h b/include/linux/
>>> platform_profile.h
>>> index da009c8a402c9..764c4812ef759 100644
>>> --- a/include/linux/platform_profile.h
>>> +++ b/include/linux/platform_profile.h
>>> @@ -30,6 +30,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)];
>>>       struct list_head list;
>>>       int (*profile_get)(struct platform_profile_handler *pprof,
>
>

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.12-rc5 next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-platform-profile-Add-a-name-member-to-handlers/20241031-121650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20241031040952.109057-21-mario.limonciello%40amd.com
patch subject: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers
config: i386-buildonly-randconfig-005-20241101 (https://download.01.org/0day-ci/archive/20241101/202411012227.46a4WcxB-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411012227.46a4WcxB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411012227.46a4WcxB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/acpi/platform_profile.c:303:7: error: call to undeclared function 'MKDEV'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     303 |                                          MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
         |                                          ^
   drivers/acpi/platform_profile.c:344:42: error: call to undeclared function 'MKDEV'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     344 |         device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
         |                                                 ^
   2 errors generated.


vim +/MKDEV +303 drivers/acpi/platform_profile.c

   261	
   262	int platform_profile_register(struct platform_profile_handler *pprof)
   263	{
   264		bool registered;
   265		int err;
   266	
   267		/* Sanity check the profile handler */
   268		if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
   269		    !pprof->profile_set || !pprof->profile_get) {
   270			pr_err("platform_profile: handler is invalid\n");
   271			return -EINVAL;
   272		}
   273		if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
   274			pr_err("platform_profile: handler does not support balanced profile\n");
   275			return -EINVAL;
   276		}
   277		if (!pprof->dev) {
   278			pr_err("platform_profile: handler device is not set\n");
   279			return -EINVAL;
   280		}
   281	
   282		guard(mutex)(&profile_lock);
   283		/* We can only have one active profile */
   284		if (cur_profile)
   285			return -EEXIST;
   286	
   287		registered = platform_profile_is_registered();
   288		if (!registered) {
   289			/* class for individual handlers */
   290			err = class_register(&platform_profile_class);
   291			if (err)
   292				return err;
   293			/* legacy sysfs files */
   294			err = sysfs_create_group(acpi_kobj, &platform_profile_group);
   295			if (err)
   296				goto cleanup_class;
   297	
   298		}
   299	
   300		/* create class interface for individual handler */
   301		pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
   302		pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
 > 303						 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
   304						 pprof->name);
   305		if (IS_ERR(pprof->class_dev)) {
   306			err = PTR_ERR(pprof->class_dev);
   307			goto cleanup_legacy;
   308		}
   309		err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
   310		if (err)
   311			goto cleanup_device;
   312	
   313		list_add_tail(&pprof->list, &platform_profile_handler_list);
   314		sysfs_notify(acpi_kobj, NULL, "platform_profile");
   315	
   316		cur_profile = pprof;
   317		return 0;
   318	
   319	cleanup_device:
   320		device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
   321	
   322	cleanup_legacy:
   323		if (!registered)
   324			sysfs_remove_group(acpi_kobj, &platform_profile_group);
   325	cleanup_class:
   326		if (!registered)
   327			class_unregister(&platform_profile_class);
   328	
   329		return err;
   330	}
   331	EXPORT_SYMBOL_GPL(platform_profile_register);
   332	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.12-rc5 next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-platform-profile-Add-a-name-member-to-handlers/20241031-121650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20241031040952.109057-21-mario.limonciello%40amd.com
patch subject: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers
config: x86_64-buildonly-randconfig-005-20241101 (https://download.01.org/0day-ci/archive/20241101/202411012317.1pQLOspC-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411012317.1pQLOspC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411012317.1pQLOspC-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/acpi/platform_profile.c: In function 'platform_profile_register':
>> drivers/acpi/platform_profile.c:303:42: error: implicit declaration of function 'MKDEV' [-Werror=implicit-function-declaration]
     303 |                                          MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
         |                                          ^~~~~
   cc1: some warnings being treated as errors


vim +/MKDEV +303 drivers/acpi/platform_profile.c

   261	
   262	int platform_profile_register(struct platform_profile_handler *pprof)
   263	{
   264		bool registered;
   265		int err;
   266	
   267		/* Sanity check the profile handler */
   268		if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
   269		    !pprof->profile_set || !pprof->profile_get) {
   270			pr_err("platform_profile: handler is invalid\n");
   271			return -EINVAL;
   272		}
   273		if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
   274			pr_err("platform_profile: handler does not support balanced profile\n");
   275			return -EINVAL;
   276		}
   277		if (!pprof->dev) {
   278			pr_err("platform_profile: handler device is not set\n");
   279			return -EINVAL;
   280		}
   281	
   282		guard(mutex)(&profile_lock);
   283		/* We can only have one active profile */
   284		if (cur_profile)
   285			return -EEXIST;
   286	
   287		registered = platform_profile_is_registered();
   288		if (!registered) {
   289			/* class for individual handlers */
   290			err = class_register(&platform_profile_class);
   291			if (err)
   292				return err;
   293			/* legacy sysfs files */
   294			err = sysfs_create_group(acpi_kobj, &platform_profile_group);
   295			if (err)
   296				goto cleanup_class;
   297	
   298		}
   299	
   300		/* create class interface for individual handler */
   301		pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
   302		pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
 > 303						 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
   304						 pprof->name);
   305		if (IS_ERR(pprof->class_dev)) {
   306			err = PTR_ERR(pprof->class_dev);
   307			goto cleanup_legacy;
   308		}
   309		err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
   310		if (err)
   311			goto cleanup_device;
   312	
   313		list_add_tail(&pprof->list, &platform_profile_handler_list);
   314		sysfs_notify(acpi_kobj, NULL, "platform_profile");
   315	
   316		cur_profile = pprof;
   317		return 0;
   318	
   319	cleanup_device:
   320		device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
   321	
   322	cleanup_legacy:
   323		if (!registered)
   324			sysfs_remove_group(acpi_kobj, &platform_profile_group);
   325	cleanup_class:
   326		if (!registered)
   327			class_unregister(&platform_profile_class);
   328	
   329		return err;
   330	}
   331	EXPORT_SYMBOL_GPL(platform_profile_register);
   332	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Mark Pearson]

On Thu, Oct 31, 2024, at 4:55 PM, Armin Wolf wrote:
> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
>
>> The "platform_profile" class device has the exact same semantics as the
>> platform profile files in /sys/firmware/acpi/ but it reflects values only
>> present for a single platform profile handler.
>>
>> The expectation is that legacy userspace can change the profile for all
>> handlers in /sys/firmware/acpi/platform_profile and can change it for
>> individual handlers by /sys/class/platform_profile/*.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
>>   include/linux/platform_profile.h |  2 +
>>   2 files changed, 85 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index 9b681884ae324..1cc8182930dde 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>>   };
>>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>
>> +static DEFINE_IDR(platform_profile_minor_idr);
>> +
>> +static const struct class platform_profile_class = {
>> +	.name = "platform-profile",
>> +};
>> +
>>   static bool platform_profile_is_registered(void)
>>   {
>>   	lockdep_assert_held(&profile_lock);
>>   	return !list_empty(&platform_profile_handler_list);
>>   }
>>
>> -static unsigned long platform_profile_get_choices(void)
>> +static bool platform_profile_is_class_device(struct device *dev)
>> +{
>> +	return dev && dev->class == &platform_profile_class;
>> +}
>> +
>> +static unsigned long platform_profile_get_choices(struct device *dev)
>>   {
>>   	struct platform_profile_handler *handler;
>>   	unsigned long aggregate = 0;
>> @@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
>>   	list_for_each_entry(handler, &platform_profile_handler_list, list) {
>>   		unsigned long individual = 0;
>>
>> +		/* if called from a class attribute then only match that one */
>> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
>> +			continue;
>
> I do not like how the sysfs attributes for the platform-profile class 
> are handled:
>
> 1. We should use .dev_groups instead of manually registering the sysfs 
> attributes.
> 2. Can we name the sysfs attributes for the class a bit differently 
> ("profile_choices" and "profile")
>     and use separate store/show functions for those?
> 3. Why do we still need platform_profile_handler_list?
>
> This would allow us to get rid of platform_profile_is_class_device().
>
>>   		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>>   			individual |= BIT(i);
>>   		if (!aggregate)
>> @@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
>>   	return aggregate;
>>   }
>>
>> -static int platform_profile_get_active(enum platform_profile_option *profile)
>> +static int platform_profile_get_active(struct device *dev, enum platform_profile_option *profile)
>>   {
>>   	struct platform_profile_handler *handler;
>>   	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
>> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>>
>>   	lockdep_assert_held(&profile_lock);
>>   	list_for_each_entry(handler, &platform_profile_handler_list, list) {
>> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
>> +			continue;
>>   		err = handler->profile_get(handler, &val);
>>   		if (err) {
>>   			pr_err("Failed to get profile for handler %s\n", handler->name);
>> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>>   		if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>   			return -EINVAL;
>>
>> +		/*
>> +		 * If the profiles are different for class devices then this must
>> +		 * show "custom" to legacy sysfs interface
>> +		 */
>>   		if (active != val && active != PLATFORM_PROFILE_LAST) {
>>   			*profile = PLATFORM_PROFILE_CUSTOM;
>>   			return 0;
>> @@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>>   	int i;
>>
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
>> -		choices = platform_profile_get_choices();
>> +		choices = platform_profile_get_choices(dev);
>>
>>   	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>>   		if (len == 0)
>> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device *dev,
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>   		if (!platform_profile_is_registered())
>>   			return -ENODEV;
>> -		err = platform_profile_get_active(&profile);
>> +		err = platform_profile_get_active(dev, &profile);
>>   		if (err)
>>   			return err;
>>   	}
>> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct device *dev,
>>   		if (!platform_profile_is_registered())
>>   			return -ENODEV;
>>
>> -		/* Check that all handlers support this profile choice */
>> -		choices = platform_profile_get_choices();
>> +		/* don't allow setting custom to legacy sysfs interface */
>> +		if (!platform_profile_is_class_device(dev) &&
>> +		     i == PLATFORM_PROFILE_CUSTOM) {
>> +			pr_warn("Custom profile not supported for legacy sysfs interface\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Check that applicable handlers support this profile choice */
>> +		choices = platform_profile_get_choices(dev);
>>   		if (!test_bit(i, &choices))
>>   			return -EOPNOTSUPP;
>>
>>   		list_for_each_entry(handler, &platform_profile_handler_list, list) {
>> +			if (platform_profile_is_class_device(dev) &&
>> +			    handler->dev != dev->parent)
>> +				continue;
>>   			err = handler->profile_set(handler, i);
>>   			if (err) {
>>   				pr_err("Failed to set profile for handler %s\n", handler->name);
>> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct device *dev,
>>   		}
>>   		if (err) {
>>   			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
>> +				if (platform_profile_is_class_device(dev) &&
>> +				    handler->dev != dev->parent)
>> +					continue;
>>   				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
>>   					pr_err("Failed to revert profile for handler %s\n",
>>   					       handler->name);
>> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>>   	int err;
>>
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> -		err = platform_profile_get_active(&profile);
>> +		err = platform_profile_get_active(NULL, &profile);
>>   		if (err)
>>   			return err;
>>
>> -		choices = platform_profile_get_choices();
>> +		choices = platform_profile_get_choices(NULL);
>>
>>   		next = find_next_bit_wrap(&choices,
>>   					  PLATFORM_PROFILE_LAST,
>> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>
>>   int platform_profile_register(struct platform_profile_handler *pprof)
>>   {
>> +	bool registered;
>>   	int err;
>>
>>   	/* Sanity check the profile handler */
>> @@ -250,14 +284,49 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>>   	if (cur_profile)
>>   		return -EEXIST;
>>
>> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +	registered = platform_profile_is_registered();
>> +	if (!registered) {
>> +		/* class for individual handlers */
>> +		err = class_register(&platform_profile_class);
>> +		if (err)
>> +			return err;
>
> Why do we need to unregister the class here? From my point of view, 
> having a empty class if no
> platform profiles are registered is totally fine.
>
>> +		/* legacy sysfs files */
>> +		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +		if (err)
>> +			goto cleanup_class;
>> +
>> +	}
>> +
>> +	/* create class interface for individual handler */
>> +	pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
>> +	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
>> +					 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
>> +					 pprof->name);
>
> I would suggest that the name of the class devices should not contain 
> the platform profile name,
> as this would mean that two platform profile handlers cannot have the 
> same name.
>
> Maybe using "platform-profile-<minor>" would be a better solution here? 
> The name can instead be
> read using an additional sysfs property.
>
> Thanks,
> Armin Wolf
>

I'm still getting my head around this patch (it's late and I'm a bit brain-dead this evening) - but isn't it a good thing to force the different profile handlers to have different names?

I like the platform-profile-<name> approach, it makes it simpler for users to know if they're changing a platform vendors profile, a CPU vendors profile, or something else. If profiles have the same name it would become quite confusing.

Mark

>> +	if (IS_ERR(pprof->class_dev)) {
>> +		err = PTR_ERR(pprof->class_dev);
>> +		goto cleanup_legacy;
>> +	}
>> +	err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
>>   	if (err)
>> -		return err;
>> +		goto cleanup_device;
>> +
>>   	list_add_tail(&pprof->list, &platform_profile_handler_list);
>>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
>>   	cur_profile = pprof;
>>   	return 0;
>> +
>> +cleanup_device:
>> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>> +cleanup_legacy:
>> +	if (!registered)
>> +		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +cleanup_class:
>> +	if (!registered)
>> +		class_unregister(&platform_profile_class);
>> +
>> +	return err;
>>   }
>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>
>> @@ -270,6 +339,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>>   	cur_profile = NULL;
>>
>>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +
>> +	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
>> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>>   	if (!platform_profile_is_registered())
>>   		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>
>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>> index da009c8a402c9..764c4812ef759 100644
>> --- a/include/linux/platform_profile.h
>> +++ b/include/linux/platform_profile.h
>> @@ -30,6 +30,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)];
>>   	struct list_head list;
>>   	int (*profile_get)(struct platform_profile_handler *pprof,

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

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> 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.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c                     | 5 +++++
>  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/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, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c 
> b/drivers/acpi/platform_profile.c
> index d2f7fd7743a13..5d9f3f7ba71c5 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -179,6 +179,11 @@ int platform_profile_register(struct 
> platform_profile_handler *pprof)
>  {
>  	int err;
> 
> +	if (!pprof->dev) {
> +		pr_err("platform_profile: handler device is not set\n");
> +		return -EINVAL;
> +	}
> +
>  	mutex_lock(&profile_lock);
>  	/* We can only have one active profile */
>  	if (cur_profile) {
> diff --git a/drivers/platform/surface/surface_platform_profile.c 
> b/drivers/platform/surface/surface_platform_profile.c
> index 9d3e3f9458186..b73cfdd920c66 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 13a97afe0112d..a5caa529351ea 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1878,12 +1878,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 =
> @@ -2536,7 +2537,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 6177fbee60573..1a8c29aafe892 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3921,6 +3921,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/dell-pc.c 
> b/drivers/platform/x86/dell/dell-pc.c
> index b145fedb6b710..730f97aab70cd 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -260,6 +260,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 10a853b6b0514..1b6677e176769 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 6c72d1b6a2aff..feaf98819dc82 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

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

Re: [PATCH v3 06/22] ACPI: platform_profile: Move sanity check out of the mutex

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> The sanity check that the platform handler had choices set doesn't
> need the mutex taken.  Move it to earlier in the registration.
>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c 
> b/drivers/acpi/platform_profile.c
> index d0198d2ccb551..f2f2274e4d83e 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -180,6 +180,12 @@ int platform_profile_register(struct 
> platform_profile_handler *pprof)
>  {
>  	int err;
> 
> +	/* Sanity check the profile handler */
> +	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
> +	    !pprof->profile_set || !pprof->profile_get) {
> +		pr_err("platform_profile: handler is invalid\n");
> +		return -EINVAL;
> +	}
>  	if (!pprof->dev) {
>  		pr_err("platform_profile: handler device is not set\n");
>  		return -EINVAL;
> @@ -192,13 +198,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

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

Re: [PATCH v3 11/22] ACPI: platform_profile: Use `scoped_cond_guard` for platform_profile_cycle()

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> Migrate away from using an interruptible mutex to scoped_cond_guard.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 39 +++++++++++++--------------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 63a5f5ac33898..2d971dba2d917 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -124,36 +124,27 @@ 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");
> -
> -	return err;
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(platform_profile_cycle);
> 
> -- 
> 2.43.0

For patches 8 to 11 - Looks good to me (guards are new to me - I had to go read up on them. Very cool and a nice clean-up)

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> As multiple platform profile handlers may come and go, send a notification
> to userspace each time that a platform profile handler is registered or
> unregistered.
>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/platform_profile.c 
> b/drivers/acpi/platform_profile.c
> index 57c66d7dbf827..7bd32f1e8d834 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -182,6 +182,7 @@ int platform_profile_register(struct 
> platform_profile_handler *pprof)
>  	if (err)
>  		return err;
>  	list_add_tail(&pprof->list, &platform_profile_handler_list);
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> 
>  	cur_profile = pprof;
>  	return 0;
> @@ -195,6 +196,8 @@ int platform_profile_remove(struct 
> platform_profile_handler *pprof)
>  	list_del(&pprof->list);
> 
>  	cur_profile = NULL;
> +
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>  	if (!platform_profile_is_registered())
>  		sysfs_remove_group(acpi_kobj, &platform_profile_group);
> 
> -- 
> 2.43.0

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> If multiple platform profile handlers have been registered, don't allow
> switching to profiles unique to only one handler.
>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 38 +++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 7bd32f1e8d834..90cbc0de4d5bc 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -29,23 +29,43 @@ static bool platform_profile_is_registered(void)
>  	return !list_empty(&platform_profile_handler_list);
>  }
> 
> +static unsigned long platform_profile_get_choices(void)
> +{
> +	struct platform_profile_handler *handler;
> +	unsigned long aggregate = 0;
> +	int i;
> +
> +	lockdep_assert_held(&profile_lock);
> +	list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +		unsigned long individual = 0;
> +
> +		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
> +			individual |= BIT(i);
> +		if (!aggregate)
> +			aggregate = individual;
> +		else
> +			aggregate &= individual;
> +	}
> +
> +	return aggregate;
> +}
> +

I realise this is very unlikely but isn't it possible that the number of profiles could overflow unsigned long number of bits? As handler->choices is an array of them. It should loop through BITS_TO_LONGS(PLATFORM_PROFILE_LAST) cycles.

Also wondered if this could be simplified with setting aggregate = ~0 and then & it with each handler->choices to avoid having to scan individual bits?

>  static ssize_t platform_profile_choices_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
>  {
> +	unsigned long choices;
>  	int len = 0;
>  	int i;
> 
> -	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
> +		choices = platform_profile_get_choices();
> 
> -		for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) {
> -			if (len == 0)
> -				len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
> -			else
> -				len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
> -		}
> +	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");
> 
> -- 
> 2.43.0

This links in to the later patches - but I was wondering if at profile registration/unregistration if building a local choices selection would simplify things.
Then instead of needing platform_profile_get_choices, you can just use your local choices selection to make decisions on what to show, or cycle to - instead of having to cycle thru the profiles and bits every time.

Not 100% sure how well it would work out - but might be simpler and faster?

Mark

Re: [PATCH v3 17/22] ACPI: platform_profile: Add concept of a "custom" profile

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> 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.
>
> 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 c2bb325ba531c..3128bd16615b6 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 6aad98f4abaf4..da009c8a402c9 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

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> If for any reason multiple profile handlers don't agree on the profile
> report the custom profile to userspace.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 39 +++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 3128bd16615b6..5baac1f9a9c0e 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -51,6 +51,36 @@ static unsigned long platform_profile_get_choices(void)
>  	return aggregate;
>  }
> 
> +static int platform_profile_get_active(enum platform_profile_option *profile)
> +{
> +	struct platform_profile_handler *handler;
> +	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
> +	enum platform_profile_option val;
> +	int err;
> +
> +	lockdep_assert_held(&profile_lock);
> +	list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +		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;
> +
> +		if (active != val && active != PLATFORM_PROFILE_LAST) {
> +			*profile = PLATFORM_PROFILE_CUSTOM;
> +			return 0;
> +		}
> +		active = val;
> +	}
> +
> +	*profile = active;
> +
> +	return 0;
> +}
> +
>  static ssize_t platform_profile_choices_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
> @@ -81,18 +111,13 @@ static ssize_t platform_profile_show(struct device *dev,
>  	int err;
> 
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> +		if (!platform_profile_is_registered())
>  			return -ENODEV;
> -
> -		err = cur_profile->profile_get(cur_profile, &profile);
> +		err = platform_profile_get_active(&profile);
>  		if (err)
>  			return err;
>  	}
> 
> -	/* Check that profile is valid index */
> -	if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> -		return -EIO;
> -
>  	return sysfs_emit(buf, "%s\n", profile_names[profile]);
>  }
> 
> -- 
> 2.43.0

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> 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.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 35 ++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 5baac1f9a9c0e..9b681884ae324 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -187,30 +187,41 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
> 
>  int platform_profile_cycle(void)
>  {
> +	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
> +	struct platform_profile_handler *handler;
>  	enum platform_profile_option profile;
> -	enum platform_profile_option next;
> +	unsigned long choices;
>  	int err;
> 
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> -		if (!cur_profile)
> -			return -ENODEV;
> -
> -		err = cur_profile->profile_get(cur_profile, &profile);
> +		err = platform_profile_get_active(&profile);
>  		if (err)
>  			return err;
> 
> -		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
> -					  profile + 1);
> +		choices = platform_profile_get_choices();
> 
> -		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
> -			return -EINVAL;
> +		next = find_next_bit_wrap(&choices,
> +					  PLATFORM_PROFILE_LAST,
> +					  profile + 1);
> 
> -		err = cur_profile->profile_set(cur_profile, next);
> -		if (err)
> -			return err;
> +		list_for_each_entry(handler, &platform_profile_handler_list, list) {
> +			err = handler->profile_set(handler, next);
> +			if (err) {
> +				pr_err("Failed to set profile for handler %s\n", handler->name);

Suggest highlighting which profile failed (profile_names[next]).

> +				break;
> +			}
> +		}
> +		if (err) {
> +			list_for_each_entry_continue_reverse(handler, 
> &platform_profile_handler_list, list) {
> +				err = handler->profile_set(handler, PLATFORM_PROFILE_BALANCED);
> +				if (err)
> +					pr_err("Failed to revert profile for handler %s\n", 
> handler->name);

Suggest 'Failed to revert to balanced profiile for ...'

> +			}
> +		}
>  	}
> 
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(platform_profile_cycle);
> -- 
> 2.43.0

My note from patch 15 applies here with having choices determined during registration/unregistration.

Mark

Re: [PATCH v3 21/22] ACPI: platform_profile: Allow multiple handlers

[Mark Pearson]

On Thu, Oct 31, 2024, at 12:09 AM, Mario Limonciello wrote:
> Multiple drivers may attempt to register platform profile handlers,
> but only one may be registered and the behavior is non-deterministic
> for which one wins.  It's mostly controlled by probing order.
>
> This can be problematic if one driver changes CPU settings and another
> driver notifies the EC for changing fan curves.
>
> Modify the ACPI platform profile handler to let multiple drivers
> register platform profile handlers and abstract this detail from userspace.
>
> To avoid undefined behaviors only offer profiles that are commonly
> advertised across multiple handlers.
>
> If any problems occur when changing profiles for any driver, then revert
> back to the balanced profile, which is now required.
>
> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/platform_profile.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 1cc8182930dde..a845524a248b9 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -9,7 +9,6 @@
>  #include <linux/platform_profile.h>
>  #include <linux/sysfs.h>
> 
> -static struct platform_profile_handler *cur_profile;
>  static LIST_HEAD(platform_profile_handler_list);
>  static DEFINE_MUTEX(profile_lock);
> 
> @@ -212,7 +211,8 @@ static const struct attribute_group 
> platform_profile_group = {
> 
>  void platform_profile_notify(void)
>  {
> -	if (!cur_profile)
> +	guard(mutex)(&profile_lock);
> +	if (!platform_profile_is_registered())
>  		return;
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>  }
> @@ -280,9 +280,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;
> 
>  	registered = platform_profile_is_registered();
>  	if (!registered) {
> @@ -313,7 +310,6 @@ int platform_profile_register(struct 
> platform_profile_handler *pprof)
>  	list_add_tail(&pprof->list, &platform_profile_handler_list);
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> 
> -	cur_profile = pprof;
>  	return 0;
> 
>  cleanup_device:
> @@ -336,8 +332,6 @@ int platform_profile_remove(struct 
> platform_profile_handler *pprof)
> 
>  	list_del(&pprof->list);
> 
> -	cur_profile = NULL;
> -
>  	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> 
>  	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
> -- 
> 2.43.0

Looks good to me
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Maximilian Luz]

On 10/31/24 5:09 AM, Mario Limonciello wrote:
> 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.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c                     | 5 +++++
>   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/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, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index d2f7fd7743a13..5d9f3f7ba71c5 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -179,6 +179,11 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   {
>   	int err;
>   
> +	if (!pprof->dev) {
> +		pr_err("platform_profile: handler device is not set\n");
> +		return -EINVAL;
> +	}
> +
>   	mutex_lock(&profile_lock);
>   	/* We can only have one active profile */
>   	if (cur_profile) {
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index 9d3e3f9458186..b73cfdd920c66 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 13a97afe0112d..a5caa529351ea 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1878,12 +1878,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 =
> @@ -2536,7 +2537,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 6177fbee60573..1a8c29aafe892 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3921,6 +3921,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/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
> index b145fedb6b710..730f97aab70cd 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -260,6 +260,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 10a853b6b0514..1b6677e176769 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 6c72d1b6a2aff..feaf98819dc82 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);

Looks good to me, thanks!

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

Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Armin Wolf]

Am 02.11.24 um 03:13 schrieb Mark Pearson:

>
> On Thu, Oct 31, 2024, at 4:55 PM, Armin Wolf wrote:
>> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
>>
>>> The "platform_profile" class device has the exact same semantics as the
>>> platform profile files in /sys/firmware/acpi/ but it reflects values only
>>> present for a single platform profile handler.
>>>
>>> The expectation is that legacy userspace can change the profile for all
>>> handlers in /sys/firmware/acpi/platform_profile and can change it for
>>> individual handlers by /sys/class/platform_profile/*.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>    drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
>>>    include/linux/platform_profile.h |  2 +
>>>    2 files changed, 85 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>>> index 9b681884ae324..1cc8182930dde 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>>>    };
>>>    static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>>
>>> +static DEFINE_IDR(platform_profile_minor_idr);
>>> +
>>> +static const struct class platform_profile_class = {
>>> +	.name = "platform-profile",
>>> +};
>>> +
>>>    static bool platform_profile_is_registered(void)
>>>    {
>>>    	lockdep_assert_held(&profile_lock);
>>>    	return !list_empty(&platform_profile_handler_list);
>>>    }
>>>
>>> -static unsigned long platform_profile_get_choices(void)
>>> +static bool platform_profile_is_class_device(struct device *dev)
>>> +{
>>> +	return dev && dev->class == &platform_profile_class;
>>> +}
>>> +
>>> +static unsigned long platform_profile_get_choices(struct device *dev)
>>>    {
>>>    	struct platform_profile_handler *handler;
>>>    	unsigned long aggregate = 0;
>>> @@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
>>>    	list_for_each_entry(handler, &platform_profile_handler_list, list) {
>>>    		unsigned long individual = 0;
>>>
>>> +		/* if called from a class attribute then only match that one */
>>> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
>>> +			continue;
>> I do not like how the sysfs attributes for the platform-profile class
>> are handled:
>>
>> 1. We should use .dev_groups instead of manually registering the sysfs
>> attributes.
>> 2. Can we name the sysfs attributes for the class a bit differently
>> ("profile_choices" and "profile")
>>      and use separate store/show functions for those?
>> 3. Why do we still need platform_profile_handler_list?
>>
>> This would allow us to get rid of platform_profile_is_class_device().
>>
>>>    		for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>>>    			individual |= BIT(i);
>>>    		if (!aggregate)
>>> @@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
>>>    	return aggregate;
>>>    }
>>>
>>> -static int platform_profile_get_active(enum platform_profile_option *profile)
>>> +static int platform_profile_get_active(struct device *dev, enum platform_profile_option *profile)
>>>    {
>>>    	struct platform_profile_handler *handler;
>>>    	enum platform_profile_option active = PLATFORM_PROFILE_LAST;
>>> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>>>
>>>    	lockdep_assert_held(&profile_lock);
>>>    	list_for_each_entry(handler, &platform_profile_handler_list, list) {
>>> +		if (platform_profile_is_class_device(dev) && handler->dev != dev->parent)
>>> +			continue;
>>>    		err = handler->profile_get(handler, &val);
>>>    		if (err) {
>>>    			pr_err("Failed to get profile for handler %s\n", handler->name);
>>> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum platform_profile_option *profile)
>>>    		if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>>    			return -EINVAL;
>>>
>>> +		/*
>>> +		 * If the profiles are different for class devices then this must
>>> +		 * show "custom" to legacy sysfs interface
>>> +		 */
>>>    		if (active != val && active != PLATFORM_PROFILE_LAST) {
>>>    			*profile = PLATFORM_PROFILE_CUSTOM;
>>>    			return 0;
>>> @@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>>>    	int i;
>>>
>>>    	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
>>> -		choices = platform_profile_get_choices();
>>> +		choices = platform_profile_get_choices(dev);
>>>
>>>    	for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>>>    		if (len == 0)
>>> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device *dev,
>>>    	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>>    		if (!platform_profile_is_registered())
>>>    			return -ENODEV;
>>> -		err = platform_profile_get_active(&profile);
>>> +		err = platform_profile_get_active(dev, &profile);
>>>    		if (err)
>>>    			return err;
>>>    	}
>>> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct device *dev,
>>>    		if (!platform_profile_is_registered())
>>>    			return -ENODEV;
>>>
>>> -		/* Check that all handlers support this profile choice */
>>> -		choices = platform_profile_get_choices();
>>> +		/* don't allow setting custom to legacy sysfs interface */
>>> +		if (!platform_profile_is_class_device(dev) &&
>>> +		     i == PLATFORM_PROFILE_CUSTOM) {
>>> +			pr_warn("Custom profile not supported for legacy sysfs interface\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		/* Check that applicable handlers support this profile choice */
>>> +		choices = platform_profile_get_choices(dev);
>>>    		if (!test_bit(i, &choices))
>>>    			return -EOPNOTSUPP;
>>>
>>>    		list_for_each_entry(handler, &platform_profile_handler_list, list) {
>>> +			if (platform_profile_is_class_device(dev) &&
>>> +			    handler->dev != dev->parent)
>>> +				continue;
>>>    			err = handler->profile_set(handler, i);
>>>    			if (err) {
>>>    				pr_err("Failed to set profile for handler %s\n", handler->name);
>>> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct device *dev,
>>>    		}
>>>    		if (err) {
>>>    			list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) {
>>> +				if (platform_profile_is_class_device(dev) &&
>>> +				    handler->dev != dev->parent)
>>> +					continue;
>>>    				if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED))
>>>    					pr_err("Failed to revert profile for handler %s\n",
>>>    					       handler->name);
>>> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>>>    	int err;
>>>
>>>    	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>> -		err = platform_profile_get_active(&profile);
>>> +		err = platform_profile_get_active(NULL, &profile);
>>>    		if (err)
>>>    			return err;
>>>
>>> -		choices = platform_profile_get_choices();
>>> +		choices = platform_profile_get_choices(NULL);
>>>
>>>    		next = find_next_bit_wrap(&choices,
>>>    					  PLATFORM_PROFILE_LAST,
>>> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>>
>>>    int platform_profile_register(struct platform_profile_handler *pprof)
>>>    {
>>> +	bool registered;
>>>    	int err;
>>>
>>>    	/* Sanity check the profile handler */
>>> @@ -250,14 +284,49 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>>>    	if (cur_profile)
>>>    		return -EEXIST;
>>>
>>> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>> +	registered = platform_profile_is_registered();
>>> +	if (!registered) {
>>> +		/* class for individual handlers */
>>> +		err = class_register(&platform_profile_class);
>>> +		if (err)
>>> +			return err;
>> Why do we need to unregister the class here? From my point of view,
>> having a empty class if no
>> platform profiles are registered is totally fine.
>>
>>> +		/* legacy sysfs files */
>>> +		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>> +		if (err)
>>> +			goto cleanup_class;
>>> +
>>> +	}
>>> +
>>> +	/* create class interface for individual handler */
>>> +	pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
>>> +	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
>>> +					 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
>>> +					 pprof->name);
>> I would suggest that the name of the class devices should not contain
>> the platform profile name,
>> as this would mean that two platform profile handlers cannot have the
>> same name.
>>
>> Maybe using "platform-profile-<minor>" would be a better solution here?
>> The name can instead be
>> read using an additional sysfs property.
>>
>> Thanks,
>> Armin Wolf
>>
> I'm still getting my head around this patch (it's late and I'm a bit brain-dead this evening) - but isn't it a good thing to force the different profile handlers to have different names?
>
> I like the platform-profile-<name> approach, it makes it simpler for users to know if they're changing a platform vendors profile, a CPU vendors profile, or something else. If profiles have the same name it would become quite confusing.
>
> Mark
>
I agree the different handlers should have different names, but including the name inside the device name will:

- make users depend on the device name, which usually causes problems later
- cause a WARN() (if i remember correctly) should two handlers accidentally have the same name

Especially the last point can happen sooner than you think, for example if the same driver is instantiated twice.

Thanks,
Armin Wolf

>>> +	if (IS_ERR(pprof->class_dev)) {
>>> +		err = PTR_ERR(pprof->class_dev);
>>> +		goto cleanup_legacy;
>>> +	}
>>> +	err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
>>>    	if (err)
>>> -		return err;
>>> +		goto cleanup_device;
>>> +
>>>    	list_add_tail(&pprof->list, &platform_profile_handler_list);
>>>    	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>
>>>    	cur_profile = pprof;
>>>    	return 0;
>>> +
>>> +cleanup_device:
>>> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>>> +
>>> +cleanup_legacy:
>>> +	if (!registered)
>>> +		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>> +cleanup_class:
>>> +	if (!registered)
>>> +		class_unregister(&platform_profile_class);
>>> +
>>> +	return err;
>>>    }
>>>    EXPORT_SYMBOL_GPL(platform_profile_register);
>>>
>>> @@ -270,6 +339,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>>>    	cur_profile = NULL;
>>>
>>>    	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> +
>>> +	sysfs_remove_group(&pprof->class_dev->kobj, &platform_profile_group);
>>> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>>> +
>>>    	if (!platform_profile_is_registered())
>>>    		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>>
>>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>>> index da009c8a402c9..764c4812ef759 100644
>>> --- a/include/linux/platform_profile.h
>>> +++ b/include/linux/platform_profile.h
>>> @@ -30,6 +30,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)];
>>>    	struct list_head list;
>>>    	int (*profile_get)(struct platform_profile_handler *pprof,