Re: [PATCH net-next v4 07/10] net: airoha: Support multiple net_devices for a single FE GDM port

[Jakub Kicinski <kuba@kernel.org>]

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: airoha: Support multiple net_devices for a single FE GDM port

This commit introduces support for multiple net_devices connected to the
same Frame Engine (FE) GDM port (GDM3 or GDM4) via an external hardware
arbiter on EN7581 or AN7583 SoCs, and adds a get_dev_from_sport callback
to differentiate between the net_devices attached to each port.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 786bc677af3c..0253919714e0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

> @@ -3188,14 +3257,23 @@ static int airoha_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
>  		struct airoha_gdm_port *port = eth->ports[i];
> -		struct airoha_gdm_dev *dev;
> +		int j;
>  
>  		if (!port)
>  			continue;
>  
> -		dev = port->dev;
> -		if (dev && dev->dev->reg_state == NETREG_REGISTERED)
> -			unregister_netdev(dev->dev);
> +		for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> +			struct airoha_gdm_dev *dev = port->devs[j];
> +			struct net_device *netdev;
> +
> +			if (!dev)
> +				continue;
> +
> +			netdev = dev->dev;
> +			of_node_put(netdev->dev.of_node);
> +			if (netdev->reg_state == NETREG_REGISTERED)
> +				unregister_netdev(netdev);
> +		}
>  		airoha_metadata_dst_free(port);
>  	}
>  	airoha_hw_cleanup(eth);

[Medium]
Is the ordering of of_node_put() and unregister_netdev() here correct?

airoha_alloc_gdm_device() now does:

	netdev->dev.of_node = of_node_get(np);

so the netdev holds its own reference to the of_node for its entire
registered lifetime. In this error unwind the of_node_put() runs before
unregister_netdev(), and netdev->dev.of_node is not cleared, so any
subsequent access to netdev->dev.of_node during unregistration (netdev
notifiers, sysfs, overlay-aware code) would be looking at a node whose
reference count has already been dropped.

Other drivers (e.g., drivers/net/ethernet/hisilicon/hns/hns_enet.c in
hns_nic_dev_remove()) put the node only after unregister_netdev().

Would swapping the order to call unregister_netdev() first, then
of_node_put(), be more appropriate here?

> @@ -3216,14 +3294,22 @@ static void airoha_remove(struct platform_device *pdev)
>  
>  	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
>  		struct airoha_gdm_port *port = eth->ports[i];
> -		struct airoha_gdm_dev *dev;
> +		int j;
>  
>  		if (!port)
>  			continue;
>  
> -		dev = port->dev;
> -		if (dev)
> -			unregister_netdev(dev->dev);
> +		for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> +			struct airoha_gdm_dev *dev = port->devs[j];
> +			struct net_device *netdev;
> +
> +			if (!dev)
> +				continue;
> +
> +			netdev = dev->dev;
> +			of_node_put(netdev->dev.of_node);
> +			unregister_netdev(netdev);
> +		}
>  		airoha_metadata_dst_free(port);
>  	}
>  	airoha_hw_cleanup(eth);

[Medium]
Same question in airoha_remove(): the of_node_put() fires before
unregister_netdev(), while netdev->dev.of_node is still assigned. Would
this be safer with the unregister first and the put afterwards?

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 3e93919a175c..207c75152fde 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h

[ ... ]

> @@ -542,14 +543,15 @@ struct airoha_qdma {
>  struct airoha_gdm_dev {
>  	struct airoha_gdm_port *port;
>  	struct airoha_qdma *qdma;
> -	struct net_device *dev;
>  	struct airoha_eth *eth;
> +	struct net_device *dev;
> +
> +	int nbq;
>  };
>  
>  struct airoha_gdm_port {
> -	struct airoha_gdm_dev *dev;
> +	struct airoha_gdm_dev *devs[AIROHA_MAX_NUM_GDM_DEVS];
>  	int id;
> -	int nbq;
>  
>  	struct airoha_hw_stats stats;

[Medium]
With multiple airoha_gdm_dev instances now sharing a single
airoha_gdm_port, do the per-netdev statistics need to become per-netdev
rather than per-port?

airoha_dev_get_stats64(), airoha_ethtool_get_mac_stats() and
airoha_ethtool_get_rmon_stats() all read from dev->port->stats, and the
underlying hardware counters at REG_FE_GDM_*_CNT(port->id) are per-GDM
port rather than per-netdev. When two netdevs share a port (for example
PCIE0/PCIE1 on EN7581 GDM3, or ETH/USB on GDM4), both netdevs would
report identical port-wide counters via rtnl_link_stats64 (ip -s link
show) and via the get_eth_mac_stats/get_rmon_stats ethtool callbacks,
and summing across the two netdevs would double-count.

Would it make sense to maintain per-netdev software counters in the
xmit/RX paths for the multi-dev case so each netdev reports its own
traffic through the standard uAPI?