Re: [PATCH net-next v3 8/8] net: xilinx: tsn: deliver endpoint RX frames to DSA user ports

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new RX demux in ep_dma_rx_cb()
  (drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c) relies on a DSA…

--

net: xilinx: tsn: deliver endpoint RX frames to DSA user ports

At probe time this commit allocates one METADATA_HW_PORT_MUX metadata dst
for each of the two MAC ports. In ep_dma_rx_cb() it then attaches the
matching dst to each RX frame based on its TUSER port_id, gated on
netdev_uses_dsa(), so DSA can deliver the frame to its swpN user port.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index bd8cd504cefb4..5103a3b4b018a 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> @@ -302,6 +311,10 @@ static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
>  	}
>  
>  	skb_put(skb, rx_len);
> +	if (netdev_uses_dsa(ndev)) {
> +		dst_hold(&ep->port_md[port_id]->dst);
> +		skb_dst_set(skb, &ep->port_md[port_id]->dst);
> +	}

[Severity: Low]
Can this branch be reached at all with the tree as submitted?

As far as I can tell, nothing under drivers/net/dsa/ binds
"xlnx,tsn-switch", and net/dsa/ has no TSN tagger. The only references
are the binding in xlnx,tsn-endpoint-ethernet-mac.yaml and this Kconfig
help text:

    The three-port switch is supported by the companion DSA driver.

Without that driver, nothing sets dsa_ptr on this netdev, so
netdev_uses_dsa() is always false here. That leaves three pieces of dead
code for now: this branch, the port_md[] allocation in
xlnx_tsn_ep_probe(), and ep_free_port_md(). It also means RX delivery to
swp1/swp2 can't be tested from this tree.

Will the companion DSA driver be posted along with this series?

The commit message also says:

    guarded by netdev_uses_dsa() so it runs only while a switch is bound
    as the conduit.

netdev_uses_dsa() checks a bit more than that:

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

dsa_port_set_tag_protocol() copies cpu_dp->rcv directly from the
tagger's ops.

Every frame accepted here carries a METADATA_HW_PORT_MUX dst, so
dsa_switch_rcv() always takes the md_dst branch and never calls
cpu_dp->rcv. The tagger still has to provide a non-NULL .rcv, though,
or this code never runs.

With a tagger that has no .rcv, such as none_ops in net/dsa/tag_none.c,
every frame would quietly stay on the conduit eth%d. tag_none.c warns
against using it for new switches, so this mainly applies to whatever
tagger the companion driver adds.

Could the commit message, or a comment here, say that the tagger must
provide an .rcv callback, even though it won't be called for these
frames?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-patches_v2_external-v3-0-e33d3960ca1e%40amd.com