Messages in this thread | | | Date | Thu, 7 Dec 2023 10:12:26 +0200 | Subject | Re: [PATCH v4] regulator: event: Add regulator netlink event support | From | Matti Vaittinen <> |
| |
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!
...
> + > +int reg_generate_netlink_event(const char *reg_name, u64 event) > +{ > + struct sk_buff *skb; > + struct nlattr *attr; > + struct reg_genl_event *edata; > + void *msg_header; > + int size; > + > + /* allocate memory */ > + size = nla_total_size(sizeof(struct reg_genl_event)) + > + nla_total_size(0); > + > + skb = genlmsg_new(size, GFP_ATOMIC); > + if (!skb) > + return -ENOMEM; > + > + /* add the genetlink message header */ > + msg_header = genlmsg_put(skb, 0, reg_event_seqnum++, > + ®_event_genl_family, 0, > + REG_GENL_CMD_EVENT);
Should the reg_event_seqnum++ be atomic or is access somehow serialized?
> + if (!msg_header) { > + nlmsg_free(skb); > + return -ENOMEM; > + } > + > + /* fill the data */ > + attr = nla_reserve(skb, REG_GENL_ATTR_EVENT, sizeof(struct reg_genl_event)); > + if (!attr) { > + nlmsg_free(skb); > + return -EINVAL; > + } > + > + edata = nla_data(attr); > + memset(edata, 0, sizeof(struct reg_genl_event)); > + > + strscpy(edata->reg_name, reg_name, sizeof(edata->reg_name)); > + edata->event = event; > + > + /* send multicast genetlink message */ > + genlmsg_end(skb, msg_header); > + size = genlmsg_multicast(®_event_genl_family, skb, 0, 0, GFP_ATOMIC); > + > + return size; > +} > +
...
> diff --git a/include/uapi/regulator/regulator.h b/include/uapi/regulator/regulator.h > new file mode 100644 > index 000000000000..d2b5612198b6 > --- /dev/null > +++ b/include/uapi/regulator/regulator.h > @@ -0,0 +1,90 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* > + * Regulator uapi header > + * > + * Author: Naresh Solanki <Naresh.Solanki@9elements.com> > + */ > + > +#ifndef _UAPI_REGULATOR_H > +#define _UAPI_REGULATOR_H > + > +#ifdef __KERNEL__ > +#include <linux/types.h> > +#else > +#include <stdint.h> > +#endif > + > +/* > + * Regulator notifier events. > + * > + * UNDER_VOLTAGE Regulator output is under voltage. > + * OVER_CURRENT Regulator output current is too high. > + * REGULATION_OUT Regulator output is out of regulation. > + * FAIL Regulator output has failed. > + * OVER_TEMP Regulator over temp. > + * FORCE_DISABLE Regulator forcibly shut down by software. > + * VOLTAGE_CHANGE Regulator voltage changed. > + * Data passed is old voltage cast to (void *). > + * DISABLE Regulator was disabled. > + * PRE_VOLTAGE_CHANGE Regulator is about to have voltage changed. > + * Data passed is "struct pre_voltage_change_data" > + * ABORT_VOLTAGE_CHANGE Regulator voltage change failed for some reason. > + * Data passed is old voltage cast to (void *). > + * PRE_DISABLE Regulator is about to be disabled > + * ABORT_DISABLE Regulator disable failed for some reason > + * > + * NOTE: These events can be OR'ed together when passed into handler. > + */ > + > +#define REGULATOR_EVENT_UNDER_VOLTAGE 0x01 > +#define REGULATOR_EVENT_OVER_CURRENT 0x02 > +#define REGULATOR_EVENT_REGULATION_OUT 0x04 > +#define REGULATOR_EVENT_FAIL 0x08 > +#define REGULATOR_EVENT_OVER_TEMP 0x10 > +#define REGULATOR_EVENT_FORCE_DISABLE 0x20 > +#define REGULATOR_EVENT_VOLTAGE_CHANGE 0x40 > +#define REGULATOR_EVENT_DISABLE 0x80 > +#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE 0x100 > +#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE 0x200 > +#define REGULATOR_EVENT_PRE_DISABLE 0x400 > +#define REGULATOR_EVENT_ABORT_DISABLE 0x800 > +#define REGULATOR_EVENT_ENABLE 0x1000 > +/* > + * Following notifications should be emitted only if detected condition > + * is such that the HW is likely to still be working but consumers should > + * take a recovery action to prevent problems esacalating into errors.
It's easier to spot my own typos when someone throws them at my face :) Maybe fix the 'esacalating' while shuffling these? (don't know if it's preferred to first move everything and only do typofix as own patch - in which case it definitely does not need to be done as a part of this series. Just commented on this as I noticed it now)
> + */ > +#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?
> + > +/* 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?
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.
> + > +#endif /* _UAPI_REGULATOR_H */ > > base-commit: 753e4d5c433da57da75dd4c3e1aececc8e874a62
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |