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

[PATCH v4 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 v3 -> v4:

- Add 'static inline' in patch2

Changes v2 -> v3:

- Fix unused variable compilation warning
- Add missed Suggested by tag to patch2
 
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/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/

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 | 95 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.c | 40 +++++++++++--
 drivers/thermal/thermal_netlink.h | 26 +++++++++
 include/net/genetlink.h           |  4 ++
 net/netlink/genetlink.c           | 30 ++++++++++
 5 files changed, 180 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v4 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 | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 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..50ec599a5cff 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,39 @@ 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) {
+		int i;
+
+		if (family->n_mcgrps == 0)
+			continue;
+
+		i = group - family->mcgrp_offset;
+		if (i < 0 || i >= family->n_mcgrps)
+			continue;
+
+		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

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

[Jakub Kicinski]

On Mon, 12 Feb 2024 17:16:13 +0100 Stanislaw Gruszka wrote:
> 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>

LGTM! genetlink code is a bit hot lately, to avoid any conflicts can
I put the first patch (or all of them) on a shared branch for both
netdev and PM to pull in? Once the other two patches are reviewed,
obviously.

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

[srinivas pandruvada]

On Mon, 2024-02-12 at 17:07 -0800, Jakub Kicinski wrote:
> On Mon, 12 Feb 2024 17:16:13 +0100 Stanislaw Gruszka wrote:
> > 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>
> 
> LGTM! genetlink code is a bit hot lately, to avoid any conflicts can
> I put the first patch (or all of them) on a shared branch for both
> netdev and PM to pull in? Once the other two patches are reviewed,
> obviously.
> 
If netlink maintainers are happy with the 1/3, you can take 1/3. Once
merged, the PM patches can go separately as they need 1/3.

Hi Daniel,
Please look at 2/3 and also 3/3 if you can.

Thanks,
Srinivas

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

[Jiri Pirko]

Mon, Feb 12, 2024 at 05:16:13PM CET, stanislaw.gruszka@linux.intel.com wrote:
>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>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

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

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.h | 26 ++++++++++++++++++++
 2 files changed, 61 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..e01221e8816b 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,16 @@ static inline int thermal_notify_tz_create(const struct thermal_zone_device *tz)
 	return 0;
 }
 
+static inline int thermal_genl_register_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline 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

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

[Rafael J. Wysocki]

On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> 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.

This should be intel_hfi.  Or better, Intel HFI.

> By leveraging these
> notifications, the driver can determine when consumers are present
> or absent.
>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
>  drivers/thermal/thermal_netlink.h | 26 ++++++++++++++++++++
>  2 files changed, 61 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[] = {

There are enough characters per code line to spell "multicast_groups"
here (and analogously below).

>         [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);

thermal_genl_chain ?

It would be more consistent with the rest of the naming.

>
>  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;

pr_warn_once() would be better IMO.  At least it would not crash the
kernel configured with "panic on warn".

> +
> +       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..e01221e8816b 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,16 @@ static inline int thermal_notify_tz_create(const struct thermal_zone_device *tz)
>         return 0;
>  }
>
> +static inline int thermal_genl_register_notifier(struct notifier_block *nb)
> +{
> +       return 0;
> +}
> +
> +static inline 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;
> --

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

[Stanislaw Gruszka]

On Tue, Feb 13, 2024 at 02:24:56PM +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > 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.
> 
> This should be intel_hfi.  Or better, Intel HFI.

Will change in next revision.

