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

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 30 Aug 2024 10:16:30 +0200 Maxime Chevallier wrote:
> > +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 ?

Maybe we can flip the order of querying regardless of the PHY that's
targeted? Always query the MAC first and then the PHY, so that the
PHY can override. Presumably the PHY can always provide more detailed
stats than the MAC (IOW if it does provide stats they will be more
accurate).

> > +	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.

We need expert insights from Vladimir on that part :)

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

Great! FWIW I'm not sure what the status of these patches is.
I don't know much about PHYs.
I wrote them to help Oleksij out with the "netlink parts".
I'm not sure how much I convinced Andrew about the applicability.
And I don't know if this is enough for Oleksij to take it forward.
So in the unlikely even that you have spare cycles and a PHY you can
test with, do not hesitate to take these, rework, reset the author 
and repost... :)