[PATCH v2 0/3] thermal/netlink/intel_hfi: Enable HFI feature only when required

[PATCH v2 0/3] thermal/netlink/intel_hfi: Enable HFI feature only when required

[Stanislaw Gruszka]

The patchset introduces a new genetlink family bind/unbind callbacks
and thermal/netlink notifications, which allow drivers to send netlink
multicast events based on the presence of actual user-space consumers.
This functionality optimizes resource usage by allowing disabling
of features when not needed.

Then implement the notification mechanism in the intel_hif driver,
it is utilized to disable the Hardware Feedback Interface (HFI)
dynamically. By implementing a thermal genl notify callback, the driver
can now enable or disable the HFI based on actual demand, particularly
when user-space applications like intel-speed-select or Intel Low Power
daemon utilize events related to performance and energy efficiency
capabilities.

On machines where Intel HFI is present, but there are no user-space
components installed, we can save tons of CPU cycles.

Changes v1 -> v2:

- Rewrite using netlink_bind/netlink_unbind callbacks.

- Minor changelog tweaks.

- Add missing check in intel hfi syscore resume (had it on my testing,
but somehow missed in post).

- Do not use netlink_has_listeners() any longer, use custom counter instead.
To keep using netlink_has_listners() would be required to rearrange 
netlink_setsockopt() and possibly netlink_bind() functions, to call 
nlk->netlink_bind() after listeners are updated. So I decided to custom
counter. This have potential issue as thermal netlink registers before
intel_hif, so theoretically intel_hif can miss events. But since both
are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
configured), they start before any user-space.

v1: https://lore.kernel.org/linux-pm/Zb48Z408e18QgsAr@nanopsycho/#r

Stanislaw Gruszka (3):
  genetlink: Add per family bind/unbind callbacks
  thermal: netlink: Add genetlink bind/unbind notifications
  thermal: intel: hfi: Enable interface only when required

 drivers/thermal/intel/intel_hfi.c | 96 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.c | 40 +++++++++++--
 drivers/thermal/thermal_netlink.h | 25 ++++++++
 include/net/genetlink.h           |  4 ++
 net/netlink/genetlink.c           | 33 +++++++++++
 5 files changed, 183 insertions(+), 15 deletions(-)


base-commit: bd0e3c391ff3c3c5c9b41227d6b7433fcf4d9c61
-- 
2.34.1

[PATCH v2 1/3] genetlink: Add per family bind/unbind callbacks

[Stanislaw Gruszka]

Add genetlink family bind()/unbind() callbacks when adding/removing
multicast group to/from netlink client socket via setsockopt() or
bind() syscall.

They can be used to track if consumers of netlink multicast messages
emerge or disappear. Thus, a client implementing callbacks, can now
send events only when there are active consumers, preventing unnecessary
work when none exist.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 include/net/genetlink.h |  4 ++++
 net/netlink/genetlink.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index e61469129402..ecadba836ae5 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -41,6 +41,8 @@ struct genl_info;
  *	do additional, common, filtering and return an error
  * @post_doit: called after an operation's doit callback, it may
  *	undo operations done by pre_doit, for example release locks
+ * @bind: called when family multicast group is added to a netlink socket
+ * @unbind: called when family multicast group is removed from a netlink socket
  * @module: pointer to the owning module (set to THIS_MODULE)
  * @mcgrps: multicast groups used by this family
  * @n_mcgrps: number of multicast groups
@@ -84,6 +86,8 @@ struct genl_family {
 	void			(*post_doit)(const struct genl_split_ops *ops,
 					     struct sk_buff *skb,
 					     struct genl_info *info);
+	int			(*bind)(int mcgrp);
+	void			(*unbind)(int mcgrp);
 	const struct genl_ops *	ops;
 	const struct genl_small_ops *small_ops;
 	const struct genl_split_ops *split_ops;
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 8c7af02f8454..0d1551dadb63 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1836,6 +1836,9 @@ static int genl_bind(struct net *net, int group)
 		    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
 			ret = -EPERM;
 
+		if (family->bind)
+			family->bind(i);
+
 		break;
 	}
 
