Re: [PATCH v4 net-next 09/14] net: dsa: add NETC switch tag support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: add NETC switch tag support

Adds support for the proprietary NXP NETC switch tag used in NXP NETC
switches version 4.3 or later. The tag has 3 types (Forward, To_Port,
To_Host) and supports a varying length between 6 and 14 bytes.

> diff --git a/net/dsa/tag_netc.c b/net/dsa/tag_netc.c
> new file mode 100644
> index 000000000000..addd41f7f1b6
> --- /dev/null
> +++ b/net/dsa/tag_netc.c
[ ... ]
> +static void *netc_fill_common_tp_tag(struct sk_buff *skb,
> +				     struct net_device *ndev,
> +				     u8 subtype, int tag_len)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(ndev);
> +	u16 queue = skb_get_queue_mapping(skb);
> +	u8 ipv = netdev_txq_to_tc(ndev, queue);

What happens here if the interface doesn't have traffic classes configured?

By default, netdev_txq_to_tc() returns -1. Because ipv is an unsigned 8-bit
integer, this casts to 255.

When this is later passed to FIELD_PREP(NETC_TAG_IPV, ipv) in
netc_fill_common_tag(), the macro blindly masks the variable, silently
forcing the IPV field to 7 (the maximum priority) for all default,
unconfigured traffic. Valid TC values greater than 7 would also be silently
truncated.

Does this code bypass the intended QoS mechanisms?

> +	void *tag;
> +
> +	skb_push(skb, tag_len);
> +	dsa_alloc_etype_header(skb, tag_len);
> +
> +	tag = dsa_etype_header_pos_tx(skb);
> +	memset(tag + NETC_TAG_CMN_LEN, 0, tag_len - NETC_TAG_CMN_LEN);
> +	netc_fill_common_tag(tag, NETC_TAG_TO_PORT, subtype,
> +			     dp->ds->index, dp->index, ipv);

Will this break communication for standard single-switch setups?

The driver passes dp->ds->index as the hardware switch ID here, which
defaults to 0 for the first switch in a DSA tree. However, on the receive
path, frames stamped with switch ID 0 are rejected.

If the physical switch hardware uses an ID greater than 0, frames will fail
the lookup in dsa_conduit_find_user() on receive because the software index
is 0.

> +
> +	return tag;
> +}
[ ... ]
> +static struct sk_buff *netc_rcv(struct sk_buff *skb,
> +				struct net_device *ndev)
> +{
> +	struct netc_tag_cmn *tag_cmn;
> +	int tag_len, sw_id, port;
[ ... ]
> +	if (tag_cmn->qos & NETC_TAG_QV)
> +		skb->priority = FIELD_GET(NETC_TAG_IPV, tag_cmn->qos);
> +
> +	sw_id = NETC_TAG_SWITCH & tag_cmn->switch_port;

This isn't a bug, but is there a reason to use a raw bitwise AND for sw_id
instead of using FIELD_GET(NETC_TAG_SWITCH, ...) to match how the port
field is extracted just below this?

> +	/* ENETC VEPA switch ID (0) is not supported yet */
> +	if (!sw_id) {
> +		dev_warn_ratelimited(&ndev->dev,
> +				     "VEPA switch ID is not supported yet\n");
> +
> +		return NULL;
> +	}
> +
> +	port = FIELD_GET(NETC_TAG_PORT, tag_cmn->switch_port);
> +	skb->dev = dsa_conduit_find_user(ndev, sw_id, port);
[ ... ]