[PATCH 1/2] thermal: netlink: Add enum for mutlicast groups indexes

[PATCH 1/2] thermal: netlink: Add enum for mutlicast groups indexes

[Stanislaw Gruszka]

Use enum instead of hard-coded numbers for indexing multicast groups.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/thermal_netlink.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 21f00d73acb7..aca36c4ddbf3 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -13,9 +13,14 @@
 
 #include "thermal_core.h"
 
+enum thermal_genl_multicast_groups {
+	THERMAL_GENL_SAMPLING_GROUP = 0,
+	THERMAL_GENL_EVENT_GROUP = 1,
+};
+
 static const struct genl_multicast_group thermal_genl_mcgrps[] = {
-	{ .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
-	{ .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
+	[THERMAL_GENL_SAMPLING_GROUP] = { .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
+	[THERMAL_GENL_EVENT_GROUP]  = { .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
 };
 
 static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
@@ -95,7 +100,7 @@ int thermal_genl_sampling_temp(int id, int temp)
 
 	genlmsg_end(skb, hdr);
 
-	genlmsg_multicast(&thermal_gnl_family, skb, 0, 0, GFP_KERNEL);
+	genlmsg_multicast(&thermal_gnl_family, skb, 0, THERMAL_GENL_SAMPLING_GROUP, GFP_KERNEL);
 
 	return 0;
 out_cancel:
@@ -290,7 +295,7 @@ static int thermal_genl_send_event(enum thermal_genl_event event,
 
 	genlmsg_end(msg, hdr);
 
-	genlmsg_multicast(&thermal_gnl_family, msg, 0, 1, GFP_KERNEL);
+	genlmsg_multicast(&thermal_gnl_family, msg, 0, THERMAL_GENL_EVENT_GROUP, GFP_KERNEL);
 
 	return 0;
 
-- 
2.34.1

[PATCH 2/2] thermal: netlink: Add thermal_group_has_listeners() helper

[Stanislaw Gruszka]

Add a helper function to check if there are listeners for
thermal_gnl_family multicast groups.

For now use it to avoid unnecessary allocations and sending
thermal genl messages when there are no recipients.

In the future, in conjunction with (not yet implemented) notification
of change in the netlink socket group membership, this helper can be
used to open/close hardware interfaces based on the presence of
user space subscribers.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/thermal_netlink.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index aca36c4ddbf3..b4e758d22077 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -76,6 +76,11 @@ typedef int (*cb_t)(struct param *);
 
 static struct genl_family thermal_gnl_family;
 
+static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
+{
+	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
+}
+
 /************************** Sampling encoding *******************************/
 
 int thermal_genl_sampling_temp(int id, int temp)
@@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp)
 	struct sk_buff *skb;
 	void *hdr;
 
+	if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP))
+		return -ESRCH;
+
 	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOMEM;
@@ -280,6 +288,9 @@ static int thermal_genl_send_event(enum thermal_genl_event event,
 	int ret = -EMSGSIZE;
 	void *hdr;
 
+	if (!thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP))
+		return -ESRCH;
+
 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH 2/2] thermal: netlink: Add thermal_group_has_listeners() helper

[Daniel Lezcano]

Hi Stanislaw,

thanks for this optimization. One question below.

On Wed, Dec 27, 2023 at 03:00:57PM +0100, Stanislaw Gruszka wrote:
> Add a helper function to check if there are listeners for
> thermal_gnl_family multicast groups.
> 
> For now use it to avoid unnecessary allocations and sending
> thermal genl messages when there are no recipients.
> 
> In the future, in conjunction with (not yet implemented) notification
> of change in the netlink socket group membership, this helper can be
> used to open/close hardware interfaces based on the presence of
> user space subscribers.
> 
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/thermal/thermal_netlink.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> index aca36c4ddbf3..b4e758d22077 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -76,6 +76,11 @@ typedef int (*cb_t)(struct param *);
>  
>  static struct genl_family thermal_gnl_family;
>  
> +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
> +{
> +	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
> +}
> +
>  /************************** Sampling encoding *******************************/
>  
>  int thermal_genl_sampling_temp(int id, int temp)
> @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp)
>  	struct sk_buff *skb;
>  	void *hdr;
>  
> +	if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP))
> +		return -ESRCH;
> +

