Re: [patch net-next 3/8] devlink: send notifications only if there are listeners

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/15/2023 6:17 AM, Jiri Pirko wrote:
> +static inline bool devlink_nl_notify_need(struct devlink *devlink)
> +{
> +	return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
> +				  DEVLINK_MCGRP_CONFIG);
> +}
> +

I assume following changes will add more checks here to filter here so
it doesn't make sense to call this "devlink_has_listeners"? I feel like
the devlink_nl_notify_need is a bit weird of a way to phrase this.

I don't have a strong objection to the name overall, just found it a bit
odd.

>  /* Notify */
>  void devlink_notify_register(struct devlink *devlink);
>  void devlink_notify_unregister(struct devlink *devlink);
> diff --git a/net/devlink/health.c b/net/devlink/health.c
> index 695df61f8ac2..93eae8b5d2d3 100644
> --- a/net/devlink/health.c
> +++ b/net/devlink/health.c
> @@ -496,6 +496,9 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
>  	WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
>  	ASSERT_DEVLINK_REGISTERED(devlink);
>  
> +	if (!devlink_nl_notify_need(devlink))
> +		return;
> +
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)
>  		return;
> diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
> index 9d080ac1734b..45b36975ee6f 100644
> --- a/net/devlink/linecard.c
> +++ b/net/devlink/linecard.c
> @@ -136,7 +136,7 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
>  	WARN_ON(cmd != DEVLINK_CMD_LINECARD_NEW &&
>  		cmd != DEVLINK_CMD_LINECARD_DEL);
>  
> -	if (!__devl_is_registered(devlink))
> +	if (!__devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
>  		return;
>  

A bunch of callers are checking both if its registered and a
notification is needed, does it make sense to combine this? Or I guess
at least a few places are notifying of removal after its no longer
registered, so we can't inline the devl_is_registered into the
devlink_nl_notify_need. Probably more clear to keep it separate too.

Ok.