Re: [PATCH 03/20] net: xilinx: tsn: add endpoint MAC driver skeleton

[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: xilinx: tsn: add endpoint MAC driver skeleton

This adds a platform driver for the AMD/Xilinx TSN endpoint MAC
("xlnx,tsn-ep-mac"), registering a netdev named "ep" with minimal netdev
and ethtool ops.  There is no data path yet: ndo_open only starts the
queues and ndo_start_xmit drops frames.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> new file mode 100644
> index 000000000000..9b556edf5423
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> +static const struct net_device_ops ep_netdev_ops = {
> +	.ndo_open		= ep_open,
> +	.ndo_stop		= ep_stop,
> +	.ndo_start_xmit		= ep_start_xmit,
> +	.ndo_validate_addr	= eth_validate_addr,
> +	.ndo_set_mac_address	= eth_mac_addr,
> +};

[ ... ]

> +static int xlnx_tsn_ep_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct net_device *ndev;
> +	struct xlnx_tsn_ep *ep;
> +	u8 mac_addr[ETH_ALEN];
> +	int ret;
> +
> +	ndev = alloc_netdev(sizeof(*ep), "ep", NET_NAME_UNKNOWN, ether_setup);
> +	if (!ndev)
> +		return -ENOMEM;

[Severity: Medium]
Can a second TSN IP instance ever probe successfully with this fixed name?
"ep" carries no "%d", so the name is not auto-uniquified and
register_netdev() ends up returning -EEXIST:

net/core/dev.c:dev_prep_valid_name() {
	...
	if (netdev_name_in_use(net, want_name))
		return -dup_errno;
	...
}

reached from register_netdev() -> dev_get_valid_name(), which passes
dup_errno = EEXIST.

The binding added earlier in the series places the ep-mac@ child under the
soft-IP wrapper node and puts no singleton restriction on the wrapper:

Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml:
patternProperties:
  "^ep-mac@":
    type: object

So an FPGA design with two "xlnx,tsn-endpoint-ethernet-mac-3.0" instances is
a valid device tree, and the second xlnx_tsn_ep_probe() would fail at
register_netdev(), free the netdev, and leave that IP's DSA switch without
its conduit.  The same collision happens if any unrelated interface already
holds the very generic global name "ep", for example after a udev rename.
Would something like "ep%d" be preferable here?

Also, since the name is chosen deterministically by the driver rather than
being of unknown origin, should this be NET_NAME_PREDICTABLE instead of
NET_NAME_UNKNOWN?  include/uapi/linux/netdevice.h describes
NET_NAME_UNKNOWN as "unknown origin (not exposed to userspace)", so as
written sysfs name_assign_type hides the provenance from udev.

This still reads the same way at the end of the series, where the call
becomes alloc_netdev_mqs(sizeof(*ep), "ep", NET_NAME_UNKNOWN, ether_setup,
num_tx, num_rx), so no later patch changes it.

> +
> +	SET_NETDEV_DEV(ndev, dev);
> +	ndev->netdev_ops = &ep_netdev_ops;
> +	ndev->ethtool_ops = &ep_ethtool_ops;
> +	ndev->features = NETIF_F_SG;
> +
> +	ep = netdev_priv(ndev);
> +	ep->ndev = ndev;
> +	ep->dev = dev;
> +
> +	ep->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ep->regs)) {
> +		ret = PTR_ERR(ep->regs);
> +		goto err_free_ndev;
> +	}
> +
> +	ret = of_get_mac_address(dev->of_node, mac_addr);
> +	if (ret == -EPROBE_DEFER) {
> +		goto err_free_ndev;
> +	} else if (!ret && is_valid_ether_addr(mac_addr)) {
> +		eth_hw_addr_set(ndev, mac_addr);
> +	} else {
> +		eth_hw_addr_random(ndev);
> +		dev_info(dev, "no valid MAC in DT, using random address %pM\n",
> +			 ndev->dev_addr);
> +	}

[Severity: Low]
Is ep->regs meant to be used by this patch?  Grepping
drivers/net/ethernet/xilinx/tsn/ at the end of the series, ep->regs is only
ever assigned and error-checked in xlnx_tsn_ep_probe() and never
dereferenced by any read or write, so the mapped window is dead state.
Would it be better to map it in the patch that first programs it?

Related question about the address handling: the station address is only
ever set in software here (eth_hw_addr_set(), or eth_hw_addr_random() when
the DT has no valid address), and ndo_set_mac_address is the generic
eth_mac_addr(), which just updates ndev->dev_addr.  If the endpoint MAC
holds its own unicast address or filter words in that window, does the
address advertised to userspace end up permanently diverging from what the
bitstream or bootloader left in the hardware, and does a later

	ip link set ep address ...

ever reach the device?

There is some counter-evidence that no hardware filtering is intended at
all: the driver implements neither ndo_set_rx_mode nor any promiscuity
handling, while DSA adds user-port addresses to the conduit via
dev_uc_add() in net/dsa/user.c and net/dsa/port.c.  If that is the design,
could the commit message say so, and could the unused register mapping be
dropped for now?

Cross-instance finding from sashiko-gemini (e8e82d52a83540aba64fbedc360b0969d77529c63fbeb8f2b749e3809191d7cb):
[Severity: Medium]
Dropped packets are freed using `dev_kfree_skb()`, which aliases to `consume_skb()` and incorrectly signals successful transmission to tracepoints.