> > By leveraging these
> > notifications, the driver can determine when consumers are present
> > or absent.
> >
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
> >  drivers/thermal/thermal_netlink.h | 26 ++++++++++++++++++++
> >  2 files changed, 61 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[] = {
> 
> There are enough characters per code line to spell "multicast_groups"
> here (and analogously below).

Not sure what you mean, change thermal_genl_mcgrps to thermal_genl_multicast_groups ?

I could change that, but it's not really related to the changes in this patch,
so perhaps in separate patch.

Additionally "mcgrps" are more consistent with genl_family fields i.e:

      .mcgrps         = thermal_genl_mcgrps,
      .n_mcgrps       = ARRAY_SIZE(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);
> 
> thermal_genl_chain ?
> 
> It would be more consistent with the rest of the naming.

Ok, will change. Additionally in separate patch thermal_gnl_family for consistency.

> >  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;
> 
> pr_warn_once() would be better IMO.  At least it would not crash the
> kernel configured with "panic on warn".

"panic on warn" is generic WARN_* issue at any place where WARN_* are used.
And I would say, crash is desired behaviour for those who use the option
to catch bugs. And mcgrp bigger than THERMAL_GENL_MAX_GROUP is definitely
a bug. Additionally pr_warn_once() does not print call trace, so I think
WARN_ON_ONCE() is more proper. But if really you prefer pr_warn_once()
I can change.

Regards
Stanislaw

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

[Rafael J. Wysocki]

On Thu, Feb 22, 2024 at 4:47 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Tue, Feb 13, 2024 at 02:24:56PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> > <stanislaw.gruszka@linux.intel.com> wrote:
> > >
> > > 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.
> >
> > This should be intel_hfi.  Or better, Intel HFI.
>
> Will change in next revision.
>
> > > By leveraging these
> > > notifications, the driver can determine when consumers are present
> > > or absent.
> > >
> > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > ---
> > >  drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
> > >  drivers/thermal/thermal_netlink.h | 26 ++++++++++++++++++++
> > >  2 files changed, 61 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[] = {
> >
> > There are enough characters per code line to spell "multicast_groups"
> > here (and analogously below).
>
> Not sure what you mean, change thermal_genl_mcgrps to thermal_genl_multicast_groups ?
>
> I could change that, but it's not really related to the changes in this patch,
> so perhaps in separate patch.
>
> Additionally "mcgrps" are more consistent with genl_family fields i.e:
>
>       .mcgrps         = thermal_genl_mcgrps,
>       .n_mcgrps       = ARRAY_SIZE(thermal_genl_mcgrps),

OK, never mind.

> > >         [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);
> >
> > thermal_genl_chain ?
> >
> > It would be more consistent with the rest of the naming.
>
> Ok, will change. Additionally in separate patch thermal_gnl_family for consistency.
>
> > >  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;
> >
> > pr_warn_once() would be better IMO.  At least it would not crash the
> > kernel configured with "panic on warn".
>
> "panic on warn" is generic WARN_* issue at any place where WARN_* are used.
> And I would say, crash is desired behaviour for those who use the option
> to catch bugs. And mcgrp bigger than THERMAL_GENL_MAX_GROUP is definitely
> a bug. Additionally pr_warn_once() does not print call trace, so I think
> WARN_ON_ONCE() is more proper. But if really you prefer pr_warn_once()
> I can change.

Fair enough.

[PATCH v4 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 v4 3/3] thermal: intel: hfi: Enable interface only when required

[Rafael J. Wysocki]

On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> 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;
>  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.
> + */

In the comment above I would say "If concurrency is not prevented by
other means, the HFI enable/disable routines must be called under
hfi_instance_lock." and I would retain the comments below (they don't
hurt IMO).

> +static void hfi_do_enable(void *ptr)

I would call this hfi_enable_instance().

> +{
> +       struct hfi_instance *hfi_instance = ptr;

Why is this variable needed ro even useful?  prt can be passed
directly to hfi_set_hw_table().

> +
> +       hfi_set_hw_table(hfi_instance);
> +       hfi_enable();
> +}
> +
> +static void hfi_do_disable(void *ptr)

And I'd call this hfi_disable_instance().

> +{
> +       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);

So AFAICS, one instance can be enabled multiple times because of this.
  I guess that's OK?  In any case, it would be kind of nice to leave a
note regarding it somewhere here.

> +
> +       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;

Is it possible for any clients to be there before the notifier is
registered?  If not, it would be good to add a comment about it.

> +
>         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];
> --

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

