[PATCH v4 0/3] Let other drivers react to platform profile changes

[PATCH v4 0/3] Let other drivers react to platform profile changes

[Mario Limonciello]

Currently only one driver can register as a handler for a platform profile.

This limitation means that if multiple drivers want to react to platform
profile changes that they will need to directly interact with individual
drivers.

Instead introduce a notification flow that drivers can register for event
changes.  The idea is that any driver that wants to can:
1. read the current profile
2. set up initial values
3. register for changes
4. react to changes

Changes from v3->v4:
 * Simplify platform_profile_notify
   - No need to check cur_profile as platform_profile_get does it
   - No need to track ret as it's unused
Changes from v2->v3:
 * Add patches to avoid collisions in hp-wmi and asus-wmi symbols

Mario Limonciello (3):
  platform/x86: hp-wmi: rename platform_profile_* function symbols
  platform/x86: asus-wmi: rename platform_profile_* function symbols
  ACPI: platform_profile: Add support for notification chains

 drivers/acpi/platform_profile.c  | 46 +++++++++++++++++++++++++++-----
 drivers/platform/x86/asus-wmi.c  | 12 ++++-----
 drivers/platform/x86/hp-wmi.c    | 12 ++++-----
 include/linux/platform_profile.h | 10 +++++++
 4 files changed, 61 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] platform/x86: hp-wmi: rename platform_profile_* function symbols

[Mario Limonciello]

An upcoming change to platform profiles will export `platform_profile_get`
as a symbol that can be used by other drivers. Avoid the collision.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/hp-wmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 8e31ffadf894..48a46466f086 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -1061,8 +1061,8 @@ static int thermal_profile_set(int thermal_profile)
 							   sizeof(thermal_profile), 0);
 }
 
