Re: [PATCH 05/20] net: xilinx: tsn: bring up the endpoint MCDMA channels

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: xilinx: tsn: bring up the endpoint MCDMA channels

This patch requests the TX and RX MCDMA channels named in the device tree
when the endpoint interface is opened, gives each channel a ring of
preallocated SKB descriptor slots, resets the controller through tx_chan0,
and releases the channels on stop.  It also adds a XILINX_DMA dependency to
the Kconfig entry.

No Link: tags are present in the commit header.

[ ... ]

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
The channel names are re-invented here as a dense 0..N-1 sequence, but is
that what the binding in this series promises?

The dma-names schema in
Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
allows any subset of tx_chan0..15 / rx_chan0..15:

      dma-names:
        minItems: 2
        maxItems: 32
        items:
          pattern: "^(tx|rx)_chan(1[0-5]|[0-9])$"

and ep_count_dma_queues() only counts the prefixes, discarding the indices:

		if (str_has_prefix(name, "tx_chan"))
			tx++;
		else if (str_has_prefix(name, "rx_chan"))
			rx++;

So for a device tree that validates against the binding, for example

	dma-names = "tx_chan2", "tx_chan3", "rx_chan4", "rx_chan5";

probe succeeds, but ep_open()->ep_reset_dma_controller() asks for a channel
literally named "tx_chan0" and ep_init_dmaengine() asks for "tx_chan0" and
"tx_chan1".  Does dma_request_chan() not return -ENODEV in that case,
leaving the interface impossible to bring up?

There is a second mismatch in the other direction: the binding permits up to
16 tx_chan entries, while ep_count_dma_queues() rejects anything above
TSN_MAX_TX_QUEUE (8) with -EINVAL at probe.

Would it be better to use the actual strings from dma-names, or to narrow the
binding to the dense, at-most-8-TX form the driver implements?  Both the
binding pattern and these two functions are unchanged through the end of the
series.

[ ... ]

[Severity: Medium]
Can -EPROBE_DEFER reach userspace through ndo_open here?

dma_request_chan() forwards that code verbatim:

drivers/dma/dmaengine.c:dma_request_chan() {
	...
	if (PTR_ERR(chan) == -EPROBE_DEFER)
		return chan;
	...
}

and of_dma_request_slave_channel() returns -EPROBE_DEFER when no of_dma
controller has registered for the phandle yet.  Since probe() requests no DMA
channel, the netdev exists before the MCDMA provider is bound, and
XILINX_DMA is a tristate that can be loaded later:

  ip link set ep up -> __dev_open() -> ep_open() -> ep_reset_dma_controller()

__dev_open() returns ops->ndo_open()'s value unchanged, so would
SIOCSIFFLAGS report errno 517 to userspace?

ep_alloc_dma_chan() has the same path: return ERR_CAST(err_chan) is turned
into ret = PTR_ERR(...) by ep_init_dmaengine() and returned from ep_open().

Separately, dev_err_probe() is being used outside a probe path.  For
-EPROBE_DEFER it logs only at dev_dbg() level and overwrites the device's
deferred-probe reason string, so this open failure would be silent for the
operator.  Would dev_err()/netdev_err() be more appropriate in ndo_open?

Both ep_reset_dma_controller() and ep_open() are unchanged at the end of the
series.

[ ... ]