Do really want to return an error ? Shall we just bail out instead ?

[ ... ]

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] thermal: netlink: Add thermal_group_has_listeners() helper

[Stanislaw Gruszka]

Hi Daniel,

On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote:
> > +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
> > +{
> > +	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
> > +}
> > +
> >  /************************** Sampling encoding *******************************/
> >  
> >  int thermal_genl_sampling_temp(int id, int temp)
> > @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp)
> >  	struct sk_buff *skb;
> >  	void *hdr;
> >  
> > +	if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP))
> > +		return -ESRCH;
> > +
> 
> Do really want to return an error ? Shall we just bail out instead ?

I decided for error because thermal_notify_* are int functions and we
return error code for some other cases when the messages can not be
sent (i.e. alloc error).
Event if currently return value is ignored by all callers (FWICT),
error information could be used theoretically. 

If returning 0 is preferable/better, I can change that.

Regards
Stanislaw

Re: [PATCH 2/2] thermal: netlink: Add thermal_group_has_listeners() helper

[Daniel Lezcano]

On 27/12/2023 16:31, Stanislaw Gruszka wrote:
> Hi Daniel,
> 
> On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote:
>>> +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
>>> +{
>>> +	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
>>> +}
>>> +
>>>   /************************** Sampling encoding *******************************/
>>>   
>>>   int thermal_genl_sampling_temp(int id, int temp)
>>> @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp)
>>>   	struct sk_buff *skb;
>>>   	void *hdr;
>>>   
>>> +	if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP))
>>> +		return -ESRCH;
>>> +
>>
>> Do really want to return an error ? Shall we just bail out instead ?
> 
> I decided for error because thermal_notify_* are int functions and we
> return error code for some other cases when the messages can not be
> sent (i.e. alloc error).
> Event if currently return value is ignored by all callers (FWICT),
> error information could be used theoretically.
> 
> If returning 0 is preferable/better, I can change that.

The caller will have to handle the specific case if there is no 
listeners. There is an error if the message can not be sent but here 
there is no error related to that, just we don't send it. So having an 
error when there is nothing to do is not really an error.




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] thermal: netlink: Add thermal_group_has_listeners() helper

[Stanislaw Gruszka]

On Wed, Dec 27, 2023 at 04:40:31PM +0100, Daniel Lezcano wrote:
> On 27/12/2023 16:31, Stanislaw Gruszka wrote:
> > Hi Daniel,
> > 
> > On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote:
> > > > +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
> > > > +{
> > > > +	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
> > > > +}
> > > > +
> > > >   /************************** Sampling encoding *******************************/
> > > >   int thermal_genl_sampling_temp(int id, int temp)
> > > > @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp)
> > > >   	struct sk_buff *skb;
> > > >   	void *hdr;
> > > > +	if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP))
> > > > +		return -ESRCH;
> > > > +
> > > 
> > > Do really want to return an error ? Shall we just bail out instead ?
> > 
> > I decided for error because thermal_notify_* are int functions and we
> > return error code for some other cases when the messages can not be
> > sent (i.e. alloc error).
> > Event if currently return value is ignored by all callers (FWICT),
> > error information could be used theoretically.
> > 
> > If returning 0 is preferable/better, I can change that.
> 
> The caller will have to handle the specific case if there is no listeners.
> There is an error if the message can not be sent but here there is no error
> related to that, just we don't send it. So having an error when there is
> nothing to do is not really an error.

Ok, will change to 0 in v2.

Regards
Stanislaw