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

[Matti Vaittinen]

On 12/7/23 14:39, Naresh Solanki wrote:
> 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!
> >
> > ...

> > > + */
> > > +#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.

I do also think it probably is. However, do you see cases where the 
action to be taken is a result of a hardware-design. Maybe in such cases 
the information like "critical under-voltage, please shut down the 
system" could originate from the board designer's drawing-table, end up 
in board device-tree, be read by the drivers/kernel and then be sent to 
a user-land with suitable severity information indicating that an action 
should be taken. I am just speculating if we could have a more generic 
user-space application which took care of this instead of always writing 
a system-specific user-space application.

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

My current idea was just to treat existing regulator notifications as a 
event + severity. For example:

REGULATOR_EVENT_UNDER_VOLTAGE and
REGULATOR_EVENT_UNDER_VOLTAGE_WARN

would send netlink message with same event 'type' attribute 
(REGULATOR_EVENT_UNDER_VOLTAGE) but with different severity attributes:

REGULATOR_SEVERITY_ERROR Vs. REGULATOR_SEVERITY_WARN.

Still, as I wrote, I am not sure this is too important. I don't know if 
any 'action' decisions can be done based on currently existing 
ERROR/WARNING severities - and the netlink message API can be later 
extended by adding new attributes. So, please treat my message just as a 
fuel for thought - I'm sure you have better insight to the use of these 
notifications than I do :)

> > > +
> > > +/* 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.

Again, this might be my delusion :) Once upon a time I wrote a 
(downstream) netlink interface for sending certain specific purpose 
measurement results to the user-space. I have a faint memory from those 
days that it was possible to specify the multicast groups of interest at 
socket bind() - time. In this way "multiplexing" the messages would be 
done by kernel and user-space code could only listen the messages of 
interest? Maybe there are caveats I am not aware of though.

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

Yes. These were my points to consider - but you / Mark are free to use 
your judgement on if this makes any sense or not. I am not confident 
enough these are necessary "features" to really push for them.

In any case, I do like this ambition! Do you plan to write open-source 
user-space tool(s) for listening these events as well?

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~