Re: [PATCH net-next] net: airoha: better handle MIBs for GDM ports with multiple devs attached

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> In the context of a GDM port that can have multiple net_devices attached
> (GDM3 and GDM4), the HW counters (MIBs) are global for the GDM port.
> This cause duplicated stats reported to the kernel for the related
> net_device.
> The SoC supports a split MIB feature where each counter is tracked based
> on the relevant HW channel (NBQ) to account for this scenario and
> provide a way to select the related counter on accessing the MIB
> registers.
> Enable this feature for GDM3 and GDM4 and configure the relevant HW
> channel before updating the HW stats to report correct HW counter to the
> kernel for the related interface.
> Move the stats struct from port to dev since HW counter are now specific
> to the network device instead of the GDM port. Refactor
> airoha_update_hw_stats() to take airoha_eth and airoha_gdm_port
> parameters since the function operates on the entire port.
> 
> Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 191 +++++++++++++++++--------------
>  drivers/net/ethernet/airoha/airoha_eth.h |   7 +-
>  2 files changed, 112 insertions(+), 86 deletions(-)
> 

[...]

> -	airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(port->id),
> +static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> +{
> +	struct airoha_gdm_port *port = dev->port;
> +	int i;
> +
> +	spin_lock(&port->stats_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> +		if (port->devs[i])
> +			airoha_dev_get_hw_stats(port->devs[i]);
> +	}
> +
> +	/* Reset MIB counters */
> +	airoha_fe_set(dev->eth, REG_FE_GDM_MIB_CLEAR(port->id),
>  		      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);

commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260611-airoha-eth-multi-serdes-stats-v1-1-42442ae42064%40kernel.org

- With split MIB enabled on GDM3/GDM4, airoha_dev_get_hw_stats() programs
  REG_FE_GDM_MIB_CFG with the device's nbq before reading per-channel
  counters, so the FE_TX_MIB_ID/FE_RX_MIB_ID selector clearly gates the
  counter reads.
  After the loop in airoha_update_hw_stats(), MIB_CFG retains the last
  iterated device's nbq, and a single airoha_fe_set() is issued against
  REG_FE_GDM_MIB_CLEAR. REG_FE_GDM_MIB_CLEAR exposes only RX/TX bits with no
  separate per-channel selector.
  If, as is typical of split-MIB designs, the clear honors the same
  FE_TX_MIB_ID/FE_RX_MIB_ID selector used for reads, will only the last
  iterated device's HW counters be cleared each cycle?
  Since airoha_dev_get_hw_stats() accumulates with += on each read, would
  the un-cleared HW counters of the other devs keep being re-added to
  dev->stats on every fetch, yielding inflated stats and eventual HW
  counter overflow?
  Would moving the MIB_CLEAR write inside the per-device loop, while that
  device's nbq is still selected in MIB_CFG, address this if the clear is
  indeed per-channel? Or can the datasheet confirm REG_FE_GDM_MIB_CLEAR is
  strictly global and unaffected by split mode?
  - This is the same report available in [0].
    REG_FE_GDM_MIB_CLEAR is global and it clears all nbq counters for the port.
 
Regards,
Lorenzo

[0] https://sashiko.dev/#/patchset/20260611-airoha-eth-multi-serdes-stats-v1-1-42442ae42064%40kernel.org
>  
> -	u64_stats_update_end(&port->stats.syncp);
> -	spin_unlock(&port->stats.lock);
> +	spin_unlock(&port->stats_lock);
>  }
>  
>  static int airoha_dev_open(struct net_device *netdev)
> @@ -2043,23 +2071,22 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
>  				   struct rtnl_link_stats64 *storage)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> -	struct airoha_gdm_port *port = dev->port;
>  	unsigned int start;
>  
>  	airoha_update_hw_stats(dev);
>  	do {
> -		start = u64_stats_fetch_begin(&port->stats.syncp);
> -		storage->rx_packets = port->stats.rx_ok_pkts;
> -		storage->tx_packets = port->stats.tx_ok_pkts;
> -		storage->rx_bytes = port->stats.rx_ok_bytes;
> -		storage->tx_bytes = port->stats.tx_ok_bytes;
> -		storage->multicast = port->stats.rx_multicast;
> -		storage->rx_errors = port->stats.rx_errors;
> -		storage->rx_dropped = port->stats.rx_drops;
> -		storage->tx_dropped = port->stats.tx_drops;
> -		storage->rx_crc_errors = port->stats.rx_crc_error;
> -		storage->rx_over_errors = port->stats.rx_over_errors;
> -	} while (u64_stats_fetch_retry(&port->stats.syncp, start));
> +		start = u64_stats_fetch_begin(&dev->stats.syncp);
> +		storage->rx_packets = dev->stats.rx_ok_pkts;
> +		storage->tx_packets = dev->stats.tx_ok_pkts;
> +		storage->rx_bytes = dev->stats.rx_ok_bytes;
> +		storage->tx_bytes = dev->stats.tx_ok_bytes;
> +		storage->multicast = dev->stats.rx_multicast;
> +		storage->rx_errors = dev->stats.rx_errors;
> +		storage->rx_dropped = dev->stats.rx_drops;
> +		storage->tx_dropped = dev->stats.tx_drops;
> +		storage->rx_crc_errors = dev->stats.rx_crc_error;
> +		storage->rx_over_errors = dev->stats.rx_over_errors;
> +	} while (u64_stats_fetch_retry(&dev->stats.syncp, start));
>  }
>  
>  static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
> @@ -2310,20 +2337,19 @@ static void airoha_ethtool_get_mac_stats(struct net_device *netdev,
>  					 struct ethtool_eth_mac_stats *stats)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> -	struct airoha_gdm_port *port = dev->port;
>  	unsigned int start;
>  
>  	airoha_update_hw_stats(dev);
>  	do {
> -		start = u64_stats_fetch_begin(&port->stats.syncp);
> -		stats->FramesTransmittedOK = port->stats.tx_ok_pkts;
> -		stats->OctetsTransmittedOK = port->stats.tx_ok_bytes;
> -		stats->MulticastFramesXmittedOK = port->stats.tx_multicast;
> -		stats->BroadcastFramesXmittedOK = port->stats.tx_broadcast;
> -		stats->FramesReceivedOK = port->stats.rx_ok_pkts;
> -		stats->OctetsReceivedOK = port->stats.rx_ok_bytes;
> -		stats->BroadcastFramesReceivedOK = port->stats.rx_broadcast;
> -	} while (u64_stats_fetch_retry(&port->stats.syncp, start));
> +		start = u64_stats_fetch_begin(&dev->stats.syncp);
> +		stats->FramesTransmittedOK = dev->stats.tx_ok_pkts;
> +		stats->OctetsTransmittedOK = dev->stats.tx_ok_bytes;
> +		stats->MulticastFramesXmittedOK = dev->stats.tx_multicast;
> +		stats->BroadcastFramesXmittedOK = dev->stats.tx_broadcast;
> +		stats->FramesReceivedOK = dev->stats.rx_ok_pkts;
> +		stats->OctetsReceivedOK = dev->stats.rx_ok_bytes;
> +		stats->BroadcastFramesReceivedOK = dev->stats.rx_broadcast;
> +	} while (u64_stats_fetch_retry(&dev->stats.syncp, start));
>  }
>  
>  static const struct ethtool_rmon_hist_range airoha_ethtool_rmon_ranges[] = {
> @@ -2343,8 +2369,7 @@ airoha_ethtool_get_rmon_stats(struct net_device *netdev,
>  			      const struct ethtool_rmon_hist_range **ranges)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> -	struct airoha_gdm_port *port = dev->port;
> -	struct airoha_hw_stats *hw_stats = &port->stats;
> +	struct airoha_hw_stats *hw_stats = &dev->stats;
>  	unsigned int start;
>  
>  	BUILD_BUG_ON(ARRAY_SIZE(airoha_ethtool_rmon_ranges) !=
> @@ -2357,7 +2382,7 @@ airoha_ethtool_get_rmon_stats(struct net_device *netdev,
>  	do {
>  		int i;
>  
> -		start = u64_stats_fetch_begin(&port->stats.syncp);
> +		start = u64_stats_fetch_begin(&dev->stats.syncp);
>  		stats->fragments = hw_stats->rx_fragment;
>  		stats->jabbers = hw_stats->rx_jabber;
>  		for (i = 0; i < ARRAY_SIZE(airoha_ethtool_rmon_ranges) - 1;
> @@ -2365,7 +2390,7 @@ airoha_ethtool_get_rmon_stats(struct net_device *netdev,
>  			stats->hist[i] = hw_stats->rx_len[i];
>  			stats->hist_tx[i] = hw_stats->tx_len[i];
>  		}
> -	} while (u64_stats_fetch_retry(&port->stats.syncp, start));
> +	} while (u64_stats_fetch_retry(&dev->stats.syncp, start));
>  }
>  
>  static int airoha_qdma_set_chan_tx_sched(struct net_device *netdev,
> @@ -3205,6 +3230,7 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
>  
>  	netdev->dev.of_node = of_node_get(np);
>  	dev = netdev_priv(netdev);
> +	u64_stats_init(&dev->stats.syncp);
>  	dev->port = port;
>  	dev->eth = eth;
>  	dev->nbq = nbq;
> @@ -3244,9 +3270,8 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  	if (!port)
>  		return -ENOMEM;
>  
> -	u64_stats_init(&port->stats.syncp);
> -	spin_lock_init(&port->stats.lock);
>  	port->id = id;
> +	spin_lock_init(&port->stats_lock);
>  	eth->ports[p] = port;
>  
>  	err = airoha_metadata_dst_alloc(port);
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 8f42973f9cf5..46b1c31939de 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -215,8 +215,6 @@ struct airoha_tx_irq_queue {
>  };
>  
>  struct airoha_hw_stats {
> -	/* protect concurrent hw_stats accesses */
> -	spinlock_t lock;
>  	struct u64_stats_sync syncp;
>  
>  	/* get_stats64 */
> @@ -554,6 +552,8 @@ struct airoha_gdm_dev {
>  
>  	u32 flags;
>  	int nbq;
> +
> +	struct airoha_hw_stats stats;
>  };
>  
>  struct airoha_gdm_port {
> @@ -561,7 +561,8 @@ struct airoha_gdm_port {
>  	int id;
>  	int users;
>  
> -	struct airoha_hw_stats stats;
> +	/* protect concurrent hw_stats accesses */
> +	spinlock_t stats_lock;
>  
>  	struct metadata_dst *dsa_meta[AIROHA_MAX_DSA_PORTS];
>  };
> 
> ---
> base-commit: c8459ee2fef502d6ef6c063751c33d9ac7943eab
> change-id: 20260611-airoha-eth-multi-serdes-stats-df2dc16c2dd6
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

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