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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0goAi7S5LQYsqs6ja=ouPX_QN6OYxNvLb5oTj=VMS2ZCA@mail.gmail.com>
Date: Thu, 22 Feb 2024 16:55:49 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org, 
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, 
	Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Jiri Pirko <jiri@...nulli.us>, Johannes Berg <johannes@...solutions.net>, 
	Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH v4 2/3] thermal: netlink: Add genetlink bind/unbind notifications

On Thu, Feb 22, 2024 at 4:47 PM Stanislaw Gruszka
<stanislaw.gruszka@...ux.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@...ux.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@...nel.org>
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@...ux.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.