[v2] regulator: event: Add netlink command for event mask

  Add netlink command to enable perticular event(s) broadcasting instead
of all regulator events.

Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>

..
Changes in v2:
- Update attribute to REG_GENL_ATTR_SET_EVENT_MASK
---
 drivers/regulator/event.c          | 28 ++++++++++++++++++++++++++++
 include/uapi/regulator/regulator.h |  1 +
 2 files changed, 29 insertions(+)


base-commit: 94cc3087aac4103c33c6da84c092301afd783200
  

Hi Naresh,

Thanks for working on this! :)

On 1/16/24 12:31, Naresh Solanki wrote:
> Add netlink command to enable perticular event(s) broadcasting instead
> of all regulator events.

I think this mechanism for limiting events being forwarded to the 
listener is worthy. My original idea was to utilize the netlink 
multicast groups for this so that the regulator core would register 
multiple multicast groups for this family. User would then listen only 
the groups he is interested, and multiplexing the messages would be done 
by netlink/socket code.

Problem(?) of the approach you propose here is that the event filtering 
is global for all users. If multicast groups were used, this filtering 
would be done per listener socket basis. I'm not sure if that would be 
needed though, but somehow I feel it would be more usable for different 
user-land appliactions (cost being the increased complexity though).

Eg, I am thinking users could enable receiving multicasts for events 
they like using:

setsockopt(..., SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, ..., ...)

Do you think allowing setting the 'filtering' this way per socket would 
work or be beneficial?

> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> 
> ...
> Changes in v2:
> - Update attribute to REG_GENL_ATTR_SET_EVENT_MASK
> ---
>   drivers/regulator/event.c          | 28 ++++++++++++++++++++++++++++
>   include/uapi/regulator/regulator.h |  1 +
>   2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/regulator/event.c b/drivers/regulator/event.c
> index ea3bd49544e8..181d16f54a21 100644
> --- a/drivers/regulator/event.c
> +++ b/drivers/regulator/event.c
> @@ -14,17 +14,41 @@
>   
>   static atomic_t reg_event_seqnum = ATOMIC_INIT(0);
>   
> +static u64 event_mask;
> +
>   static const struct genl_multicast_group reg_event_mcgrps[] = {
>   	{ .name = REG_GENL_MCAST_GROUP_NAME, },
>   };
>   
> +static int reg_genl_cmd_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	if (info->attrs[REG_GENL_ATTR_SET_EVENT_MASK]) {
> +		event_mask = nla_get_u64(info->attrs[REG_GENL_ATTR_SET_EVENT_MASK]);

If we go with just a global event_mask (not per listener) event 
filtering, then we might need protection/barrier for this write of a 
64bit value(?)


> +		pr_info("event_mask -> %llx", event_mask);

I would drop this print, or by very least, make it dbg.

> +		return 0;
> +	}
> +	pr_warn("Unknown attribute.");

I would make this dbg as well.

> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct genl_small_ops reg_genl_ops[] = {
> +	{
> +		.cmd = REG_GENL_CMD_EVENT,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = reg_genl_cmd_doit,
> +	}
> +};
> +
>   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,
> +	.small_ops	= reg_genl_ops,
> +	.n_small_ops	= ARRAY_SIZE(reg_genl_ops),
>   	.mcgrps = reg_event_mcgrps,
>   	.n_mcgrps = ARRAY_SIZE(reg_event_mcgrps),
> +	.resv_start_op = __REG_GENL_CMD_MAX,
>   };
>   
>   int reg_generate_netlink_event(const char *reg_name, u64 event)
> @@ -35,6 +59,9 @@ int reg_generate_netlink_event(const char *reg_name, u64 event)
>   	void *msg_header;
>   	int size;
>   

Barrier/locking here too?

> +	if (!(event_mask & event))
> +		return 0; > +
>   	/* allocate memory */
>   	size = nla_total_size(sizeof(struct reg_genl_event)) +
>   	    nla_total_size(0);
> @@ -73,6 +100,7 @@ int reg_generate_netlink_event(const char *reg_name, u64 event)
>   
>   static int __init reg_event_genetlink_init(void)
>   {
> +	event_mask = 0;
>   	return genl_register_family(&reg_event_genl_family);
>   }
>   
> diff --git a/include/uapi/regulator/regulator.h b/include/uapi/regulator/regulator.h
> index 71bf71a22e7f..2a0af512b61c 100644
> --- a/include/uapi/regulator/regulator.h
> +++ b/include/uapi/regulator/regulator.h
> @@ -69,6 +69,7 @@ struct reg_genl_event {
>   enum {
>   	REG_GENL_ATTR_UNSPEC,
>   	REG_GENL_ATTR_EVENT,	/* reg event info needed by user space */
> +	REG_GENL_ATTR_SET_EVENT_MASK,	/* reg event mask */
>   	__REG_GENL_ATTR_MAX,
>   };
>   
> 
> base-commit: 94cc3087aac4103c33c6da84c092301afd783200

Yours,
	-- Matti
  

On Tue, Jan 16, 2024 at 02:46:41PM +0200, Matti Vaittinen wrote:

> Do you think allowing setting the 'filtering' this way per socket would work
> or be beneficial?

I haven't thought about it enough to say anything about the working bit
but it does seem like it could be useful (eg, a UI might be interested
in more events than something just looking for critical errors).
  

On Tue, Jan 16, 2024 at 02:46:41PM +0200, Matti Vaittinen wrote:
> On 1/16/24 12:31, Naresh Solanki wrote:

> > Add netlink command to enable perticular event(s) broadcasting instead

> 
> I think this mechanism for limiting events being forwarded to the listener
> is worthy. My original idea was to utilize the netlink multicast groups for
> this so that the regulator core would register multiple multicast groups for
> this family. User would then listen only the groups he is interested, and
> multiplexing the messages would be done by netlink/socket code.

> Problem(?) of the approach you propose here is that the event filtering is
> global for all users. If multicast groups were used, this filtering would be
> done per listener socket basis. I'm not sure if that would be needed though,
> but somehow I feel it would be more usable for different user-land
> appliactions (cost being the increased complexity though).

Thinking about this some more I do think that global filtering like
the current patch would at least need some sort of permission check,
otherwise just any random process can disrupt everyone's monitoring.  
Per socket filtering does seem like the way to go.
  

Hi

On Thu, 18 Jan 2024 at 19:50, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 16, 2024 at 02:46:41PM +0200, Matti Vaittinen wrote:
> > On 1/16/24 12:31, Naresh Solanki wrote:
>
> > > Add netlink command to enable perticular event(s) broadcasting instead
>
> >
> > I think this mechanism for limiting events being forwarded to the listener
> > is worthy. My original idea was to utilize the netlink multicast groups for
> > this so that the regulator core would register multiple multicast groups for
> > this family. User would then listen only the groups he is interested, and
> > multiplexing the messages would be done by netlink/socket code.
>
> > Problem(?) of the approach you propose here is that the event filtering is
> > global for all users. If multicast groups were used, this filtering would be
> > done per listener socket basis. I'm not sure if that would be needed though,
> > but somehow I feel it would be more usable for different user-land
> > appliactions (cost being the increased complexity though).
>
> Thinking about this some more I do think that global filtering like
> the current patch would at least need some sort of permission check,
> otherwise just any random process can disrupt everyone's monitoring.
> Per socket filtering does seem like the way to go.
Agree. Will work on it & after validation will post the CL.
This change can be abandoned for now.

Regards,
Naresh