Message-ID: <70cbf708-72a6-4413-8181-99441f676d57@gmail.com>
Date:   Thu, 7 Dec 2023 15:29:18 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Naresh Solanki <naresh.solanki@...ements.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] regulator: event: Add regulator netlink event support

On 12/7/23 14:39, Naresh Solanki wrote:
> Hi Matti,
> 
> 
> On Thu, 7 Dec 2023 at 13:42, Matti Vaittinen <mazziesaccount@...il.com> 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@...ements.com>
>>
>> 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! ~~

Powered by blists - more mailing lists