| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87926a26-3f9e-4094-b72a-d64ab9176cde@intel.com>
Date: Thu, 5 Dec 2024 08:45:17 +0100
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Andrew Lunn
<andrew+netdev@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, "Jonathan
Corbet" <corbet@....net>
CC: <kernel@...gutronix.de>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>, Simon Horman <horms@...nel.org>, Russell King
<linux@...linux.org.uk>, Maxime Chevallier <maxime.chevallier@...tlin.com>,
<linux-doc@...r.kernel.org>
Subject: Re: [PATCH net-next v1 1/7] net: ethtool: plumb PHY stats to PHY
drivers
On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> From: Jakub Kicinski <kuba@...nel.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@...nel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.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.
> + */
> + 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);
Maybe silly questions but... Why to use dev->phydev when you created
*phydev pointer at the top of the function? Moreover, is that `if`
necessary? I understand that it will be always true as negative
scenario you handle in the first if? Or do I misunderstand something?
> +
> + 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);
>
> 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)
> +{
Nitpick, not big deal but you have the same function names in both
files. I see that they are static and there should not be conflict
but maybe is it worth to add `_phy_` or `_link_` in the name of
the function? Like:
ethtool_get_phydev_phy_stats
or
ethtool_get_phydev_link_stats
?
> + 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);
> +
> if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> dev->ethtool_ops->get_eth_phy_stats)
> dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);
Powered by blists - more mailing lists