Re: [PATCH v4] regulator: event: Add regulator netlink event support

[Naresh Solanki]

Hi Matti,


On Thu, 7 Dec 2023 at 13:42, Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> Hi Naresh,
>
> On 12/5/23 12:52, Naresh Solanki wrote:
> > This commit introduces netlink event support to the regulator subsystem.
> >
> > Changes:
> > - Introduce event.c and regnl.h for netlink event handling.
> > - Implement reg_generate_netlink_event to broadcast regulator events.
> > - Update Makefile to include the new event.c file.
> >
> > Signed-off-by: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
>
> Thanks! I have somehow missed the earlier patches (or just don't
> remember seeing them). I _really_ like the idea of sending the regulator
> events as netlink multicasts!
>
> ...
>
> > +
> > +int reg_generate_netlink_event(const char *reg_name, u64 event)
> > +{
> > +     struct sk_buff *skb;
> > +     struct nlattr *attr;
> > +     struct reg_genl_event *edata;
> > +     void *msg_header;
> > +     int size;
> > +
> > +     /* allocate memory */
> > +     size = nla_total_size(sizeof(struct reg_genl_event)) +
> > +         nla_total_size(0);
> > +
> > +     skb = genlmsg_new(size, GFP_ATOMIC);
> > +     if (!skb)
> > +             return -ENOMEM;
> > +
> > +     /* add the genetlink message header */
> > +     msg_header = genlmsg_put(skb, 0, reg_event_seqnum++,
> > +                              &reg_event_genl_family, 0,
> > +                              REG_GENL_CMD_EVENT);
>
> Should the reg_event_seqnum++ be atomic or is access somehow serialized?
Yes making it atomic will be better.
>
> > +     if (!msg_header) {
> > +             nlmsg_free(skb);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* fill the data */
> > +     attr = nla_reserve(skb, REG_GENL_ATTR_EVENT, sizeof(struct reg_genl_event));
> > +     if (!attr) {
> > +             nlmsg_free(skb);
> > +             return -EINVAL;
> > +     }
> > +
> > +     edata = nla_data(attr);
> > +     memset(edata, 0, sizeof(struct reg_genl_event));
> > +
> > +     strscpy(edata->reg_name, reg_name, sizeof(edata->reg_name));
> > +     edata->event = event;
> > +
> > +     /* send multicast genetlink message */
> > +     genlmsg_end(skb, msg_header);
> > +     size = genlmsg_multicast(&reg_event_genl_family, skb, 0, 0, GFP_ATOMIC);
> > +
> > +     return size;
> > +}
> > +
>
> ...
>
> > diff --git a/include/uapi/regulator/regulator.h b/include/uapi/regulator/regulator.h
> > new file mode 100644
> > index 000000000000..d2b5612198b6
> > --- /dev/null
> > +++ b/include/uapi/regulator/regulator.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Regulator uapi header
> > + *
> > + * Author: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > + */
> > +
> > +#ifndef _UAPI_REGULATOR_H
> > +#define _UAPI_REGULATOR_H
> > +
> > +#ifdef __KERNEL__
> > +#include <linux/types.h>
> > +#else
> > +#include <stdint.h>
> > +#endif
> > +
> > +/*
> > + * Regulator notifier events.
> > + *
> > + * UNDER_VOLTAGE  Regulator output is under voltage.
> > + * OVER_CURRENT   Regulator output current is too high.
> > + * REGULATION_OUT Regulator output is out of regulation.
> > + * FAIL           Regulator output has failed.
> > + * OVER_TEMP      Regulator over temp.
> > + * FORCE_DISABLE  Regulator forcibly shut down by software.
> > + * VOLTAGE_CHANGE Regulator voltage changed.
> > + *                Data passed is old voltage cast to (void *).
> > + * DISABLE        Regulator was disabled.
> > + * PRE_VOLTAGE_CHANGE   Regulator is about to have voltage changed.
> > + *                      Data passed is "struct pre_voltage_change_data"
> > + * ABORT_VOLTAGE_CHANGE Regulator voltage change failed for some reason.
> > + *                      Data passed is old voltage cast to (void *).
> > + * PRE_DISABLE    Regulator is about to be disabled
> > + * ABORT_DISABLE  Regulator disable failed for some reason
> > + *
> > + * NOTE: These events can be OR'ed together when passed into handler.
> > + */
> > +
> > +#define REGULATOR_EVENT_UNDER_VOLTAGE                0x01
> > +#define REGULATOR_EVENT_OVER_CURRENT         0x02
> > +#define REGULATOR_EVENT_REGULATION_OUT               0x04
> > +#define REGULATOR_EVENT_FAIL                 0x08
> > +#define REGULATOR_EVENT_OVER_TEMP            0x10
> > +#define REGULATOR_EVENT_FORCE_DISABLE                0x20
> > +#define REGULATOR_EVENT_VOLTAGE_CHANGE               0x40
> > +#define REGULATOR_EVENT_DISABLE                      0x80
> > +#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE   0x100
> > +#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE 0x200
> > +#define REGULATOR_EVENT_PRE_DISABLE          0x400
> > +#define REGULATOR_EVENT_ABORT_DISABLE                0x800
> > +#define REGULATOR_EVENT_ENABLE                       0x1000
> > +/*
> > + * Following notifications should be emitted only if detected condition
> > + * is such that the HW is likely to still be working but consumers should
> > + * take a recovery action to prevent problems esacalating into errors.
>
> It's easier to spot my own typos when someone throws them at my face :)
> Maybe fix the 'esacalating' while shuffling these? (don't know if it's
> preferred to first move everything and only do typofix as own patch - in
> which case it definitely does not need to be done as a part of this
> series. Just commented on this as I noticed it now)
Yes.  I missed fixing the typo.
>
> > + */
> > +#define REGULATOR_EVENT_UNDER_VOLTAGE_WARN   0x2000
> > +#define REGULATOR_EVENT_OVER_CURRENT_WARN    0x4000
> > +#define REGULATOR_EVENT_OVER_VOLTAGE_WARN    0x8000
> > +#define REGULATOR_EVENT_OVER_TEMP_WARN               0x10000
> > +#define REGULATOR_EVENT_WARN_MASK            0x1E000
> > +
> > +struct reg_genl_event {
> > +     char reg_name[32];
> > +     uint64_t event;
> > +};
>
> Do you think we could and / or should separate the event type and event
> severity to own attributes here? I wonder if we will see more
> 'severities' of events in the future. I see we have currently some
> activity for deciding if a regulator event should result for example a
> "data storage protection" by shutting down storage hardware before a
> back-up capacitor runs out of energy. Maybe we see more cases where the
> user-space needs to decide whether to run a (partial) system shutdown or
> do some other action based on regulator events.
>
> I have a feeling that there will be "actions" which are common (like
> system shutdown or data flushing) and could utilize some generic
> user-space daemon - maybe the severity is something such a generic
> daemon could use to decide if shutdown/flush/whatsoever is needed? If
> so, being able to add new severities may be needed - and duplicating
> event flags for all severities may not scale.
>
> OTOH, it's not that hard to append new netlink attributes to the end of
> the message to give user-space a hint regarding what should be done. In
> that sense this is not something I would insist - just wonder if you see
> it sensible?

When I wrote the code, I kept in mind to make sure to receive all events
from regulators so that userspace application (in my case BMC
application which does power sequence) has regulator events information.
Hence I assumed that its upto userspace application to decide on corrective
action based on events of interest.

At the same time I think having it in a way which is very generic & meets
requirement of many use case would be better.

Please correct me if my understanding is inaccurate.
As I understand from your inputs, receiving severity information along
with event would help application decide corrective information insteading
of decoding regulator events.

>
> > +
> > +/* attributes of reg_genl_family */
> > +enum {
> > +     REG_GENL_ATTR_UNSPEC,
> > +     REG_GENL_ATTR_EVENT,    /* reg event info needed by user space */
> > +     __REG_GENL_ATTR_MAX,
> > +};
> > +
> > +#define REG_GENL_ATTR_MAX (__REG_GENL_ATTR_MAX - 1)
> > +
> > +/* commands supported by the reg_genl_family */
> > +enum {
> > +     REG_GENL_CMD_UNSPEC,
> > +     REG_GENL_CMD_EVENT,     /* kernel->user notifications for reg events */
> > +     __REG_GENL_CMD_MAX,
> > +};
> > +
> > +#define REG_GENL_CMD_MAX (__REG_GENL_CMD_MAX - 1)
> > +
> > +#define REG_GENL_FAMILY_NAME         "reg_event"
> > +#define REG_GENL_VERSION             0x01
> > +#define REG_GENL_MCAST_GROUP_NAME    "reg_mc_group"
>
> I am wondering what will the user-space handlers look like? Do we think
> that there will be a 'I am interested in _all_ regulator multicast
> events' type of listener, or do we think there will be listeners who
> would like to listen only for a subset of regulator netlink notifications?
>
> Asking this just because I wonder if we should be prepared for more than
> one regulator multicast group? Do you think that an ability to say "Hey,
> I'd like to listen for under-voltage events only" or "I would like to
> get all temperature-related notifications" should/could be supported by
> more specific multicast groups or is that just over-engineering at this
> point?
Current implementation is such that all events will be sent.
But I agree with you that it is not something desired as sometimes
application might not be interested in all events.
Also I'm not sure on multicast group, as currently only one group
exist for regulator event & how adding additional group would help.


>
> It has been a long while since I wrote netlink code. So, if this makes
> no sense to you it's probably me who is overlooking something.
I'm aligned to make sure regulator netlink serves wider purpose &
hence your inputs are highly valuable.

Based on inputs provided by you(please add if missed anything):
1. Add an attribute severity & set it if event is of critical nature like:
    under-voltage, over-current, event_fail & *any others* ?.
2. Ability to receive specific set of regulator events instead of all events.

Regards,
Naresh

>
> > +
> > +#endif /* _UAPI_REGULATOR_H */
> >
> > base-commit: 753e4d5c433da57da75dd4c3e1aececc8e874a62
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>