@@ -1843,12 +1846,42 @@ static int genl_bind(struct net *net, int group)
 	return ret;
 }
 
+static void genl_unbind(struct net *net, int group)
+{
+	const struct genl_family *family;
+	unsigned int id;
+
+	down_read(&cb_lock);
+
+	idr_for_each_entry(&genl_fam_idr, family, id) {
+		const struct genl_multicast_group *grp;
+		int i;
+
+		if (family->n_mcgrps == 0)
+			continue;
+
+		i = group - family->mcgrp_offset;
+		if (i < 0 || i >= family->n_mcgrps)
+			continue;
+
+		grp = &family->mcgrps[i];
+
+		if (family->unbind)
+			family->unbind(i);
+
+		break;
+	}
+
+	up_read(&cb_lock);
+}
+
 static int __net_init genl_pernet_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input		= genl_rcv,
 		.flags		= NL_CFG_F_NONROOT_RECV,
 		.bind		= genl_bind,
+		.unbind		= genl_unbind,
 		.release	= genl_release,
 	};
 
-- 
2.34.1

[PATCH v2 2/3] thermal: netlink: Add genetlink bind/unbind notifications

[Stanislaw Gruszka]

Introduce a new feature to the thermal netlink framework, enabling the
registration of sub drivers to receive events via a notifier mechanism.
Specifically, implement genetlink family bind and unbind callbacks to send
BIND and UNBIND events.

The primary purpose of this enhancement is to facilitate the tracking of
user-space consumers by the intel_hif driver. By leveraging these
notifications, the driver can determine when consumers are present
or absent.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.h | 25 +++++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 76a231a29654..86c7653a9530 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -7,17 +7,13 @@
  * Generic netlink for thermal management framework
  */
 #include <linux/module.h>
+#include <linux/notifier.h>
 #include <linux/kernel.h>
 #include <net/genetlink.h>
 #include <uapi/linux/thermal.h>
 
 #include "thermal_core.h"
 
