Re: [PATCH net-next V2 4/8] devlink: Expose port function commands to control RoCE

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 2 Dec 2022 10:26:18 +0200 Shay Drory wrote:
> Expose port function commands to enable / disable RoCE, this is used to
> control the port RoCE device capabilities.

> @@ -122,6 +122,9 @@ A user may set the hardware address of the function using
>  'devlink port function set hw_addr' command. For Ethernet port function
>  this means a MAC address.
>  
> +Users may also set the RoCE capability of the function using
> +'devlink port function set roce' command.

nit: use backticks (`) for better highlight?

> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 70191d96af89..830f8ffd69d1 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -658,11 +658,23 @@ enum devlink_resource_unit {
>  	DEVLINK_RESOURCE_UNIT_ENTRY,
>  };
>  
> +enum devlink_port_fn_attr_cap {
> +	DEVLINK_PORT_FN_ATTR_CAP_ROCE,
> +
> +	/* Add new caps above */
> +	__DEVLINK_PORT_FN_ATTR_CAPS_MAX,
> +	DEVLINK_PORT_FN_ATTR_CAPS_MAX = __DEVLINK_PORT_FN_ATTR_CAPS_MAX - 1

Is DEVLINK_PORT_FN_ATTR_CAPS_MAX actually needed?
This is a bit list, not an attribute list, don't copy the format 
of netlink attribute definition without a reason.

> +};
> +
> +#define DEVLINK_PORT_FN_ATTR_CAPS_VALID_MASK \
> +	(_BITUL(__DEVLINK_PORT_FN_ATTR_CAPS_MAX) - 1)

This does not belong in the uAPI. User space has to discover the mask
at runtime via a policy dump, anyway.

> +	[DEVLINK_PORT_FN_ATTR_CAPS] =
> +		NLA_POLICY_BITFIELD32(DEVLINK_PORT_FN_ATTR_CAPS_VALID_MASK),

Why is there _ATTR in the name of the CAPS mask?

>  };
>  
>  static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_ATTR_SELFTEST_ID_MAX + 1] = {
> @@ -692,6 +694,64 @@ devlink_sb_tc_index_get_from_attrs(struct devlink_sb *devlink_sb,
>  	return 0;
>  }
>  
> +#define DEVLINK_PORT_FN_CAP(_name) \
> +	BIT(DEVLINK_PORT_FN_ATTR_CAP_##_name)

No, just work harder to make the name concise :/
Not being able to grep or ctag uses of a value is a huge PITA during
code reviews.

> +#define DEVLINK_PORT_FN_SET_CAP(caps, cap, enable)	\
> +	do {						\
> +		typeof(cap) cap_ = (cap); \
> +		typeof(caps) caps_ = (caps); \
> +		(caps_)->selector |= cap_;	\
> +		if (enable)					\
> +			(caps_)->value |= cap_; \
> +	} while (0)

I think you can code this up as a function instead of a macro.