Re: [PATCH net-next v2 4/8] net: xilinx: tsn: parse endpoint DMA channel configuration

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] xlnx_tsn_ep_probe() and ep_parse_tx_queue_config() enforce two
  new mandatory device-tree constraints that the in-tree binding does…

--

net: xilinx: tsn: parse endpoint DMA channel configuration

This patch counts the TX and RX MCDMA channels listed in the endpoint's
"dma-names" property, allocates the conduit netdev with that many hardware
queues, and parses the "tx-queues-config" child node to map each logical TX
queue to a physical DMA channel.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 089f17a126f5..24025b1f6e66 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> @@ -69,15 +88,173 @@ static const struct ethtool_ops ep_ethtool_ops = {
>  	.get_drvinfo	= ep_get_drvinfo,
>  };
>  
> +/*
> + * Parse the "tx-queues-config" child of the EP node. The logical queue
> + * index is taken from the "queue<N>" node name, so the mapping does not
> + * depend on the order the child nodes appear in the device tree.
> + */
> +static int ep_parse_tx_queue_config(struct xlnx_tsn_ep *ep,
> +				    struct device_node *txcfg_np, u16 tx_present)
> +{
> +	DECLARE_BITMAP(queue_seen, TSN_MAX_TX_QUEUE) = {};
> +	DECLARE_BITMAP(chan_seen, TSN_MAX_TX_QUEUE) = {};
> +	unsigned int count = 0;
> +	int ret;
> +
> +	for_each_child_of_node_scoped(txcfg_np, qnode) {
> +		u32 chan, queue;
> +
> +		if (!str_has_prefix(qnode->name, "queue") ||
> +		    kstrtou32(qnode->name + strlen("queue"), 10, &queue) ||
> +		    queue >= ep->num_tx_queues)
> +			return dev_err_probe(ep->dev, -EINVAL,
> +					     "tx-config: invalid queue node %pOFn (have %u queues)\n",
> +					     qnode, ep->num_tx_queues);

[ ... ]

> +		ep->tx_dma_chan_map[queue] = chan;
> +		count++;
> +	}
> +
> +	if (count != ep->num_tx_queues)
> +		return dev_err_probe(ep->dev, -EINVAL,
> +				     "tx-config: described %u queues but expected %u\n",
> +				     count, ep->num_tx_queues);
> +
> +	return 0;
> +}

[ ... ]

>  static int xlnx_tsn_ep_probe(struct platform_device *pdev)
>  {
> +	u32 rx_chan_num[TSN_MAX_RX_QUEUE];
>  	struct device *dev = &pdev->dev;
> +	struct device_node *txcfg_np;
> +	u32 num_tx, num_rx, num_prio;
> +	struct device_node *ip_np;
>  	struct net_device *ndev;
>  	struct xlnx_tsn_ep *ep;
>  	u8 mac_addr[ETH_ALEN];
> +	u16 tx_present = 0;
>  	int ret;
> +	int i;
> +
> +	ret = ep_count_dma_queues(dev, &num_tx, &num_rx, &tx_present, rx_chan_num);
> +	if (ret)
> +		return ret;
> +
> +	ip_np = of_get_parent(dev->of_node);
> +	if (!ip_np)
> +		return dev_err_probe(dev, -EINVAL, "missing parent IP node\n");
> +
> +	ret = of_property_read_u32(ip_np, "xlnx,num-priorities", &num_prio);
> +	of_node_put(ip_np);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing xlnx,num-priorities\n");
> +
> +	if (num_tx != num_prio)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "TX channel count %u must equal num-priorities %u\n",
> +				     num_tx, num_prio);

[Severity: Medium]
Can this equality check reject device trees that the in-tree binding
accepts?  Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
describes the two properties completely independently:

  xlnx,num-priorities:
    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 2
    maximum: 8

      dma-names:
        minItems: 2
        maxItems: 16
        items:
          pattern: "^(tx|rx)_chan[0-7]$"

With that schema, dma-names = "tx_chan0", "rx_chan0" together with
xlnx,num-priorities = <2> validates, but xlnx_tsn_ep_probe() now fails with
"TX channel count 1 must equal num-priorities 2".  A 4 TX + 4 RX endpoint
under xlnx,num-priorities = <8> fails the same way.

The queue node checks in ep_parse_tx_queue_config() add a second constraint
that the schema does not express: exactly one queue<N> node per TX channel,
with dense indices 0..num_tx-1.  The schema has no required queue children
under tx-queues-config, so an empty

  tx-queues-config {
  };

is schema-valid and gets "tx-config: described 0 queues but expected N",
and sparse names such as queue0/queue1/queue2/queue7 with 4 TX channels get
"tx-config: invalid queue node queue7 (have 4 queues)".

Because the endpoint MAC is the DSA conduit referenced by
port@0 { ethernet = <&ep_mac>; }, a probe failure here also takes away the
three-port switch, not just the ep0 netdev.

Semantically these look like different things: xlnx,num-priorities is an
IP-wide traffic class count on the parent node, while dma-names describes
host-side MCDMA wiring on the endpoint node, and the binding's own example
uses an arbitrary queueN -> tx_chanM permutation.  num_prio is read here for
no other purpose than this comparison.

Should the binding encode the coupling (required queue children and a
dma-names/num-priorities relation), or should the driver accept every
topology the binding advertises?

The YAML is unchanged by this patch and by the remainder of the series, and
only the 8 TX / 8 RX, xlnx,num-priorities = <8> example in the binding
satisfies the driver, so dt_binding_check does not surface the mismatch.

Could the commit message body state these new device tree requirements?
Right now they appear only in the below-the-line v2 changelog:

  - Require one queue node per tx_chan and num-priorities equal to the TX
    channel count.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-patches_v2_external-v2-0-3a40babaff4c%40amd.com