lkml.org 
[lkml]   [2023]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4] regulator: event: Add regulator netlink event support
From
On 12/7/23 14:39, Naresh Solanki wrote:
> Hi Matti,
>
>
> On Thu, 7 Dec 2023 at 13:42, Matti Vaittinen <mazziesaccount@gmail.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@9elements.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! ~~

\
 
 \ /
  Last update: 2023-12-07 14:30    [W:0.077 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site