| Message ID | 20241203075622.2452169-2-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce unified and structured PHY | expand |
Hi Oleksij, On Tue, 3 Dec 2024 08:56:15 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > From: Jakub Kicinski <kuba@kernel.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@kernel.org> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> [...] > +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
On 12/3/2024 8:56 AM, Oleksij Rempel wrote: > From: Jakub Kicinski <kuba@kernel.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@kernel.org> > Signed-off-by: Oleksij Rempel <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(-) > > 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);
On Tue, Dec 03, 2024 at 08:56:15AM +0100, Oleksij Rempel wrote: > From: Jakub Kicinski <kuba@kernel.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@kernel.org> > Signed-off-by: Oleksij Rempel <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(-) > > 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. Eh what? This states to me that the structure is not initialised, but drivers should not write to all members unless they support the statistic. Doesn't this mean we end up returning uninitialised data to userspace? If the structure is not initialised, how does core code know which statistics the driver has set to avoid returning uninitialised data? Also, one comment per function pointer please. > + */ > + 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); I don't like the idea of code outside of phylib fiddling around with phy_device members. Please make the bulk of this a function in phylib, and then do: if (phydev) phy_ethtool_get_stats(phydev, &data->link_stats); However, at that point it's probably not worth having the separate function, and it might as well be placed in linkstate_prepare_data(). > +} > + > 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); Ditto. Thanks.
On Thu, 5 Dec 2024 11:57:33 +0000 Russell King (Oracle) wrote: > > + * The input structure is not zero-initialized and the implementation > > + * must only set statistics which are actually collected by the device. > > Eh what? This states to me that the structure is not initialised, but > drivers should not write to all members unless they support the > statistic. > > Doesn't this mean we end up returning uninitialised data to userspace? > If the structure is not initialised, how does core code know which > statistics the driver has set to avoid returning uninitialised data? It's not zero-initialized. Meaning it's initialized to a special magic value that the core then checks for to decide if the driver actually reported something. Maybe this: * Drivers must not zero out statistics which they don't report. * Core will initialize members to ETHTOOL_STAT_NOT_SET and check * for this value to report to user space only the stats actually * supported by the device. IDK how to phrase this better..
On Thu, Dec 05, 2024 at 05:19:09PM -0800, Jakub Kicinski wrote: > On Thu, 5 Dec 2024 11:57:33 +0000 Russell King (Oracle) wrote: > > > + * The input structure is not zero-initialized and the implementation > > > + * must only set statistics which are actually collected by the device. > > > > Eh what? This states to me that the structure is not initialised, but > > drivers should not write to all members unless they support the > > statistic. > > > > Doesn't this mean we end up returning uninitialised data to userspace? > > If the structure is not initialised, how does core code know which > > statistics the driver has set to avoid returning uninitialised data? > > It's not zero-initialized. Meaning it's initialized to a special magic > value that the core then checks for to decide if the driver actually > reported something. > > Maybe this: > > * Drivers must not zero out statistics which they don't report. > * Core will initialize members to ETHTOOL_STAT_NOT_SET and check > * for this value to report to user space only the stats actually > * supported by the device. > > IDK how to phrase this better.. Maybe: * The input structure is pre-initialised with ETHTOOL_STAT_NOT_SET and * the implementation must only change implemented statistics. ?
On Fri, 6 Dec 2024 09:11:32 +0000 Russell King (Oracle) wrote: > Maybe: > > * The input structure is pre-initialised with ETHTOOL_STAT_NOT_SET and > * the implementation must only change implemented statistics. Yup, that's better! FWIW I think my brain goes to talking about zero-init because for per-queue or per-cpu stats some drivers do: for each q: struct->stat += q->stat; without first setting to 0. And it _seems_ fine since NOT_SET is -1, and the off-by-one is hard to spot. But for PHY stats this sort of iteration is very unlikely.
On Tue, Dec 03, 2024 at 08:56:15AM +0100, Oleksij Rempel wrote: > From: Jakub Kicinski <kuba@kernel.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@kernel.org> > Signed-off-by: Oleksij Rempel <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(-) > > 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); nit: There should be a kernel doc for @get_link_stats here. Flagged by ./scripts/kernel-doc -none > + 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/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); + + 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);