Re: [PATCH v14 02/20] netlink: add new family to manage ULP_DDP enablement and stats

[Jakub Kicinski <kuba@kernel.org>]

On Wed,  6 Sep 2023 11:30:00 +0000 Aurelien Aptel wrote:
> Add a new netlink family to get/set ULP DDP capabilities on a network
> device and to retrieve statistics.
> 
> The messages use the genetlink infrastructure and are specified in a
> YAML file which was used to generate some of the files in this commit:
> 
> ./tools/net/ynl/ynl-gen-c.py --mode kernel \
>     --spec ./Documentation/netlink/specs/ulp_ddp.yaml --header \
>     -o net/core/ulp_ddp_gen_nl.h
> ./tools/net/ynl/ynl-gen-c.py --mode kernel \
>     --spec ./Documentation/netlink/specs/ulp_ddp.yaml --source \
>     -o net/core/ulp_ddp_gen_nl.c
> ./tools/net/ynl/ynl-gen-c.py --mode uapi \
>     --spec ./Documentation/netlink/specs/ulp_ddp.yaml --header \
>     > include/uapi/linux/ulp_ddp_nl.h  

Looks mostly good, but we lost dump support. TAL at netdev_nl_dev_get_dumpit(), 
the iteration is not that hard, I reckon we should support dumps.

Few extra nits below.

> +static struct ulp_ddp_netdev_caps *netdev_ulp_ddp_caps(struct net_device *dev)
> +{
> +#ifdef CONFIG_ULP_DDP
> +	return &dev->ulp_ddp_caps;
> +#else
> +	return NULL;
> +#endif

Are the ifdefs still needed? Can't we compile out all of the family code
if the config is not selected?

> +/* pre_doit */
> +int ulp_ddp_get_netdev(const struct genl_split_ops *ops,
> +		       struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct reply_data *data;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, ULP_DDP_A_DEV_IFINDEX))
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->ifindex = nla_get_u32(info->attrs[ULP_DDP_A_DEV_IFINDEX]);
> +	data->dev = netdev_get_by_index(genl_info_net(info),
> +					data->ifindex,
> +					&data->tracker,
> +					GFP_KERNEL);
> +

nit pointless empty line.

> +	if (!data->dev) {
> +		kfree(data);
> +		return -EINVAL;

ENOENT ? Maybe also 

		NL_SET_BAD_ATTR(info->extack, info->attrs[ULP_DDP_A_DEV_IFINDEX]) 
?

> +int ulp_ddp_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct reply_data *data = info->user_ptr[0];
> +	unsigned long wanted, wanted_mask;
> +	struct sk_buff *rsp;
> +	bool notify;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, ULP_DDP_A_DEV_WANTED) ||
> +	    GENL_REQ_ATTR_CHECK(info, ULP_DDP_A_DEV_WANTED_MASK))
> +		return -EINVAL;
> +
> +	rsp = genlmsg_new(reply_size(ULP_DDP_CMD_STATS), GFP_KERNEL);
> +	if (!rsp)
> +		return -EMSGSIZE;
> +
> +	wanted = nla_get_u64(info->attrs[ULP_DDP_A_DEV_WANTED]);
> +	wanted_mask = nla_get_u64(info->attrs[ULP_DDP_A_DEV_WANTED_MASK]);
> +
> +	ret = apply_bits(data, &wanted, &wanted_mask, info->extack);
> +	if (ret < 0)
> +		return ret;

leaks rsp

> +	notify = !!ret;
> +	ret = prepare_data(info, data, ULP_DDP_CMD_SET);
> +	if (ret)
> +		return ret;
> +
> +	ret = fill_data(rsp, data, ULP_DDP_CMD_SET, info->snd_portid, info->snd_seq, 0);

fill_date() can probably use genlmsg_iput() ?
-- 
pw-bot: cr