Date:   Thu, 7 Dec 2023 10:12:26 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Naresh Solanki <naresh.solanki@...ements.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] regulator: event: Add regulator netlink event support

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@...ements.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++,
> +				 &reg_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(&reg_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@...ements.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! ~~

Powered by blists - more mailing lists