[Stanislaw Gruszka]

On Tue, Feb 13, 2024 at 02:59:10PM +0100, Rafael J. Wysocki wrote:
> > -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.
> > + */
> 
> In the comment above I would say "If concurrency is not prevented by
> other means, the HFI enable/disable routines must be called under
> hfi_instance_lock." 

Ok. Will reword this way.

> and I would retain the comments below (they don't
> hurt IMO).

I found those comments somewhat confusing. FWICT at worst
what can happen when enable/resume race CPU online and
disable/suspend race with CPU offline is enable twice
or disable twice. What I think is fine, though plan to
check this (see below).

> > +static void hfi_do_enable(void *ptr)
> 
> I would call this hfi_enable_instance().
> 
> > +{
> > +       struct hfi_instance *hfi_instance = ptr;
> 
> Why is this variable needed ro even useful?  prt can be passed
> directly to hfi_set_hw_table().

Ok, will remove it.

> > +
> > +       hfi_set_hw_table(hfi_instance);
> > +       hfi_enable();
> > +}
> > +
> > +static void hfi_do_disable(void *ptr)
> 
> And I'd call this hfi_disable_instance().

Ok.

> > +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);
> 
> So AFAICS, one instance can be enabled multiple times because of this.
>   I guess that's OK?  In any case, it would be kind of nice to leave a
> note regarding it somewhere here.

It's write the same values to the same registers. So I think this 
should be fine. However after your comment I start to think there
perhaps could be some side-effect of writing the registers.
I'll double check (previously I verified that double enable works,
but only on MTL) or eventually rearrange code to do not enable already
enabled interface.

> > +
> > +       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;
> 
> Is it possible for any clients to be there before the notifier is
> registered?  If not, it would be good to add a comment about it.

HFI is build-in so it's started before any user space. I added note about that
in the cover letter but indeed it should be comment in the code. Will fix.  

Regards
Stanislaw

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

[Jakub Kicinski]

On Mon, 12 Feb 2024 17:16:12 +0100 Stanislaw Gruszka wrote:
>   genetlink: Add per family bind/unbind callbacks

genetlink patch is now in net-next, and pushed to a 6.8-rc4-based
branch at:

 https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git
  for-thermal-genetlink-family-bind-unbind-callbacks

for anyone to pull.

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

[Stanislaw Gruszka]

On Thu, Feb 15, 2024 at 09:29:46PM -0800, Jakub Kicinski wrote:
> On Mon, 12 Feb 2024 17:16:12 +0100 Stanislaw Gruszka wrote:
> >   genetlink: Add per family bind/unbind callbacks
> 
> genetlink patch is now in net-next, and pushed to a 6.8-rc4-based
> branch at:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git
>   for-thermal-genetlink-family-bind-unbind-callbacks
> 
> for anyone to pull.

Thanks!

I'll post next version of this set just to linux-pm since remaining
patches are thermal specific. If they will be ready to apply
the above dependency can be pulled by Rafael - I assume this will
not create any marge conflict.

Regards
Stanislaw

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 12 Feb 2024 17:16:12 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v4,1/3] genetlink: Add per family bind/unbind callbacks
    https://git.kernel.org/netdev/net-next/c/3de21a8990d3
  - [v4,2/3] thermal: netlink: Add genetlink bind/unbind notifications
    (no matching commit)
  - [v4,3/3] thermal: intel: hfi: Enable interface only when required
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Rafael J. Wysocki]

