Received-SPF: pass (google.com: domain of linux-kernel+bounces-27389-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <e3e16af2-7f8d-4776-9726-f6282128a766@gmail.com>
Date: Tue, 16 Jan 2024 14:46:41 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] regulator: event: Add netlink command for event mask
To: Naresh Solanki <naresh.solanki@9elements.com>, broonie@kernel.org,
 Liam Girdwood <lgirdwood@gmail.com>
Cc: linux-kernel@vger.kernel.org
References: <20240116103131.413205-1-naresh.solanki@9elements.com>
Content-Language: en-US, en-GB
From: Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20240116103131.413205-1-naresh.solanki@9elements.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~