Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY drivers

[Mateusz Polchlopek <mateusz.polchlopek@intel.com>]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   include/linux/phy.h     | 10 ++++++++++
>   net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
>   net/ethtool/stats.c     | 19 +++++++++++++++++++
>   3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 563c46205685..523195c724b5 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1090,6 +1090,16 @@ struct phy_driver {
>   	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
>   
>   	/* Get statistics from the PHY using ethtool */
> +	/**
> +	 * @get_phy_stats: Get well known statistics.
> +	 * @get_link_stats: Get well known link statistics.
> +	 * The input structure is not zero-initialized and the implementation
> +	 * must only set statistics which are actually collected by the device.
> +	 */
> +	void (*get_phy_stats)(struct phy_device *dev,
> +			      struct ethtool_eth_phy_stats *eth_stats);
> +	void (*get_link_stats)(struct phy_device *dev,
> +			       struct ethtool_link_ext_stats *link_stats);
>   	/** @get_sset_count: Number of statistic counters */
>   	int (*get_sset_count)(struct phy_device *dev);
>   	/** @get_strings: Names of the statistic counters */
> diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
> index 34d76e87847d..8d3a38cc3d48 100644
> --- a/net/ethtool/linkstate.c
> +++ b/net/ethtool/linkstate.c
> @@ -94,6 +94,27 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
>   	return 0;
>   }
>   
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev)
> +		return;
> +
> +	if (dev->phydev)
> +		data->link_stats.link_down_events =
> +			READ_ONCE(dev->phydev->link_down_events);

Maybe silly questions but... Why to use dev->phydev when you created
*phydev pointer at the top of the function? Moreover, is that `if`
necessary? I understand that it will be always true as negative
scenario you handle in the first if? Or do I misunderstand something?

> +
> +	if (!phydev->drv || !phydev->drv->get_link_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_link_stats(phydev, &data->link_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>   static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>   				  struct ethnl_reply_data *reply_base,
>   				  const struct genl_info *info)
> @@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>   			   sizeof(data->link_stats) / 8);
>   
>   	if (req_base->flags & ETHTOOL_FLAG_STATS) {
> -		if (dev->phydev)
> -			data->link_stats.link_down_events =
> -				READ_ONCE(dev->phydev->link_down_events);
> +		ethtool_get_phydev_stats(dev, data);
>   
>   		if (dev->ethtool_ops->get_link_ext_stats)
>   			dev->ethtool_ops->get_link_ext_stats(dev,
> diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
> index 912f0c4fff2f..cf802b1cda6f 100644
> --- a/net/ethtool/stats.c
> +++ b/net/ethtool/stats.c
> @@ -1,5 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   
> +#include <linux/phy.h>
> +
>   #include "netlink.h"
>   #include "common.h"
>   #include "bitset.h"
> @@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
>   	return 0;
>   }
>   
> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{

Nitpick, not big deal but you have the same function names in both
files. I see that they are static and there should not be conflict
but maybe is it worth to add `_phy_` or `_link_` in the name of
the function? Like:

ethtool_get_phydev_phy_stats
or
ethtool_get_phydev_link_stats

?

> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>   static int stats_prepare_data(const struct ethnl_req_info *req_base,
>   			      struct ethnl_reply_data *reply_base,
>   			      const struct genl_info *info)
> @@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
>   	data->ctrl_stats.src = src;
>   	data->rmon_stats.src = src;
>   
> +	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> +	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		ethtool_get_phydev_stats(dev, data);
> +
>   	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
>   	    dev->ethtool_ops->get_eth_phy_stats)
>   		dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);