On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> 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 v3 -> v4:
>
> - Add 'static inline' in patch2
>
> Changes v2 -> v3:
>
> - Fix unused variable compilation warning
> - Add missed Suggested by tag to patch2
>
> 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/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
>
> 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 | 95 +++++++++++++++++++++++++++----
>  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
>  drivers/thermal/thermal_netlink.h | 26 +++++++++
>  include/net/genetlink.h           |  4 ++
>  net/netlink/genetlink.c           | 30 ++++++++++
>  5 files changed, 180 insertions(+), 15 deletions(-)
>
> --

What tree is this based on?

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

[Stanislaw Gruszka]

On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > 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 v3 -> v4:
> >
> > - Add 'static inline' in patch2
> >
> > Changes v2 -> v3:
> >
> > - Fix unused variable compilation warning
> > - Add missed Suggested by tag to patch2
> >
> > 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/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> >
> > 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 | 95 +++++++++++++++++++++++++++----
> >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> >  include/net/genetlink.h           |  4 ++
> >  net/netlink/genetlink.c           | 30 ++++++++++
> >  5 files changed, 180 insertions(+), 15 deletions(-)
> >
> > --
> 
> What tree is this based on?

v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
it's on top of linux-pm master, but require additional net dependency,
which can be added by:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
git merge FETCH_HEAD

and will be merged mainline in the next merge window.
So at this point would be probably better just wait for 6.9-rc1 
when the dependency will be in the mainline, before applying this set.

Regards
Stanislaw

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

[Rafael J. Wysocki]

On Thu, Feb 29, 2024 at 5:13 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> > <stanislaw.gruszka@linux.intel.com> wrote:
> > >
> > > 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 v3 -> v4:
> > >
> > > - Add 'static inline' in patch2
> > >
> > > Changes v2 -> v3:
> > >
> > > - Fix unused variable compilation warning
> > > - Add missed Suggested by tag to patch2
> > >
> > > 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/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> > >
> > > 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 | 95 +++++++++++++++++++++++++++----
> > >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> > >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> > >  include/net/genetlink.h           |  4 ++
> > >  net/netlink/genetlink.c           | 30 ++++++++++
> > >  5 files changed, 180 insertions(+), 15 deletions(-)
> > >
> > > --
> >
> > What tree is this based on?
>
> v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
> it's on top of linux-pm master, but require additional net dependency,
> which can be added by:
>
> git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
> git merge FETCH_HEAD
>
> and will be merged mainline in the next merge window.
> So at this point would be probably better just wait for 6.9-rc1
> when the dependency will be in the mainline, before applying this set.

And that's what's going to happen, unless I have any additional
comments on the patches.

Thanks!

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

[Rafael J. Wysocki]

On Thu, Feb 29, 2024 at 5:24 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 29, 2024 at 5:13 PM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> > > On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> > > <stanislaw.gruszka@linux.intel.com> wrote:
> > > >
> > > > 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 v3 -> v4:
> > > >
> > > > - Add 'static inline' in patch2
> > > >
> > > > Changes v2 -> v3:
> > > >
> > > > - Fix unused variable compilation warning
> > > > - Add missed Suggested by tag to patch2
> > > >
> > > > 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/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > > > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > > > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> > > >
> > > > 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 | 95 +++++++++++++++++++++++++++----
> > > >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> > > >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> > > >  include/net/genetlink.h           |  4 ++
> > > >  net/netlink/genetlink.c           | 30 ++++++++++
> > > >  5 files changed, 180 insertions(+), 15 deletions(-)
> > > >
> > > > --
> > >
> > > What tree is this based on?
> >
> > v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
> > it's on top of linux-pm master, but require additional net dependency,
> > which can be added by:
> >
> > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
> > git merge FETCH_HEAD
> >
> > and will be merged mainline in the next merge window.
> > So at this point would be probably better just wait for 6.9-rc1
> > when the dependency will be in the mainline, before applying this set.
>
> And that's what's going to happen, unless I have any additional
> comments on the patches.

Now applied as 6.10 material (with a minor change in the subject of
the last patch) and I'm assuming that Srinivas likes this.