| Message ID | 20250106083301.1039850-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce unified and structured PHY | expand |
On Mon, 6 Jan 2025 09:32:55 +0100 Oleksij Rempel wrote: > + /** > + * @get_link_stats: Retrieve link statistics. > + * @dev: The PHY device for which the statistics are retrieved. > + * @link_stats: structure where link-specific stats will be stored. > + * > + * Retrieves link-related statistics for the given PHY device. The input > + * structure is pre-initialized with `ETHTOOL_STAT_NOT_SET`, and the > + * driver must only modify members corresponding to supported > + * statistics. Unmodified members will remain set to > + * `ETHTOOL_STAT_NOT_SET` and will not be returned to userspace. > + * > + * Return: 0 on success or a negative error code on failure. These get callbacks return void > + */ > + 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 */ > @@ -1777,6 +1810,49 @@ static inline bool phy_is_pseudo_fixed_link(struct phy_device *phydev) > return phydev->is_pseudo_fixed_link; > } > > +/** > + * phy_ethtool_get_phy_stats - Retrieve standardized PHY statistics > + * @phydev: Pointer to the PHY device > + * @phy_stats: Pointer to ethtool_eth_phy_stats structure > + * @phydev_stats: Pointer to ethtool_phy_stats structure > + * > + * Fetches PHY statistics using a kernel-defined interface for consistent > + * diagnostics. Unlike phy_ethtool_get_stats(), which allows custom stats, > + * this function enforces a standardized format for better interoperability. > + */ > +static inline void phy_ethtool_get_phy_stats(struct phy_device *phydev, > + struct ethtool_eth_phy_stats *phy_stats, > + struct ethtool_phy_stats *phydev_stats) > +{ > + if (!phydev->drv || !phydev->drv->get_phy_stats) > + return; > + > + mutex_lock(&phydev->lock); > + phydev->drv->get_phy_stats(phydev, phy_stats, phydev_stats); > + mutex_unlock(&phydev->lock); > +} > + > +/** > + * phy_ethtool_get_link_ext_stats - Retrieve extended link statistics for a PHY > + * @phydev: Pointer to the PHY device > + * @link_stats: Pointer to the structure to store extended link statistics > + * > + * Populates the ethtool_link_ext_stats structure with link down event counts > + * and additional driver-specific link statistics, if available. > + */ > +static inline void phy_ethtool_get_link_ext_stats(struct phy_device *phydev, > + struct ethtool_link_ext_stats *link_stats) > +{ > + link_stats->link_down_events = READ_ONCE(phydev->link_down_events); > + > + if (!phydev->drv || !phydev->drv->get_link_stats) > + return; > + > + mutex_lock(&phydev->lock); > + phydev->drv->get_link_stats(phydev, link_stats); > + mutex_unlock(&phydev->lock); > +} Do these have to be static inlines? Seemds like it will just bloat the header, and make alignment more painful.
On Tue, Jan 07, 2025 at 06:02:16PM -0800, Jakub Kicinski wrote: > On Mon, 6 Jan 2025 09:32:55 +0100 Oleksij Rempel wrote: > > +/** > > + * phy_ethtool_get_link_ext_stats - Retrieve extended link statistics for a PHY > > + * @phydev: Pointer to the PHY device > > + * @link_stats: Pointer to the structure to store extended link statistics > > + * > > + * Populates the ethtool_link_ext_stats structure with link down event counts > > + * and additional driver-specific link statistics, if available. > > + */ > > +static inline void phy_ethtool_get_link_ext_stats(struct phy_device *phydev, > > + struct ethtool_link_ext_stats *link_stats) > > +{ > > + link_stats->link_down_events = READ_ONCE(phydev->link_down_events); > > + > > + if (!phydev->drv || !phydev->drv->get_link_stats) > > + return; > > + > > + mutex_lock(&phydev->lock); > > + phydev->drv->get_link_stats(phydev, link_stats); > > + mutex_unlock(&phydev->lock); > > +} > > Do these have to be static inlines? > > Seemds like it will just bloat the header, and make alignment more > painful. On one side I need to address the request to handle phydev specific thing withing the PHYlib framework. On other side, I can't do it without openen a pandora box of build dependencies. It will be a new main-side-quest to resolve build dependency of net/ethtool/ and PHYlib. The workaround is to put this functions to the header.
On Wed, Jan 08, 2025 at 10:08:54AM +0100, Oleksij Rempel wrote: > On Tue, Jan 07, 2025 at 06:02:16PM -0800, Jakub Kicinski wrote: > > On Mon, 6 Jan 2025 09:32:55 +0100 Oleksij Rempel wrote: > > > +/** > > > + * phy_ethtool_get_link_ext_stats - Retrieve extended link statistics for a PHY > > > + * @phydev: Pointer to the PHY device > > > + * @link_stats: Pointer to the structure to store extended link statistics > > > + * > > > + * Populates the ethtool_link_ext_stats structure with link down event counts > > > + * and additional driver-specific link statistics, if available. > > > + */ > > > +static inline void phy_ethtool_get_link_ext_stats(struct phy_device *phydev, > > > + struct ethtool_link_ext_stats *link_stats) > > > +{ > > > + link_stats->link_down_events = READ_ONCE(phydev->link_down_events); > > > + > > > + if (!phydev->drv || !phydev->drv->get_link_stats) > > > + return; > > > + > > > + mutex_lock(&phydev->lock); > > > + phydev->drv->get_link_stats(phydev, link_stats); > > > + mutex_unlock(&phydev->lock); > > > +} > > > > Do these have to be static inlines? > > > > Seemds like it will just bloat the header, and make alignment more > > painful. > > On one side I need to address the request to handle phydev specific > thing withing the PHYlib framework. On other side, I can't do it without > openen a pandora box of build dependencies. It will be a new main-side-quest > to resolve build dependency of net/ethtool/ and PHYlib. The workaround is to > put this functions to the header. Yes, the code is like this because phylib can be a module, and when it is, you would end up with unresolved symbols if ethtool code is built in. There are circular dependence as well, if both ethtool and phylib are module. The inlines help solve this. However, the number of these inline functions keeps growing. At some point, we might want a different solution. Maybe phylib needs to register a structure of ops with ethtool when it loads? Or we give up with phylib being modular and only allow it to be built in. Andrew
Hi Andrew, Oleksij, On Wed, 8 Jan 2025 17:03:25 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > On one side I need to address the request to handle phydev specific > > thing withing the PHYlib framework. On other side, I can't do it without > > openen a pandora box of build dependencies. It will be a new main-side-quest > > to resolve build dependency of net/ethtool/ and PHYlib. The workaround is to > > put this functions to the header. > > Yes, the code is like this because phylib can be a module, and when it > is, you would end up with unresolved symbols if ethtool code is built > in. There are circular dependence as well, if both ethtool and phylib > are module. The inlines help solve this. > > However, the number of these inline functions keeps growing. At some > point, we might want a different solution. Maybe phylib needs to > register a structure of ops with ethtool when it loads? Isn't it already the case with the ethtool_phy_ops singleton ? Maybe we can add wrap the get_phy_stats / get_link_ext_stats ops to the ethtool_phy_ops ? My understanding was that this singleton served this purpose. Thanks, Maxime
On Wed, 8 Jan 2025 17:24:56 +0100 Maxime Chevallier wrote: > > > On one side I need to address the request to handle phydev specific > > > thing withing the PHYlib framework. On other side, I can't do it without > > > openen a pandora box of build dependencies. It will be a new main-side-quest > > > to resolve build dependency of net/ethtool/ and PHYlib. The workaround is to > > > put this functions to the header. > > > > Yes, the code is like this because phylib can be a module, and when it > > is, you would end up with unresolved symbols if ethtool code is built > > in. There are circular dependence as well, if both ethtool and phylib > > are module. The inlines help solve this. > > > > However, the number of these inline functions keeps growing. At some > > point, we might want a different solution. Maybe phylib needs to > > register a structure of ops with ethtool when it loads? > > Isn't it already the case with the ethtool_phy_ops singleton ? Maybe we > can add wrap the get_phy_stats / get_link_ext_stats ops to the > ethtool_phy_ops ? My understanding was that this singleton served this > purpose. Right, or for tiny pieces of code like this we could as well always build them in? Isn't drivers/net/phy/stubs.c already always built in?
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index f711bfd75c4d..4bf70cfec826 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -412,6 +412,29 @@ struct ethtool_eth_phy_stats { ); }; +/** + * struct ethtool_phy_stats - PHY-level statistics counters + * @rx_packets: Total successfully received frames + * @rx_bytes: Total successfully received bytes + * @rx_errors: Total received frames with errors (e.g., CRC errors) + * @tx_packets: Total successfully transmitted frames + * @tx_bytes: Total successfully transmitted bytes + * @tx_errors: Total transmitted frames with errors + * + * This structure provides a standardized interface for reporting + * PHY-level statistics counters. It is designed to expose statistics + * commonly provided by PHYs but not explicitly defined in the IEEE + * 802.3 standard. + */ +struct ethtool_phy_stats { + u64 rx_packets; + u64 rx_bytes; + u64 rx_errors; + u64 tx_packets; + u64 tx_bytes; + u64 tx_errors; +}; + /* Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*), not otherwise exposed * via a more targeted API. */ diff --git a/include/linux/phy.h b/include/linux/phy.h index 5bc71d59910c..81606d8c6a8b 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1144,6 +1144,39 @@ struct phy_driver { int (*cable_test_get_status)(struct phy_device *dev, bool *finished); /* Get statistics from the PHY using ethtool */ + /** + * @get_phy_stats: Retrieve PHY statistics. + * @dev: The PHY device for which the statistics are retrieved. + * @eth_stats: structure where Ethernet PHY stats will be stored. + * @stats: structure where additional PHY-specific stats will be stored. + * + * Retrieves the supported PHY statistics and populates the provided + * structures. The input structures are pre-initialized with + * `ETHTOOL_STAT_NOT_SET`, and the driver must only modify members + * corresponding to supported statistics. Unmodified members will remain + * set to `ETHTOOL_STAT_NOT_SET` and will not be returned to userspace. + * + * Return: 0 on success or a negative error code on failure. + */ + void (*get_phy_stats)(struct phy_device *dev, + struct ethtool_eth_phy_stats *eth_stats, + struct ethtool_phy_stats *stats); + + /** + * @get_link_stats: Retrieve link statistics. + * @dev: The PHY device for which the statistics are retrieved. + * @link_stats: structure where link-specific stats will be stored. + * + * Retrieves link-related statistics for the given PHY device. The input + * structure is pre-initialized with `ETHTOOL_STAT_NOT_SET`, and the + * driver must only modify members corresponding to supported + * statistics. Unmodified members will remain set to + * `ETHTOOL_STAT_NOT_SET` and will not be returned to userspace. + * + * Return: 0 on success or a negative error code on failure. + */ + 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 */ @@ -1777,6 +1810,49 @@ static inline bool phy_is_pseudo_fixed_link(struct phy_device *phydev) return phydev->is_pseudo_fixed_link; } +/** + * phy_ethtool_get_phy_stats - Retrieve standardized PHY statistics + * @phydev: Pointer to the PHY device + * @phy_stats: Pointer to ethtool_eth_phy_stats structure + * @phydev_stats: Pointer to ethtool_phy_stats structure + * + * Fetches PHY statistics using a kernel-defined interface for consistent + * diagnostics. Unlike phy_ethtool_get_stats(), which allows custom stats, + * this function enforces a standardized format for better interoperability. + */ +static inline void phy_ethtool_get_phy_stats(struct phy_device *phydev, + struct ethtool_eth_phy_stats *phy_stats, + struct ethtool_phy_stats *phydev_stats) +{ + if (!phydev->drv || !phydev->drv->get_phy_stats) + return; + + mutex_lock(&phydev->lock); + phydev->drv->get_phy_stats(phydev, phy_stats, phydev_stats); + mutex_unlock(&phydev->lock); +} + +/** + * phy_ethtool_get_link_ext_stats - Retrieve extended link statistics for a PHY + * @phydev: Pointer to the PHY device + * @link_stats: Pointer to the structure to store extended link statistics + * + * Populates the ethtool_link_ext_stats structure with link down event counts + * and additional driver-specific link statistics, if available. + */ +static inline void phy_ethtool_get_link_ext_stats(struct phy_device *phydev, + struct ethtool_link_ext_stats *link_stats) +{ + link_stats->link_down_events = READ_ONCE(phydev->link_down_events); + + if (!phydev->drv || !phydev->drv->get_link_stats) + return; + + mutex_lock(&phydev->lock); + phydev->drv->get_link_stats(phydev, link_stats); + mutex_unlock(&phydev->lock); +} + int phy_save_page(struct phy_device *phydev); int phy_select_page(struct phy_device *phydev, int page); int phy_restore_page(struct phy_device *phydev, int oldpage, int ret); diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c index 459cfea7652d..d73cf9e3bf4d 100644 --- a/net/ethtool/linkstate.c +++ b/net/ethtool/linkstate.c @@ -135,8 +135,8 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base, if (req_base->flags & ETHTOOL_FLAG_STATS) { if (phydev) - data->link_stats.link_down_events = - READ_ONCE(phydev->link_down_events); + phy_ethtool_get_link_ext_stats(phydev, + &data->link_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..1bdaf071ec6b 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" @@ -20,6 +22,7 @@ struct stats_reply_data { struct ethtool_eth_mac_stats mac_stats; struct ethtool_eth_ctrl_stats ctrl_stats; struct ethtool_rmon_stats rmon_stats; + struct ethtool_phy_stats phydev_stats; ); const struct ethtool_rmon_hist_range *rmon_ranges; }; @@ -120,8 +123,15 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base, struct stats_reply_data *data = STATS_REPDATA(reply_base); enum ethtool_mac_stats_src src = req_info->src; struct net_device *dev = reply_base->dev; + struct nlattr **tb = info->attrs; + struct phy_device *phydev; int ret; + phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_STATS_HEADER], + info->extack); + if (IS_ERR(phydev)) + return PTR_ERR(phydev); + ret = ethnl_ops_begin(dev); if (ret < 0) return ret; @@ -145,6 +155,13 @@ 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) { + if (phydev) + phy_ethtool_get_phy_stats(phydev, &data->phy_stats, + &data->phydev_stats); + } + 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);