-static int platform_profile_get(struct platform_profile_handler *pprof,
-				enum platform_profile_option *profile)
+static int hp_wmi_platform_profile_get(struct platform_profile_handler *pprof,
+					enum platform_profile_option *profile)
 {
 	int tp;
 
@@ -1087,8 +1087,8 @@ static int platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
-static int platform_profile_set(struct platform_profile_handler *pprof,
-				enum platform_profile_option profile)
+static int hp_wmi_platform_profile_set(struct platform_profile_handler *pprof,
+					enum platform_profile_option profile)
 {
 	int err, tp;
 
@@ -1147,8 +1147,8 @@ static int thermal_profile_setup(void)
 		if (err)
 			return err;
 
-		platform_profile_handler.profile_get = platform_profile_get;
-		platform_profile_handler.profile_set = platform_profile_set;
+		platform_profile_handler.profile_get = hp_wmi_platform_profile_get;
+		platform_profile_handler.profile_set = hp_wmi_platform_profile_set;
 	}
 
 	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
-- 
2.25.1

Re: [PATCH v4 1/3] platform/x86: hp-wmi: rename platform_profile_* function symbols

[Hans de Goede]

Hi,

On 10/26/21 21:08, Mario Limonciello wrote:
> An upcoming change to platform profiles will export `platform_profile_get`
> as a symbol that can be used by other drivers. Avoid the collision.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/hp-wmi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 8e31ffadf894..48a46466f086 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -1061,8 +1061,8 @@ static int thermal_profile_set(int thermal_profile)
>  							   sizeof(thermal_profile), 0);
>  }
>  
> -static int platform_profile_get(struct platform_profile_handler *pprof,
> -				enum platform_profile_option *profile)
> +static int hp_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> +					enum platform_profile_option *profile)
>  {
>  	int tp;
>  
> @@ -1087,8 +1087,8 @@ static int platform_profile_get(struct platform_profile_handler *pprof,
>  	return 0;
>  }
>  
> -static int platform_profile_set(struct platform_profile_handler *pprof,
> -				enum platform_profile_option profile)
> +static int hp_wmi_platform_profile_set(struct platform_profile_handler *pprof,
> +					enum platform_profile_option profile)
>  {
>  	int err, tp;
>  
> @@ -1147,8 +1147,8 @@ static int thermal_profile_setup(void)
>  		if (err)
>  			return err;
>  
> -		platform_profile_handler.profile_get = platform_profile_get;
> -		platform_profile_handler.profile_set = platform_profile_set;
> +		platform_profile_handler.profile_get = hp_wmi_platform_profile_get;
> +		platform_profile_handler.profile_set = hp_wmi_platform_profile_set;
>  	}
>  
>  	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
> 

[PATCH v4 2/3] platform/x86: asus-wmi: rename platform_profile_* function symbols

[Mario Limonciello]

An upcoming change to platform profiles will export `platform_profile_get`
as a symbol that can be used by other drivers. Avoid the collision.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/asus-wmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e14fb5fa7324..8f067ac4e952 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -2169,8 +2169,8 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 static DEVICE_ATTR_RW(throttle_thermal_policy);
 
 /* Platform profile ***********************************************************/
-static int platform_profile_get(struct platform_profile_handler *pprof,
-				enum platform_profile_option *profile)
+static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
+					enum platform_profile_option *profile)
 {
 	struct asus_wmi *asus;
 	int tp;
@@ -2196,8 +2196,8 @@ static int platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
-static int platform_profile_set(struct platform_profile_handler *pprof,
-				enum platform_profile_option profile)
+static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
+					enum platform_profile_option profile)
 {
 	struct asus_wmi *asus;
 	int tp;
@@ -2236,8 +2236,8 @@ 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.profile_get = platform_profile_get;
-	asus->platform_profile_handler.profile_set = platform_profile_set;
+	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
+	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
 
 	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
 	set_bit(PLATFORM_PROFILE_BALANCED,
-- 
2.25.1

Re: [PATCH v4 2/3] platform/x86: asus-wmi: rename platform_profile_* function symbols

[Hans de Goede]

Hi,

On 10/26/21 21:08, Mario Limonciello wrote:
> An upcoming change to platform profiles will export `platform_profile_get`
> as a symbol that can be used by other drivers. Avoid the collision.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
>  drivers/platform/x86/asus-wmi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index e14fb5fa7324..8f067ac4e952 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -2169,8 +2169,8 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>  static DEVICE_ATTR_RW(throttle_thermal_policy);
>  
>  /* Platform profile ***********************************************************/
> -static int platform_profile_get(struct platform_profile_handler *pprof,
> -				enum platform_profile_option *profile)
> +static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> +					enum platform_profile_option *profile)
>  {
>  	struct asus_wmi *asus;
>  	int tp;
> @@ -2196,8 +2196,8 @@ static int platform_profile_get(struct platform_profile_handler *pprof,
>  	return 0;
>  }
>  
> -static int platform_profile_set(struct platform_profile_handler *pprof,
> -				enum platform_profile_option profile)
> +static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
> +					enum platform_profile_option profile)
>  {
>  	struct asus_wmi *asus;
>  	int tp;
> @@ -2236,8 +2236,8 @@ 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.profile_get = platform_profile_get;
> -	asus->platform_profile_handler.profile_set = platform_profile_set;
> +	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
> +	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
>  
>  	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
>  	set_bit(PLATFORM_PROFILE_BALANCED,
> 

[PATCH v4 3/3] ACPI: platform_profile: Add support for notification chains

[Mario Limonciello]

Allow other drivers to initialize relative to current active
profile and react to platform profile changes.

Drivers wishing to utilize this should register for notification
at module load and unregister when unloading.

Notifications will come in the from a notifier call.

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

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d418462ab791..a2ea36d2b329 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -21,6 +21,24 @@ static const char * const profile_names[] = {
 	[PLATFORM_PROFILE_PERFORMANCE] = "performance",
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
+static BLOCKING_NOTIFIER_HEAD(platform_profile_chain_head);
+
+int platform_profile_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&platform_profile_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(platform_profile_register_notifier);
+
+int platform_profile_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&platform_profile_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(platform_profile_unregister_notifier);
+
+static void platform_profile_call_notifier(unsigned long action, void *data)
+{
+	blocking_notifier_call_chain(&platform_profile_chain_head, action, data);
+}
 
 static ssize_t platform_profile_choices_show(struct device *dev,
 					struct device_attribute *attr,
@@ -49,11 +67,8 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 	return len;
 }
 
-static ssize_t platform_profile_show(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+int platform_profile_get(enum platform_profile_option *profile)
 {
-	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
 	int err;
 
 	err = mutex_lock_interruptible(&profile_lock);
@@ -65,15 +80,28 @@ static ssize_t platform_profile_show(struct device *dev,
 		return -ENODEV;
 	}
 
-	err = cur_profile->profile_get(cur_profile, &profile);
+	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))))
+	if (WARN_ON((*profile < 0) || (*profile >= ARRAY_SIZE(profile_names))))
 		return -EIO;
 
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_get);
+
+static ssize_t platform_profile_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
+	int ret = platform_profile_get(&profile);
+
+	if (ret)
+		return ret;
 	return sysfs_emit(buf, "%s\n", profile_names[profile]);
 }
 
