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 <[email protected]>
---
Chagnes in V4:
- Change REGULATOR_NETLINK_EVENTS from tristate to bool.
Chagnes in V3:
- Add Kconfig option REGULATOR_NETLINK_EVENTS for regulator netlink
support
- Add mechanism to avoid regulator name duplication
- Add UAPI header for regulator with define's for regulator events &
netlink.
---
drivers/regulator/Kconfig | 10 ++++
drivers/regulator/Makefile | 1 +
drivers/regulator/core.c | 19 ++++++-
drivers/regulator/event.c | 91 ++++++++++++++++++++++++++++++
drivers/regulator/regnl.h | 13 +++++
include/linux/regulator/consumer.h | 47 +--------------
include/uapi/regulator/regulator.h | 90 +++++++++++++++++++++++++++++
7 files changed, 224 insertions(+), 47 deletions(-)
create mode 100644 drivers/regulator/event.c
create mode 100644 drivers/regulator/regnl.h
create mode 100644 include/uapi/regulator/regulator.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f3ec24691378..550145f82726 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -56,6 +56,16 @@ config REGULATOR_USERSPACE_CONSUMER
If unsure, say no.
+config REGULATOR_NETLINK_EVENTS
+ bool "Enable support for receiving regulator events via netlink"
+ depends on NET
+ help
+ Enabling this option allows the kernel to broadcast regulator events using
+ the netlink mechanism. User-space applications can subscribe to these events
+ for real-time updates on various regulator events.
+
+ If unsure, say no.
+
config REGULATOR_88PG86X
tristate "Marvell 88PG86X voltage regulators"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b2b059b5ee56..46fb569e6be8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o irq_helpers.o
+obj-$(CONFIG_REGULATOR_NETLINK_EVENTS) += event.o
obj-$(CONFIG_OF) += of_regulator.o
obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4aa9ec8c22f3..a968dabb48f5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -33,6 +33,7 @@
#include "dummy.h"
#include "internal.h"
+#include "regnl.h"
static DEFINE_WW_CLASS(regulator_ww_class);
static DEFINE_MUTEX(regulator_nesting_mutex);
@@ -4854,7 +4855,23 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data)
{
/* call rdev chain first */
- return blocking_notifier_call_chain(&rdev->notifier, event, data);
+ int ret = blocking_notifier_call_chain(&rdev->notifier, event, data);
+
+ if (IS_REACHABLE(CONFIG_REGULATOR_NETLINK_EVENTS)) {
+ struct device *parent = rdev->dev.parent;
+ const char *rname = rdev_get_name(rdev);
+ char name[32];
+
+ /* Avoid duplicate debugfs directory names */
+ if (parent && rname == rdev->desc->name) {
+ snprintf(name, sizeof(name), "%s-%s", dev_name(parent),
+ rname);
+ rname = name;
+ }
+ reg_generate_netlink_event(rname, event);
+ }
+
+ return ret;
}
int _regulator_bulk_get(struct device *dev, int num_consumers,
diff --git a/drivers/regulator/event.c b/drivers/regulator/event.c
new file mode 100644
index 000000000000..0ec58f306b38
--- /dev/null
+++ b/drivers/regulator/event.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Regulator event over netlink
+ *
+ * Author: Naresh Solanki <[email protected]>
+ */
+
+#include <regulator/regulator.h>
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include "regnl.h"
+
+static unsigned int reg_event_seqnum;
+
+static const struct genl_multicast_group reg_event_mcgrps[] = {
+ { .name = REG_GENL_MCAST_GROUP_NAME, },
+};
+
+static struct genl_family reg_event_genl_family __ro_after_init = {
+ .module = THIS_MODULE,
+ .name = REG_GENL_FAMILY_NAME,
+ .version = REG_GENL_VERSION,
+ .maxattr = REG_GENL_ATTR_MAX,
+ .mcgrps = reg_event_mcgrps,
+ .n_mcgrps = ARRAY_SIZE(reg_event_mcgrps),
+};
+
+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);
+ 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;
+}
+
+static int __init reg_event_genetlink_init(void)
+{
+ return genl_register_family(®_event_genl_family);
+}
+
+static int __init reg_event_init(void)
+{
+ int error;
+
+ /* create genetlink for acpi event */
+ error = reg_event_genetlink_init();
+ if (error)
+ pr_warn("Failed to create genetlink family for reg event\n");
+
+ return 0;
+}
+
+fs_initcall(reg_event_init);
diff --git a/drivers/regulator/regnl.h b/drivers/regulator/regnl.h
new file mode 100644
index 000000000000..bcba16cc05cc
--- /dev/null
+++ b/drivers/regulator/regnl.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Regulator event over netlink
+ *
+ * Author: Naresh Solanki <[email protected]>
+ */
+
+#ifndef __REGULATOR_EVENT_H
+#define __REGULATOR_EVENT_H
+
+int reg_generate_netlink_event(const char *reg_name, u64 event);
+
+#endif
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 39b666b40ea6..4660582a3302 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -33,6 +33,7 @@
#include <linux/err.h>
#include <linux/suspend.h>
+#include <regulator/regulator.h>
struct device;
struct notifier_block;
@@ -84,52 +85,6 @@ struct regulator_dev;
#define REGULATOR_MODE_IDLE 0x4
#define REGULATOR_MODE_STANDBY 0x8
-/*
- * 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.
- */
-#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
-
/*
* Regulator errors that can be queried using regulator_get_error_flags
*
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 <[email protected]>
+ */
+
+#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.
+ */
+#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;
+};
+
+/* 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"
+
+#endif /* _UAPI_REGULATOR_H */
base-commit: 753e4d5c433da57da75dd4c3e1aececc8e874a62
--
2.41.0
On Tue, 05 Dec 2023 16:22:04 +0530, 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.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/1] regulator: event: Add regulator netlink event support
commit: 16e5ac127d8d18adf85fe5ba847d77b58d1ed418
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
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 <[email protected]>
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 <[email protected]>
> + */
> +
> +#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! ~~
Hi Matti,
On Thu, 7 Dec 2023 at 13:42, Matti Vaittinen <[email protected]> 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 <[email protected]>
>
> 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?
Yes making it atomic will be better.
>
> > + 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 <[email protected]>
> > + */
> > +
> > +#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)
Yes. I missed fixing the typo.
>
> > + */
> > +#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.
>
> > +
> > +/* 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.
>
> 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.
Regards,
Naresh
>
> > +
> > +#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! ~~
>
On 12/7/23 14:39, Naresh Solanki wrote:
> Hi Matti,
>
>
> On Thu, 7 Dec 2023 at 13:42, Matti Vaittinen <[email protected]> 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 <[email protected]>
>>
>> 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! ~~
On Thu, Dec 07, 2023 at 03:29:18PM +0200, Matti Vaittinen wrote:
> On 12/7/23 14:39, Naresh Solanki wrote:
> > 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.
That definitely seems doable (see also the discussion about emergency
shutdown) and also for other applications like providing a UI
representing what's going on. I can imagine figuring out which
regulators supply a SD card slot and looking for errors from them for
example. BMCs might have similar applications.
> 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 :)
It does feel like it's going to be up to the system/application how
severe an individual error will be.
> > 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.
I guess that'd overlap with the use case above with UI for something
like a SD card, filtering on the regulator.
> 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.
It does seem like they could be useful.