Messages in this thread | | | Date | Thu, 7 Dec 2023 15:29:18 +0200 | Subject | Re: [PATCH v4] regulator: event: Add regulator netlink event support | From | Matti Vaittinen <> |
| |
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! ~~
| |