[v2] regulator: event: Add regulator netlink event support
Commit Message
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>
---
drivers/regulator/Makefile | 1 +
drivers/regulator/core.c | 7 +-
drivers/regulator/event.c | 134 +++++++++++++++++++++++++++++++++++++
drivers/regulator/regnl.h | 8 +++
4 files changed, 149 insertions(+), 1 deletion(-)
create mode 100644 drivers/regulator/event.c
create mode 100644 drivers/regulator/regnl.h
base-commit: 753e4d5c433da57da75dd4c3e1aececc8e874a62
Comments
On Tue, Nov 21, 2023 at 10:13:30AM +0000, Naresh Solanki wrote:
> This commit introduces netlink event support to the regulator subsystem.
This looks a lot nicer, there's some feedback below but nothing
maissively substantial.
> +#ifdef CONFIG_NET
This is essentially the entire file - it's probably better to just put
the stub functions in the header and do the building with Kconfig.
> +static unsigned int reg_event_seqnum;
> +struct reg_genl_event {
> + char reg_name[15];
> + u64 event;
> +};
Given that this is going to get sent to userspace shouldn't it be in a
uapi header? Some of the other event types don't seem to do this
though... that might be an issue with those examples though. We'll
also make the event numbers into uapi so they should go in a uapi header
as well.
I'm also not clear on where the 15 byte limit comes from.
> +EXPORT_SYMBOL(reg_generate_netlink_event);
Everything else in regulator is EXPORT_SYMBOL_GPL() so this should be
too - though given that the only caller is _notifier_call_chain() which
is in the core does it need to be exported at all? I can't see a case
for anything calling it independently of that.
Hi Mark,
On Thu, 23 Nov 2023 at 18:27, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Nov 21, 2023 at 10:13:30AM +0000, Naresh Solanki wrote:
>
> > This commit introduces netlink event support to the regulator subsystem.
>
> This looks a lot nicer, there's some feedback below but nothing
> maissively substantial.
>
> > +#ifdef CONFIG_NET
>
> This is essentially the entire file - it's probably better to just put
> the stub functions in the header and do the building with Kconfig.
Sure.
As I understood, Will add an option REGULATOR_NETLINK to include the
event.c file during build.
>
> > +static unsigned int reg_event_seqnum;
> > +struct reg_genl_event {
> > + char reg_name[15];
> > + u64 event;
> > +};
>
> Given that this is going to get sent to userspace shouldn't it be in a
> uapi header? Some of the other event types don't seem to do this
> though... that might be an issue with those examples though. We'll
> also make the event numbers into uapi so they should go in a uapi header
> as well.
Will update next revision to include uapi header.
>
> I'm also not clear on where the 15 byte limit comes from.
I felt 15 characters would be sufficient for regulator names.
Would need your inputs to make sure here.
>
> > +EXPORT_SYMBOL(reg_generate_netlink_event);
>
> Everything else in regulator is EXPORT_SYMBOL_GPL() so this should be
> too - though given that the only caller is _notifier_call_chain() which
> is in the core does it need to be exported at all? I can't see a case
> for anything calling it independently of that.
Agree. The current scope is limited to regulator core so will remove the
export.
Regards,
Naresh
On Mon, Nov 27, 2023 at 04:07:45PM +0530, Naresh Solanki wrote:
> On Thu, 23 Nov 2023 at 18:27, Mark Brown <broonie@kernel.org> wrote:
> > I'm also not clear on where the 15 byte limit comes from.
> I felt 15 characters would be sufficient for regulator names.
> Would need your inputs to make sure here.
It does feel like it might be a bit tight - perhaps double it?
Hi Mark,
On Mon, 27 Nov 2023 at 19:45, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 04:07:45PM +0530, Naresh Solanki wrote:
> > On Thu, 23 Nov 2023 at 18:27, Mark Brown <broonie@kernel.org> wrote:
>
> > > I'm also not clear on where the 15 byte limit comes from.
>
> > I felt 15 characters would be sufficient for regulator names.
> > Would need your inputs to make sure here.
>
> It does feel like it might be a bit tight - perhaps double it?
Sure. Will change to 32.
Also there is a possibility of regulator name duplication.
And below mechanism is already used in debugfs:
const char *rname = rdev_get_name(rdev);
char name[NAME_MAX];
/* Avoid duplicate debugfs directory names */
if (parent && rname == rdev->desc->name) {
snprintf(name, sizeof(name), "%s-%s", dev_name(parent),
rname);
rname = name;
}
I need recommendations here.
Regards,
Naresh
@@ -5,6 +5,7 @@
obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o irq_helpers.o
+obj-$(CONFIG_REGULATOR) += event.o
obj-$(CONFIG_OF) += of_regulator.o
obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
@@ -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,11 @@ 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);
+
+ reg_generate_netlink_event(rdev_get_name(rdev), event);
+
+ return ret;
}
int _regulator_bulk_get(struct device *dev, int num_consumers,
new file mode 100644
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include "regnl.h"
+
+#ifdef CONFIG_NET
+static unsigned int reg_event_seqnum;
+struct reg_genl_event {
+ char reg_name[15];
+ u64 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"
+
+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);
+
+ pr_debug("%s -> %llx , ret: %x", reg_name, event, size);
+ if (size == -ESRCH)
+ pr_debug("multicast message sent, but nobody was listening...\n");
+ else if (size)
+ pr_debug("failed to send multicast genl message\n");
+ else
+ pr_debug("multicast message sent %d\n", reg_event_seqnum);
+
+ return 0;
+}
+EXPORT_SYMBOL(reg_generate_netlink_event);
+
+static int __init reg_event_genetlink_init(void)
+{
+ return genl_register_family(®_event_genl_family);
+}
+
+#else
+int reg_generate_netlink_event(const char *reg_name, u64 event)
+{
+ return 0;
+}
+EXPORT_SYMBOL(reg_generate_netlink_event);
+
+static int reg_event_genetlink_init(void)
+{
+ return -ENODEV;
+}
+#endif
+
+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);
new file mode 100644
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __REGULATOR_EVENT_H
+#define __REGULATOR_EVENT_H
+
+int reg_generate_netlink_event(const char *reg_name, u64 event);
+
+#endif