Re: [PATCH net-next v4 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Feb 24, 2026 at 06:18:03PM +0530, Meghana Malladi wrote:
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.h b/drivers/net/ethernet/ti/icssg/icssg_qos.h
> index 653dbb57791d..bf84cc1b8282 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_qos.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
> @@ -57,4 +57,24 @@ void icssg_qos_link_up(struct net_device *ndev);
>  void icssg_qos_link_down(struct net_device *ndev);
>  int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  			   void *type_data);
> +static inline int icssg_qos_frag_size_min_to_add(u32 min_frag_size,
> +						 struct netlink_ext_ack *extack)

This function is poorly named.

It draws obvious inspiration from ethtool_mm_frag_size_min_to_add(),
which had the following meaning: "convert min_frag_size (input) to
addFragSize (output)". That's where the "min *to* add" comes from.

Your function does _not_ do that (nor is it used like that). It returns
negative failure, or 0. But that 0 is confusingly not an addFragSize
value, just "success".

> +{
> +	/* The minimum size of the non-final mPacket supported
> +	 * by the firmware is 64B and multiples of 64B.
> +	 */
> +	if (min_frag_size < 64) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "tx_min_frag_size must be at least 64 bytes");
> +		return -EINVAL;
> +	}
> +
> +	if (min_frag_size % (ETH_ZLEN + ETH_FCS_LEN)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "tx_min_frag_size must be a multiple of 64 bytes");

The min_frag_size as passed by user space represents a value that
doesn't include the FCS length (this is what the "4" in the addFragSize
formula excludes: "64 x (1 + addFragSize) - 4").

If your firmware takes a value including FCS, make the adjustment
privately to the driver, but accept values from user space that are 4
octets larger than what is programmed to the firmware. This allows the
same user space commands to have the same meaning on your hardware.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}