Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

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

Hello Jakub,

On Thu, 29 Aug 2024 10:43:41 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> 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.
> 
> Vladimir, I don't understand MM but doesn't MM share the PHY?
> Ocelot seems to aggregate which I did not expect.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

[...]

> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;

This would be a very nice spot to use the new
ethnl_req_get_phydev(), if there are multiple PHYs on that device.
Being able to access the stats individually can help
troubleshoot HW issues.

[...]

> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;

Here as well, but that's trickier, as the MAC can override the PHY
stats, but it doesn't know which PHY were getting the stats from.

Maybe we could make so that when we pass a phy_index in the netlink
command, we don't allow the mac to override the phy stats ? Or better,
don't allow the mac to override these stats and report the MAC-reported
PHY stats alongside the PHY-reported stats ?

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

I might be missing something, but I think
ETHTOOL_MAC_STATS_SRC_AGGREGATE is a MM-specific flag and I don't really
see how this applies to getting the PHY stats. I don't know much about
MM though so I could be missing the point.

I'm all in for getting the PHY stats from netlink though :)

Thanks,

Maxime