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

[Wei Fang]

> > 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?

netdev_txq_to_tc() will return 0 if not TC configured, I mean ndev->num_tc is
0.

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

Okay, this is a potential bug when we add TC_SETUP_QDISC_MQPRIO
support. I will fix it.

> 
> 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?

No, configure IPV to send packets according to the expected TC.

> 
> > +	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.

We added the 'dsa,member' property to the netc switch DT-binding doc, specifying
that the 'member' (switch index) value cannot be 0. Furthermore, the netc switch
driver also checks the switch index to ensure it is not 0. Therefore, the software index
and hardware index are equal.

> 
> > +
> > +	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?

I think either is fine, the final result is the same. I just wrote it this way for
convenience during implementation.