Re: [PATCH 09/20] net: dsa: tag_xlnx_tsn: add skeleton tag protocol

[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: tag_xlnx_tsn: add skeleton tag protocol

This adds DSA_TAG_PROTO_XLNX_TSN and a new tag driver for the AMD/Xilinx
TSN Endpoint Ethernet MAC, which carries port classification out of band
via a METADATA_HW_PORT_MUX metadata_dst instead of an in-band CPU tag.
The tagger is described as a no-op, with the PTP TX intercept deferred to
a later patch.

A few questions below.

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index d5e725b90d78..8ca48ce46ec1 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -221,4 +221,13 @@ config NET_DSA_TAG_YT921X
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  Motorcomm YT921x switches.
>  
> +config NET_DSA_TAG_XLNX_TSN

[Severity: Low]
Should this symbol sit between NET_DSA_TAG_VSC73XX_8021Q and
NET_DSA_TAG_XRS700X instead of after NET_DSA_TAG_YT921X?  The Makefile
change in this same patch does place it alphabetically:

  obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
  obj-$(CONFIG_NET_DSA_TAG_XLNX_TSN) += tag_xlnx_tsn.o
  obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o

so the two lists no longer correspond.  This ordering is still the same at
the end of the series.

> +	tristate "Tag driver for AMD/Xilinx TSN Endpoint Ethernet MAC"
> +	help
> +	  Say Y or M if you want to enable support for the AMD/Xilinx TSN
> +	  Endpoint Ethernet MAC IP. The IP does not insert an in-band CPU
> +	  tag; port classification is conveyed via metadata_dst attached on
> +	  RX by the EP MAC driver, and PTP TX frames are intercepted in the
> +	  xmit hook to be steered into the per-MAC PTP TX register window.

[Severity: Low]
The help text tells the user that "PTP TX frames are intercepted in the
xmit hook to be steered into the per-MAC PTP TX register window", but
xlnx_tsn_xmit() added by this same patch is a bare pass-through with no
ETH_P_1588 classification and no tagger_data->ptp_tx() call, and the file
does not include <linux/dsa/xlnx_tsn.h> yet.

Would it make sense to add the second sentence of the help text together
with the later patch that implements it?  The commit message itself says
"A later patch adds the PTP TX intercept", so at this revision menuconfig
describes behaviour that is not present.

The mismatch does go away at the end of the series, where
xlnx_tsn_xmit() gains:

  net/dsa/tag_xlnx_tsn.c:xlnx_tsn_xmit() {
      ...
      if (eth_hdr(skb)->h_proto != htons(ETH_P_1588) ||
          skb_vlan_tag_present(skb))
              return skb;

      tagger_data->ptp_tx(dp, skb);
      return NULL;
  }

> +
>  endif

[ ... ]

> diff --git a/net/dsa/tag_xlnx_tsn.c b/net/dsa/tag_xlnx_tsn.c
> new file mode 100644
> index 000000000000..de352aa3d9a8
> --- /dev/null
> +++ b/net/dsa/tag_xlnx_tsn.c
> @@ -0,0 +1,33 @@

[ ... ]

> +static struct sk_buff *xlnx_tsn_rcv(struct sk_buff *skb,
> +				    struct net_device *dev)
> +{
> +	kfree_skb(skb);
> +	return NULL;
> +}

[Severity: Low]
Could this drop be documented and made visible?  The commit message and
the Kconfig text describe a "no-op tag driver" that "has no tag bytes to
add or strip", but xlnx_tsn_rcv() destroys every skb it is handed, with no
drop reason, no counter and no comment.

The hook does look mandatory for the design, since the conduit RX
interception only happens when .rcv is non-NULL:

  include/net/dsa.h:netdev_uses_dsa() {
      return dev->dsa_ptr && dev->dsa_ptr->rcv;
  }

but that rationale is not stated anywhere in the file.

For frames that do carry the metadata dst attached by the EP MAC driver:

  drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c:ep_dma_rx_cb() {
      ...
      skb_dst_set_noref(skb, &ep->port_md[port_id]->dst);
      ...
  }

dsa_switch_rcv() takes the METADATA_HW_PORT_MUX branch and never reaches
this function.  Anything else falls into:

  net/dsa/tag.c:dsa_switch_rcv() {
      ...
      } else {
              nskb = cpu_dp->rcv(skb, dev);
      }
      ...
  }

so a conduit frame whose metadata dst was stripped, for example by a tc
ingress action on the conduit, disappears without showing up in
ip -s link, dropwatch or drop-reason tracing.  Would
kfree_skb_reason() plus a short comment explaining that the hook exists
only to make netdev_uses_dsa() true be preferable here?  This body is
unchanged at the end of the series.