Re: [PATCH net-next v4] octeontx2-pf: Retain ethtool stats across interface down/up.

[Paolo Abeni <pabeni@redhat.com>]

On 5/14/26 11:16 AM, Anshumali Gaur wrote:
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 971fcab1c248..0ffcc613a4b2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -139,19 +139,21 @@ void otx2_get_stats64(struct net_device *netdev,
>  		      struct rtnl_link_stats64 *stats)
>  {
>  	struct otx2_nic *pfvf = netdev_priv(netdev);
> -	struct otx2_dev_stats *dev_stats;
> +	struct otx2_dev_stats *dev_stats, *old_stats;

Please respect the reverse christmas tree order above.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index ee623476e5ff..21fd1dd4d755 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -2154,6 +2154,8 @@ EXPORT_SYMBOL(otx2_open);
>  int otx2_stop(struct net_device *netdev)
>  {
>  	struct otx2_nic *pf = netdev_priv(netdev);
> +	struct otx2_dev_stats *old_stats = &pf->hw.old_stats;
> +	struct otx2_dev_stats *dev = &pf->hw.dev_stats;
>  	struct otx2_cq_poll *cq_poll = NULL;
>  	struct otx2_qset *qset = &pf->qset;
>  	int qidx, vec, wrk;

Same here. You need to split variable declaration and initialization.

> @@ -2162,6 +2164,27 @@ int otx2_stop(struct net_device *netdev)
>  	if (pf->flags & OTX2_FLAG_INTF_DOWN)
>  		return 0;
>  
> +	/* First stop packet Rx/Tx */
> +	otx2_rxtx_enable(pf, false);
> +
> +	/* Read final HW counters and accumulate */
> +	otx2_get_dev_stats(pf);
> +
> +	/* Accumulate old stats */
> +	old_stats->rx_bytes     += dev->rx_bytes;
> +	old_stats->rx_drops     += dev->rx_drops;
> +	old_stats->rx_bcast_frames      += dev->rx_bcast_frames;
> +	old_stats->rx_mcast_frames      += dev->rx_mcast_frames;
> +	old_stats->rx_ucast_frames      += dev->rx_ucast_frames;
> +	old_stats->rx_frames            += dev->rx_frames;
> +
> +	old_stats->tx_bytes             += dev->tx_bytes;
> +	old_stats->tx_drops             += dev->tx_drops;
> +	old_stats->tx_bcast_frames      += dev->tx_bcast_frames;
> +	old_stats->tx_mcast_frames      += dev->tx_mcast_frames;
> +	old_stats->tx_ucast_frames      += dev->tx_ucast_frames;
> +	old_stats->tx_frames            += dev->tx_frames;

Sashiko notes this is still racy, as napi can still run and update the
counters. You should move the stat collection just before freeing H/W
resources.

/P