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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Oleksij,

On Tue,  3 Dec 2024 08:56:15 +0100
Oleksij Rempel <o.rempel@pengutronix.de> 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>

[...] 

> +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);
> +
> +	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);

I'm sorry to bother you with my multi-phy stuff, but I'd like to avoid
directly accessing netdev->phydev at least in the netlink code.

Could it be possible for you to pass a phydev directly to the
ethtool_get_phydev_stats() helper you're creating ? That way, you could
get the stats from other phydevs on the link if userspace passed a phy
index in the netlink header. You'd get the phydev that way :

phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_LINKSTATE_HEADER,], info->extack);

This is what's done in the pse-pd, plca and cabletest netlink code that
deals with phydevs.

Note that this helper will fallback to netdev->phydev if user didn't
pass any PHY index, which I expect to be what people do most of the
time. However should the netdev have more than 1 PHY, we would be able
to get the far-away PHY's stats :)

>  
>  		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)
> +{
> +	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);

Same here :)

Thanks,

Maxime