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

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

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