Hi Oleksij,
On Tue, 3 Dec 2024 08:56:15 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
[...]
> +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
