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.
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.
Current implementation is such that all events will be sent.+
+/* 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?
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 makesI'm aligned to make sure regulator netlink serves wider purpose &
no sense to you it's probably me who is overlooking something.
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.