Re: [PATCH net-next v3 3/8] net: xilinx: tsn: add endpoint MAC driver skeleton

[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] xlnx_tsn_ep_probe() in
  drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c (line 80) open-codes…

--

net: xilinx: tsn: add endpoint MAC driver skeleton

This adds a platform driver for the "xlnx,tsn-ep-mac" endpoint MAC. The
endpoint MAC provides the conduit netdev for the TSN DSA switch. The driver
registers an eth%d netdev, sets its MAC address from DT or a random one,
and provides minimal netdev/ethtool ops where xmit just 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 0000000000000..609d444c68356
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> +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), "eth%d", NET_NAME_UNKNOWN, ether_setup);

[Severity: Low]
Should this be NET_NAME_ENUM rather than NET_NAME_UNKNOWN?

With the "eth%d" template the kernel chooses the number. The standard
helper for that case uses NET_NAME_ENUM:

net/ethernet/eth.c:alloc_etherdev_mqs() {
	return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_ENUM,
				ether_setup, txqs, rxqs);
}

The DSA user port fallback in dsa_user_create() in net/dsa/user.c also
pairs "eth%d" with NET_NAME_ENUM. The v2 changelog gave that as its reason.

With NET_NAME_UNKNOWN, reading /sys/class/net/ethX/name_assign_type for
this device returns -EINVAL:

net/core/net-sysfs.c:name_assign_type_show() {
	...
	if (READ_ONCE(ndev->name_assign_type) != NET_NAME_UNKNOWN)
		ret = netdev_show(dev, attr, buf, format_name_assign_type);
	...
}

Netdevs from alloc_etherdev() and the DSA user netdevs stacked on top of
this conduit report 1 there.

The v3 changelog says:

  "Register the netdev with the standard eth%d name (NET_NAME_UNKNOWN)"

That doesn't match what the standard helper does. Would alloc_etherdev()
work here instead? It would also drop the open-coded ether_setup.

Later in the series, "net: xilinx: tsn: parse endpoint DMA channel
configuration" changes this call to alloc_netdev_mqs(sizeof(*ep), "eth%d",
NET_NAME_UNKNOWN, ether_setup, ...) and still uses NET_NAME_UNKNOWN.
alloc_etherdev_mqs() would cover that multi-queue version.

> +	if (!ndev)
> +		return -ENOMEM;

[ ... ]

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