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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Dec 03, 2024 at 08:56:15AM +0100, 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.

Eh what? This states to me that the structure is not initialised, but
drivers should not write to all members unless they support the
statistic.

Doesn't this mean we end up returning uninitialised data to userspace?
If the structure is not initialised, how does core code know which
statistics the driver has set to avoid returning uninitialised data?

Also, one comment per function pointer please.

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

I don't like the idea of code outside of phylib fiddling around with
phy_device members. Please make the bulk of this a function in phylib,
and then do:

	if (phydev)
		phy_ethtool_get_stats(phydev, &data->link_stats);

However, at that point it's probably not worth having the separate
function, and it might as well be placed in linkstate_prepare_data().

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

Ditto.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!