-enum thermal_genl_multicast_groups {
-	THERMAL_GENL_SAMPLING_GROUP = 0,
-	THERMAL_GENL_EVENT_GROUP = 1,
-};
-
 static const struct genl_multicast_group thermal_genl_mcgrps[] = {
 	[THERMAL_GENL_SAMPLING_GROUP] = { .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
 	[THERMAL_GENL_EVENT_GROUP]  = { .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
@@ -75,6 +71,7 @@ struct param {
 typedef int (*cb_t)(struct param *);
 
 static struct genl_family thermal_gnl_family;
+static BLOCKING_NOTIFIER_HEAD(thermal_gnl_chain);
 
 static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
 {
@@ -645,6 +642,27 @@ static int thermal_genl_cmd_doit(struct sk_buff *skb,
 	return ret;
 }
 
+static int thermal_genl_bind(int mcgrp)
+{
+	struct thermal_genl_notify n = { .mcgrp = mcgrp };
+
+	if (WARN_ON_ONCE(mcgrp > THERMAL_GENL_MAX_GROUP))
+		return -EINVAL;
+
+	blocking_notifier_call_chain(&thermal_gnl_chain, THERMAL_NOTIFY_BIND, &n);
+	return 0;
+}
+
+static void thermal_genl_unbind(int mcgrp)
+{
+	struct thermal_genl_notify n = { .mcgrp = mcgrp };
+
+	if (WARN_ON_ONCE(mcgrp > THERMAL_GENL_MAX_GROUP))
+		return;
+
+	blocking_notifier_call_chain(&thermal_gnl_chain, THERMAL_NOTIFY_UNBIND, &n);
+}
+
 static const struct genl_small_ops thermal_genl_ops[] = {
 	{
 		.cmd = THERMAL_GENL_CMD_TZ_GET_ID,
@@ -679,6 +697,8 @@ static struct genl_family thermal_gnl_family __ro_after_init = {
 	.version	= THERMAL_GENL_VERSION,
 	.maxattr	= THERMAL_GENL_ATTR_MAX,
 	.policy		= thermal_genl_policy,
+	.bind		= thermal_genl_bind,
+	.unbind		= thermal_genl_unbind,
 	.small_ops	= thermal_genl_ops,
 	.n_small_ops	= ARRAY_SIZE(thermal_genl_ops),
 	.resv_start_op	= THERMAL_GENL_CMD_CDEV_GET + 1,
@@ -686,6 +706,16 @@ static struct genl_family thermal_gnl_family __ro_after_init = {
 	.n_mcgrps	= ARRAY_SIZE(thermal_genl_mcgrps),
 };
 
+int thermal_genl_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&thermal_gnl_chain, nb);
+}
+
+int thermal_genl_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&thermal_gnl_chain, nb);
+}
+
 int __init thermal_netlink_init(void)
 {
 	return genl_register_family(&thermal_gnl_family);
diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
index 93a927e144d5..69211ece7392 100644
--- a/drivers/thermal/thermal_netlink.h
+++ b/drivers/thermal/thermal_netlink.h
@@ -10,6 +10,19 @@ struct thermal_genl_cpu_caps {
 	int efficiency;
 };
 
+enum thermal_genl_multicast_groups {
+	THERMAL_GENL_SAMPLING_GROUP = 0,
+	THERMAL_GENL_EVENT_GROUP = 1,
+	THERMAL_GENL_MAX_GROUP = THERMAL_GENL_EVENT_GROUP,
+};
+
+#define THERMAL_NOTIFY_BIND	0
+#define THERMAL_NOTIFY_UNBIND	1
+
+struct thermal_genl_notify {
+	int mcgrp;
+};
+
 struct thermal_zone_device;
 struct thermal_trip;
 struct thermal_cooling_device;
@@ -18,6 +31,9 @@ struct thermal_cooling_device;
 #ifdef CONFIG_THERMAL_NETLINK
 int __init thermal_netlink_init(void);
 void __init thermal_netlink_exit(void);
+int thermal_genl_register_notifier(struct notifier_block *nb);
+int thermal_genl_unregister_notifier(struct notifier_block *nb);
+
 int thermal_notify_tz_create(const struct thermal_zone_device *tz);
 int thermal_notify_tz_delete(const struct thermal_zone_device *tz);
 int thermal_notify_tz_enable(const struct thermal_zone_device *tz);
@@ -48,6 +64,15 @@ static inline int thermal_notify_tz_create(const struct thermal_zone_device *tz)
 	return 0;
 }
 
+int thermal_genl_register_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+int thermal_genl_unregister_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
 static inline int thermal_notify_tz_delete(const struct thermal_zone_device *tz)
 {
 	return 0;
-- 
2.34.1

[PATCH v2 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

Enable and disable hardware feedback interface (HFI) when user space
handler is present. For example, enable HFI, when intel-speed-select or
Intel Low Power daemon is running and subscribing to thermal netlink
events. When user space handlers exit or remove subscription for
thermal netlink events, disable HFI.

Summary of changes:

- Register a thermal genetlink notifier

- In the notifier, process THERMAL_NOTIFY_BIND and THERMAL_NOTIFY_UNBIND
reason codes to count number of thermal event group netlink multicast
clients. If thermal netlink group has any listener enable HFI on all
packages. If there are no listener disable HFI on all packages.

- When CPU is online, instead of blindly enabling HFI, check if
the thermal netlink group has any listener. This will make sure that
HFI is not enabled by default during boot time.

- Actual processing to enable/disable matches what is done in
suspend/resume callbacks. Create two functions hfi_do_enable()
and hfi_do_disable(), which can be called from  the netlink notifier
callback and suspend/resume callbacks.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
index 3b04c6ec4fca..5e1e2b5269b7 100644
--- a/drivers/thermal/intel/intel_hfi.c
+++ b/drivers/thermal/intel/intel_hfi.c
@@ -159,6 +159,7 @@ struct hfi_cpu_info {
 static DEFINE_PER_CPU(struct hfi_cpu_info, hfi_cpu_info) = { .index = -1 };
 
 static int max_hfi_instances;
+static int hfi_thermal_clients_num;
 static struct hfi_instance *hfi_instances;
 
 static struct hfi_features hfi_features;
@@ -477,8 +478,11 @@ void intel_hfi_online(unsigned int cpu)
 enable:
 	cpumask_set_cpu(cpu, hfi_instance->cpus);
 
-	/* Enable this HFI instance if this is its first online CPU. */
-	if (cpumask_weight(hfi_instance->cpus) == 1) {
+	/*
+	 * Enable this HFI instance if this is its first online CPU and
+	 * there are user-space clients of thermal events.
+	 */
+	if (cpumask_weight(hfi_instance->cpus) == 1 && hfi_thermal_clients_num > 0) {
 		hfi_set_hw_table(hfi_instance);
 		hfi_enable();
 	}
@@ -573,28 +577,93 @@ static __init int hfi_parse_features(void)
 	return 0;
 }
 
-static void hfi_do_enable(void)
+/*
+ * HFI enable/disable run in non-concurrent manner on boot CPU in syscore
+ * callbacks or under protection of hfi_instance_lock.
+ */
+static void hfi_do_enable(void *ptr)
+{
+	struct hfi_instance *hfi_instance = ptr;
+
+	hfi_set_hw_table(hfi_instance);
+	hfi_enable();
+}
+
+static void hfi_do_disable(void *ptr)
+{
+	hfi_disable();
+}
+
+static void hfi_syscore_resume(void)
 {
 	/* This code runs only on the boot CPU. */
 	struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0);
 	struct hfi_instance *hfi_instance = info->hfi_instance;
 
-	/* No locking needed. There is no concurrency with CPU online. */
-	hfi_set_hw_table(hfi_instance);
-	hfi_enable();
+	if (hfi_thermal_clients_num > 0)
+		hfi_do_enable(hfi_instance);
 }
 
-static int hfi_do_disable(void)
+static int hfi_syscore_suspend(void)
 {
-	/* No locking needed. There is no concurrency with CPU offline. */
 	hfi_disable();
 
 	return 0;
 }
 
 static struct syscore_ops hfi_pm_ops = {
-	.resume = hfi_do_enable,
-	.suspend = hfi_do_disable,
+	.resume = hfi_syscore_resume,
+	.suspend = hfi_syscore_suspend,
+};
+
+static int hfi_thermal_notify(struct notifier_block *nb, unsigned long state,
+			      void *_notify)
+{
+	struct thermal_genl_notify *notify = _notify;
+	struct hfi_instance *hfi_instance;
+	smp_call_func_t func;
+	unsigned int cpu;
+	int i;
+
+	if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
+		return NOTIFY_DONE;
+
+	if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
+		return NOTIFY_DONE;
+
+	mutex_lock(&hfi_instance_lock);
+
+	switch (state) {
+	case THERMAL_NOTIFY_BIND:
+		hfi_thermal_clients_num++;
+		break;
+
+	case THERMAL_NOTIFY_UNBIND:
+		hfi_thermal_clients_num--;
+		break;
+	}
+
+	if (hfi_thermal_clients_num > 0)
+		func = hfi_do_enable;
+	else
+		func = hfi_do_disable;
+
+	for (i = 0; i < max_hfi_instances; i++) {
+		hfi_instance = &hfi_instances[i];
+		if (cpumask_empty(hfi_instance->cpus))
+			continue;
+
+		cpu = cpumask_any(hfi_instance->cpus);
+		smp_call_function_single(cpu, func, hfi_instance, true);
+	}
+
+	mutex_unlock(&hfi_instance_lock);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block hfi_thermal_nb = {
+	.notifier_call = hfi_thermal_notify,
 };
 
 void __init intel_hfi_init(void)
@@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
 	if (!hfi_updates_wq)
 		goto err_nomem;
 
+	if (thermal_genl_register_notifier(&hfi_thermal_nb))
+		goto err_nl_notif;
+
 	register_syscore_ops(&hfi_pm_ops);
 
 	return;
 
+err_nl_notif:
+	destroy_workqueue(hfi_updates_wq);
+
 err_nomem:
 	for (j = 0; j < i; ++j) {
 		hfi_instance = &hfi_instances[j];
-- 
2.34.1

Re: [PATCH v2 1/3] genetlink: Add per family bind/unbind callbacks

[Jakub Kicinski]

On Tue,  6 Feb 2024 14:36:03 +0100 Stanislaw Gruszka wrote:
> +static void genl_unbind(struct net *net, int group)
> +{
> +	const struct genl_family *family;
> +	unsigned int id;
> +
> +	down_read(&cb_lock);
> +
> +	idr_for_each_entry(&genl_fam_idr, family, id) {
> +		const struct genl_multicast_group *grp;
> +		int i;
> +
> +		if (family->n_mcgrps == 0)
> +			continue;
> +
> +		i = group - family->mcgrp_offset;
> +		if (i < 0 || i >= family->n_mcgrps)
> +			continue;
> +
> +		grp = &family->mcgrps[i];
> +
> +		if (family->unbind)
> +			family->unbind(i);
> +
> +		break;

Compiler says:

net/netlink/genetlink.c:1857:52: warning: variable ‘grp’ set but not used [-Wunused-but-set-variable]
 1857 |                 const struct genl_multicast_group *grp;
      |                                                    ^~~
-- 
pw-bot: cr

Re: [PATCH v2 3/3] thermal: intel: hfi: Enable interface only when required

[Ricardo Neri]

On Tue, Feb 06, 2024 at 02:36:05PM +0100, Stanislaw Gruszka wrote:
> Enable and disable hardware feedback interface (HFI) when user space
> handler is present. For example, enable HFI, when intel-speed-select or
> Intel Low Power daemon is running and subscribing to thermal netlink
> events. When user space handlers exit or remove subscription for
> thermal netlink events, disable HFI.
> 
> Summary of changes:
> 
> - Register a thermal genetlink notifier
> 
> - In the notifier, process THERMAL_NOTIFY_BIND and THERMAL_NOTIFY_UNBIND
> reason codes to count number of thermal event group netlink multicast
> clients. If thermal netlink group has any listener enable HFI on all
> packages. If there are no listener disable HFI on all packages.
> 
> - When CPU is online, instead of blindly enabling HFI, check if
> the thermal netlink group has any listener. This will make sure that
> HFI is not enabled by default during boot time.
> 
> - Actual processing to enable/disable matches what is done in
> suspend/resume callbacks. Create two functions hfi_do_enable()
> and hfi_do_disable(), which can be called from  the netlink notifier
> callback and suspend/resume callbacks.
> 
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index 3b04c6ec4fca..5e1e2b5269b7 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -159,6 +159,7 @@ struct hfi_cpu_info {
>  static DEFINE_PER_CPU(struct hfi_cpu_info, hfi_cpu_info) = { .index = -1 };
>  
>  static int max_hfi_instances;
> +static int hfi_thermal_clients_num;

Perhaps this counter can be generalized for other clients besides netlink.
KVM could also use it to enable/disable HFI as needed for virtual machines.

Maybe we should expose a function intel_hfi_toggle(bool enable) or a couple
of intel_hfi_enable()/intel_hfi_disable() functions. The former would
increase the counter and enable HFI on all packages. The latter would
decrease the counter and disable HFI if the counter becomes 0.

>  static struct hfi_instance *hfi_instances;
>  
>  static struct hfi_features hfi_features;
> @@ -477,8 +478,11 @@ void intel_hfi_online(unsigned int cpu)
>  enable:
>  	cpumask_set_cpu(cpu, hfi_instance->cpus);
>  
> -	/* Enable this HFI instance if this is its first online CPU. */
> -	if (cpumask_weight(hfi_instance->cpus) == 1) {
> +	/*
> +	 * Enable this HFI instance if this is its first online CPU and
> +	 * there are user-space clients of thermal events.
> +	 */
> +	if (cpumask_weight(hfi_instance->cpus) == 1 && hfi_thermal_clients_num > 0) {
>  		hfi_set_hw_table(hfi_instance);
>  		hfi_enable();
>  	}
> @@ -573,28 +577,93 @@ static __init int hfi_parse_features(void)
>  	return 0;
>  }
>  
> -static void hfi_do_enable(void)
> +/*
> + * HFI enable/disable run in non-concurrent manner on boot CPU in syscore
> + * callbacks or under protection of hfi_instance_lock.
> + */
> +static void hfi_do_enable(void *ptr)
> +{
> +	struct hfi_instance *hfi_instance = ptr;
> +
> +	hfi_set_hw_table(hfi_instance);
> +	hfi_enable();
> +}
> +
> +static void hfi_do_disable(void *ptr)
> +{
> +	hfi_disable();
> +}
> +
> +static void hfi_syscore_resume(void)
>  {
>  	/* This code runs only on the boot CPU. */
>  	struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0);
>  	struct hfi_instance *hfi_instance = info->hfi_instance;
>  
> -	/* No locking needed. There is no concurrency with CPU online. */
> -	hfi_set_hw_table(hfi_instance);
> -	hfi_enable();
> +	if (hfi_thermal_clients_num > 0)
> +		hfi_do_enable(hfi_instance);
>  }
>  
> -static int hfi_do_disable(void)
> +static int hfi_syscore_suspend(void)
>  {
> -	/* No locking needed. There is no concurrency with CPU offline. */
>  	hfi_disable();
>  
>  	return 0;
>  }
>  
>  static struct syscore_ops hfi_pm_ops = {
> -	.resume = hfi_do_enable,
> -	.suspend = hfi_do_disable,
> +	.resume = hfi_syscore_resume,
> +	.suspend = hfi_syscore_suspend,
> +};
> +
> +static int hfi_thermal_notify(struct notifier_block *nb, unsigned long state,
> +			      void *_notify)
> +{
> +	struct thermal_genl_notify *notify = _notify;
> +	struct hfi_instance *hfi_instance;
> +	smp_call_func_t func;
> +	unsigned int cpu;
> +	int i;
> +
> +	if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
> +		return NOTIFY_DONE;
> +
> +	if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&hfi_instance_lock);
> +
> +	switch (state) {
> +	case THERMAL_NOTIFY_BIND:
> +		hfi_thermal_clients_num++;
> +		break;

Perhaps here you could call intel_hfi_enable()

> +	case THERMAL_NOTIFY_UNBIND:
> +		hfi_thermal_clients_num--;
> +		break;
> +	}

and here intel_hfi_disable().

> +
> +	if (hfi_thermal_clients_num > 0)
> +		func = hfi_do_enable;
> +	else
> +		func = hfi_do_disable;
> +
> +	for (i = 0; i < max_hfi_instances; i++) {
> +		hfi_instance = &hfi_instances[i];
> +		if (cpumask_empty(hfi_instance->cpus))
> +			continue;
> +
> +		cpu = cpumask_any(hfi_instance->cpus);
> +		smp_call_function_single(cpu, func, hfi_instance, true);
> +	}

This block would go in a helper function.

I know this is beyond the scope of the patchset but it would make the
logic more generic for other clients to use.
> +
> +	mutex_unlock(&hfi_instance_lock);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block hfi_thermal_nb = {
> +	.notifier_call = hfi_thermal_notify,
>  };
>  
>  void __init intel_hfi_init(void)
> @@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
>  	if (!hfi_updates_wq)
>  		goto err_nomem;
>  
> +	if (thermal_genl_register_notifier(&hfi_thermal_nb))
> +		goto err_nl_notif;
> +
>  	register_syscore_ops(&hfi_pm_ops);
>  
>  	return;
>  
> +err_nl_notif:
> +	destroy_workqueue(hfi_updates_wq);
> +
>  err_nomem:
>  	for (j = 0; j < i; ++j) {
>  		hfi_instance = &hfi_instances[j];
> -- 
> 2.34.1
> 

Re: [PATCH v2 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

On Tue, Feb 06, 2024 at 07:18:54PM -0800, Ricardo Neri wrote:
> On Tue, Feb 06, 2024 at 02:36:05PM +0100, Stanislaw Gruszka wrote:
> > Enable and disable hardware feedback interface (HFI) when user space
> > handler is present. For example, enable HFI, when intel-speed-select or
> > Intel Low Power daemon is running and subscribing to thermal netlink
> > events. When user space handlers exit or remove subscription for
> > thermal netlink events, disable HFI.
> > 
> > Summary of changes:
> > 
> > - Register a thermal genetlink notifier
> > 
> > - In the notifier, process THERMAL_NOTIFY_BIND and THERMAL_NOTIFY_UNBIND
> > reason codes to count number of thermal event group netlink multicast
> > clients. If thermal netlink group has any listener enable HFI on all
> > packages. If there are no listener disable HFI on all packages.
> > 
> > - When CPU is online, instead of blindly enabling HFI, check if
> > the thermal netlink group has any listener. This will make sure that
> > HFI is not enabled by default during boot time.
> > 
> > - Actual processing to enable/disable matches what is done in
> > suspend/resume callbacks. Create two functions hfi_do_enable()
> > and hfi_do_disable(), which can be called from  the netlink notifier
> > callback and suspend/resume callbacks.
> > 
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
> >  1 file changed, 85 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> > index 3b04c6ec4fca..5e1e2b5269b7 100644
> > --- a/drivers/thermal/intel/intel_hfi.c
> > +++ b/drivers/thermal/intel/intel_hfi.c
> > @@ -159,6 +159,7 @@ struct hfi_cpu_info {
> >  static DEFINE_PER_CPU(struct hfi_cpu_info, hfi_cpu_info) = { .index = -1 };
> >  
> >  static int max_hfi_instances;
> > +static int hfi_thermal_clients_num;
> 
> Perhaps this counter can be generalized for other clients besides netlink.
> KVM could also use it to enable/disable HFI as needed for virtual machines.

Probably it will be useful for other clients, however that will depend
how (KVM) callback/notification would be implemented. For now I would
stick with thermal_clients_num, name can be always changed.

> Maybe we should expose a function intel_hfi_toggle(bool enable) or a couple
I think toggle(true) / toggle(false) is less understandable than
enable() / disable(). Generally "toggle" means switch to opposite
state.

> of intel_hfi_enable()/intel_hfi_disable() functions. The former would
> increase the counter and enable HFI on all packages. The latter would
> decrease the counter and disable HFI if the counter becomes 0.

I would prefer not to do this, as well as other similar things you
requested below, in this patchset. Those can be done as separate
cleanup patch on top of this set. If there is benefit of such refactors
it will be clean seen then.
 
Regards
Stanislaw


> >  static struct hfi_instance *hfi_instances;
> >  
> >  static struct hfi_features hfi_features;
> > @@ -477,8 +478,11 @@ void intel_hfi_online(unsigned int cpu)
> >  enable:
> >  	cpumask_set_cpu(cpu, hfi_instance->cpus);
> >  
> > -	/* Enable this HFI instance if this is its first online CPU. */
> > -	if (cpumask_weight(hfi_instance->cpus) == 1) {
> > +	/*
> > +	 * Enable this HFI instance if this is its first online CPU and
> > +	 * there are user-space clients of thermal events.
> > +	 */
> > +	if (cpumask_weight(hfi_instance->cpus) == 1 && hfi_thermal_clients_num > 0) {
> >  		hfi_set_hw_table(hfi_instance);
> >  		hfi_enable();
> >  	}
> > @@ -573,28 +577,93 @@ static __init int hfi_parse_features(void)
> >  	return 0;
> >  }
> >  
> > -static void hfi_do_enable(void)
> > +/*
> > + * HFI enable/disable run in non-concurrent manner on boot CPU in syscore
> > + * callbacks or under protection of hfi_instance_lock.
> > + */
> > +static void hfi_do_enable(void *ptr)
> > +{
> > +	struct hfi_instance *hfi_instance = ptr;
> > +
> > +	hfi_set_hw_table(hfi_instance);
> > +	hfi_enable();
> > +}
> > +
> > +static void hfi_do_disable(void *ptr)
> > +{
> > +	hfi_disable();
> > +}
> > +
> > +static void hfi_syscore_resume(void)
> >  {
> >  	/* This code runs only on the boot CPU. */
> >  	struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0);
> >  	struct hfi_instance *hfi_instance = info->hfi_instance;
> >  
> > -	/* No locking needed. There is no concurrency with CPU online. */
> > -	hfi_set_hw_table(hfi_instance);
> > -	hfi_enable();
> > +	if (hfi_thermal_clients_num > 0)
> > +		hfi_do_enable(hfi_instance);
> >  }
> >  
> > -static int hfi_do_disable(void)
> > +static int hfi_syscore_suspend(void)
> >  {
> > -	/* No locking needed. There is no concurrency with CPU offline. */
> >  	hfi_disable();
> >  
> >  	return 0;
> >  }
> >  
> >  static struct syscore_ops hfi_pm_ops = {
> > -	.resume = hfi_do_enable,
> > -	.suspend = hfi_do_disable,
> > +	.resume = hfi_syscore_resume,
> > +	.suspend = hfi_syscore_suspend,
> > +};
> > +
> > +static int hfi_thermal_notify(struct notifier_block *nb, unsigned long state,
> > +			      void *_notify)
> > +{
> > +	struct thermal_genl_notify *notify = _notify;
> > +	struct hfi_instance *hfi_instance;
> > +	smp_call_func_t func;
> > +	unsigned int cpu;
> > +	int i;
> > +
> > +	if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
> > +		return NOTIFY_DONE;
> > +
> > +	if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
> > +		return NOTIFY_DONE;
> > +
> > +	mutex_lock(&hfi_instance_lock);
> > +
> > +	switch (state) {
> > +	case THERMAL_NOTIFY_BIND:
> > +		hfi_thermal_clients_num++;
> > +		break;
> 
> Perhaps here you could call intel_hfi_enable()
> 
> > +	case THERMAL_NOTIFY_UNBIND:
> > +		hfi_thermal_clients_num--;
> > +		break;
> > +	}
> 
> and here intel_hfi_disable().
> 
> > +
> > +	if (hfi_thermal_clients_num > 0)
> > +		func = hfi_do_enable;
> > +	else
> > +		func = hfi_do_disable;
> > +
> > +	for (i = 0; i < max_hfi_instances; i++) {
> > +		hfi_instance = &hfi_instances[i];
> > +		if (cpumask_empty(hfi_instance->cpus))
> > +			continue;
> > +
> > +		cpu = cpumask_any(hfi_instance->cpus);
> > +		smp_call_function_single(cpu, func, hfi_instance, true);
> > +	}
> 
> This block would go in a helper function.
> 
> I know this is beyond the scope of the patchset but it would make the
> logic more generic for other clients to use.
> > +
> > +	mutex_unlock(&hfi_instance_lock);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block hfi_thermal_nb = {
> > +	.notifier_call = hfi_thermal_notify,
> >  };
> >  
> >  void __init intel_hfi_init(void)
> > @@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
> >  	if (!hfi_updates_wq)
> >  		goto err_nomem;
> >  
> > +	if (thermal_genl_register_notifier(&hfi_thermal_nb))
> > +		goto err_nl_notif;
> > +
> >  	register_syscore_ops(&hfi_pm_ops);
> >  
> >  	return;
> >  
> > +err_nl_notif:
> > +	destroy_workqueue(hfi_updates_wq);
> > +
> >  err_nomem:
> >  	for (j = 0; j < i; ++j) {
> >  		hfi_instance = &hfi_instances[j];
> > -- 
> > 2.34.1
> >