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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]

> 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?

ack, I will fix it in v5.

> 
> > @@ -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?

ack, I will fix it in v5.

> 
> > 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?

We have already a separated patch to address this issue that is not part of the
series. I will add it to the v5 in the next iteration.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]