@@ -130,9 +158,13 @@ static const struct attribute_group platform_profile_group = {
 
 void platform_profile_notify(void)
 {
-	if (!cur_profile)
+	enum platform_profile_option profile;
+
+	if (platform_profile_get(&profile))
 		return;
 	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	platform_profile_call_notifier(PLATFORM_PROFILE_CHANGED, &profile);
+
 }
 EXPORT_SYMBOL_GPL(platform_profile_notify);
 
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index e5cbb6841f3a..05ba3403509a 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -11,6 +11,8 @@
 
 #include <linux/bitops.h>
 
+struct notifier_block;
+
 /*
  * If more options are added please update profile_names array in
  * platform_profile.c and sysfs-platform_profile documentation.
@@ -37,5 +39,13 @@ struct platform_profile_handler {
 int platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_remove(void);
 void platform_profile_notify(void);
+int platform_profile_get(enum platform_profile_option *profile);
+
+int platform_profile_register_notifier(struct notifier_block *nb);
+int platform_profile_unregister_notifier(struct notifier_block *nb);
+
+enum platform_profile_notifier_actions {
+	PLATFORM_PROFILE_CHANGED,
+};
 
 #endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.25.1

Re: [PATCH v4 0/3] Let other drivers react to platform profile changes

[Hans de Goede]

Hi Mario,

On 10/26/21 21:08, Mario Limonciello wrote:
> Currently only one driver can register as a handler for a platform profile.
> 
> This limitation means that if multiple drivers want to react to platform
> profile changes that they will need to directly interact with individual
> drivers.
> 
> Instead introduce a notification flow that drivers can register for event
> changes.  The idea is that any driver that wants to can:
> 1. read the current profile
> 2. set up initial values
> 3. register for changes
> 4. react to changes
> 
> Changes from v3->v4:
>  * Simplify platform_profile_notify
>    - No need to check cur_profile as platform_profile_get does it
>    - No need to track ret as it's unused
> Changes from v2->v3:
>  * Add patches to avoid collisions in hp-wmi and asus-wmi symbols

We typically do not merge new in-kernel API without at least one
consumer of that in-kernel API.

I can take patches 1 + 2 now as prep work for the future,
but I would expect Rafael to only want to take patch 3/3 if
you post a new series which also includes at least one consumer
for the functionality added by 3/3.

That will also make it a lot easier to review 3/3 / to consider
other means to reach the same goal.
Rafael, do you concur?

Regards,

Hans

Re: [PATCH v4 0/3] Let other drivers react to platform profile changes

[Rafael J. Wysocki]

On Wed, Oct 27, 2021 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Mario,
>
> On 10/26/21 21:08, Mario Limonciello wrote:
> > Currently only one driver can register as a handler for a platform profile.
> >
> > This limitation means that if multiple drivers want to react to platform
> > profile changes that they will need to directly interact with individual
> > drivers.
> >
> > Instead introduce a notification flow that drivers can register for event
> > changes.  The idea is that any driver that wants to can:
> > 1. read the current profile
> > 2. set up initial values
> > 3. register for changes
> > 4. react to changes
> >
> > Changes from v3->v4:
> >  * Simplify platform_profile_notify
> >    - No need to check cur_profile as platform_profile_get does it
> >    - No need to track ret as it's unused
> > Changes from v2->v3:
> >  * Add patches to avoid collisions in hp-wmi and asus-wmi symbols
>
> We typically do not merge new in-kernel API without at least one
> consumer of that in-kernel API.
>
> I can take patches 1 + 2 now as prep work for the future,
> but I would expect Rafael to only want to take patch 3/3 if
> you post a new series which also includes at least one consumer
> for the functionality added by 3/3.
>
> That will also make it a lot easier to review 3/3 / to consider
> other means to reach the same goal.
> Rafael, do you concur?

Yes, I do.

Thanks!

RE: [PATCH v4 0/3] Let other drivers react to platform profile changes

[Limonciello, Mario]

[Public]

+Alex, Shyam

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Wednesday, October 27, 2021 08:57
> To: Hans de Goede <hdegoede@redhat.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Mark Gross
> <mgross@linux.intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; open
> list:X86 PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; Mark
> Pearson <markpearson@lenovo.com>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>
> Subject: Re: [PATCH v4 0/3] Let other drivers react to platform profile changes
> 
> On Wed, Oct 27, 2021 at 3:51 PM Hans de Goede <hdegoede@redhat.com>
> wrote:
> >
> > Hi Mario,
> >
> > On 10/26/21 21:08, Mario Limonciello wrote:
> > > Currently only one driver can register as a handler for a platform profile.
> > >
> > > This limitation means that if multiple drivers want to react to platform
> > > profile changes that they will need to directly interact with individual
> > > drivers.
> > >
> > > Instead introduce a notification flow that drivers can register for event
> > > changes.  The idea is that any driver that wants to can:
> > > 1. read the current profile
> > > 2. set up initial values
> > > 3. register for changes
> > > 4. react to changes
> > >
> > > Changes from v3->v4:
> > >  * Simplify platform_profile_notify
> > >    - No need to check cur_profile as platform_profile_get does it
> > >    - No need to track ret as it's unused
> > > Changes from v2->v3:
> > >  * Add patches to avoid collisions in hp-wmi and asus-wmi symbols
> >
> > We typically do not merge new in-kernel API without at least one
> > consumer of that in-kernel API.
> >
> > I can take patches 1 + 2 now as prep work for the future,
> > but I would expect Rafael to only want to take patch 3/3 if
> > you post a new series which also includes at least one consumer
> > for the functionality added by 3/3.
> >
> > That will also make it a lot easier to review 3/3 / to consider
> > other means to reach the same goal.
> > Rafael, do you concur?
> 
> Yes, I do.
> 
> Thanks!

OK Thanks.  At least landing 1 and 2 will make things easier later when
it's time to merge 3.

Alex, Shyam,

This was mostly in preparation for your drivers that may build on top of this.
Please feel free to pull the last patch into your series when you're ready.
Full thread is here: https://patchwork.kernel.org/project/platform-driver-x86/cover/20211026190835.10697-1-mario.limonciello@amd.com/