| Message ID | 20240829174342.3255168-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethtool: add phy(dev) specific stats over netlink | expand |
On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski 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> Huh.. it is completely different compared to what I was thinking. If I see it correctly, it will help to replace missing HW stats for some MACs like ASIX. But it will fail to help diagnose MAC-PHY connections issues like, wrong RGMII configurations or other kind of damages on the PCB. Right? Regards, Oleksij
On Thu, 29 Aug 2024 20:10:00 +0200 Oleksij Rempel wrote: > On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski 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> > > Huh.. it is completely different compared to what I was thinking. > If I see it correctly, it will help to replace missing HW stats for some > MACs like ASIX. But it will fail to help diagnose MAC-PHY connections > issues like, wrong RGMII configurations or other kind of damages on the > PCB. Right? This is just a pre-req for the next patch, to let phy drivers report the (very few) stats we have already defined for integrated NIC drivers. What statistics we choose to add later is up to us, this is just groundwork. BTW the series is primarily to allow you to report the packet / error and OA stats in a structured way, it's not related directly by the discussion on T1L troubleshooting.
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
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... :)
On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski wrote: > Vladimir, I don't understand MM MAC Merge / Frame Preemption in a nutshell: - Frame is express if, after the preamble, it has a "normal" SFD of 0xD5 - Frame is preemptible if, after the preamble, it has an SFD of 0x07, 0x19, 0xE6, 0x4C, 0x7F, 0xB3, 0x61, 0x52, 0x9E or 0x2A express MAC handles express frames preemptible MAC handles preemptible frames ETHTOOL_MAC_STATS_SRC_EMAC counts express frames ETHTOOL_MAC_STATS_SRC_PMAC counts preemptible frames ETHTOOL_MAC_STATS_SRC_AGGREGATE counts both - also works when you don't know Now you know as much as I do. > but doesn't MM share the PHY? It does, yes. There is a single set of MII lines, and distinction between the express and preemptible MAC is done as described above. I wouldn't expect the PHY to be aware of MAC Merge / Frame Preemption, and thus, this component would normally not pay attention to the SFD of the frames it's counting. The entire feature actually depends on the PHY being unaware of the SFD, because they don't make PHYs "for" frame preemption. Although, imaginably, just like we have PHYs which emit PAUSE frames, and that technically means they have a MAC embedded inside, it would not be impossible to twist standards such that the PHY handles FPE/MM. This is only in the realm of theory, AFAIU, and I'm not suggesting we should model the UAPI based on pure theory. > Ocelot seems to aggregate which I did not expect. Ocelot aggregates stats when the request is to aggregate them (explicit ETHTOOL_MAC_STATS_SRC_AGGREGATE, and also default, for comparability/ compatibility with unaware drivers). Otherwise it reports them individually. Also, the stats it reports into phy_stats->SymbolErrorDuringCarrier are MAC stats. They count the number of frames received by the MAC with RX_ER being asserted on the MII interface. So these could be counted by either the MAC, or the PHY. The MAC is MM-aware, the PHY is probably not. Though if I follow the thread, I'm not sure if this is exactly useful to Oleksij, who would like to report an entirely different set of counters. I never got the impression that the ETHTOOL_STATS_ETH_PHY structured netlink counters were for NICs with embedded non-phylib PHYs. If they are - sorry. I thought it was about those MAC counters which are collected at the interface with the PHY.
Hi Jakub, On Fri, 30 Aug 2024 11:30:47 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > 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). I think that could work indeed, good point. [...] > > 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... :) I do have some hardware I can test this on, and I'm starting to get familiar with netlink :) I can give it a try, however I can't guarantee as of right now that I'll be able to send anything before net-next closes. I'll ping here if I start moving forward with this :) Thanks, Maxime
Hi all, On Wed, Sep 04, 2024 at 09:20:24AM +0200, Maxime Chevallier wrote: > Hi Jakub, > > On Fri, 30 Aug 2024 11:30:47 -0700 > Jakub Kicinski <kuba@kernel.org> wrote: > > > 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). > > I think that could work indeed, good point. > > [...] > > > > 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... :) > > I do have some hardware I can test this on, and I'm starting to get > familiar with netlink :) I can give it a try, however I can't guarantee > as of right now that I'll be able to send anything before net-next > closes. I'll ping here if I start moving forward with this :) I reserved some time for this work. I hope it will be this year. In case some one else will start working on it, please let me know :) Regard, Oleksij
On Wed, 4 Sep 2024 10:05:10 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Hi all, > > On Wed, Sep 04, 2024 at 09:20:24AM +0200, Maxime Chevallier wrote: > > Hi Jakub, > > > > On Fri, 30 Aug 2024 11:30:47 -0700 > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > 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). > > > > I think that could work indeed, good point. > > > > [...] > > > > > > 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... :) > > > > I do have some hardware I can test this on, and I'm starting to get > > familiar with netlink :) I can give it a try, however I can't guarantee > > as of right now that I'll be able to send anything before net-next > > closes. I'll ping here if I start moving forward with this :) > > I reserved some time for this work. I hope it will be this year. > > In case some one else will start working on it, please let me know :) Ah perfect then :) Let me know if you ever need some extra testing, I can run this on the hardware I have on hand if that can help. Best regards, Maxime
diff --git a/include/linux/phy.h b/include/linux/phy.h index 4a9a11749c55..53942fd7760f 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); + + 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) +{ + 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);
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> --- CC: Vladimir Oltean <olteanv@gmail.com> CC: andrew@lunn.ch CC: hkallweit1@gmail.com CC: linux@armlinux.org.uk CC: woojung.huh@microchip.com CC: o.rempel@pengutronix.de --- include/linux/phy.h | 10 ++++++++++ net/ethtool/linkstate.c | 25 ++++++++++++++++++++++--- net/ethtool/stats.c | 19 +++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-)