[PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Jakub Kicinski]

The previous attempt to augment carrier_down (see Link)
was not met with much enthusiasm so let's do the simple
thing of exposing what some devices already maintain.
Add a common ethtool statistic for link going down.
Currently users have to maintain per-driver mapping
to extract the right stat from the vendor-specific ethtool -S
stats. carrier_down does not fit the bill because it counts
a lot of software related false positives.

Add the statistic to the extended link state API to steer
vendors towards implementing all of it.

Implement for bnxt. mlx5 and (possibly) enic also have
a counter for this but I leave the implementation to their
maintainers.

Link: https://lore.kernel.org/r/20220520004500.2250674-1-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: michael.chan@broadcom.com
CC: huangguangbin2@huawei.com
CC: chenhao288@hisilicon.com
CC: moshet@nvidia.com
CC: linux@rempel-privat.de
CC: linux-doc@vger.kernel.org
---
 Documentation/networking/ethtool-netlink.rst  |  1 +
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 15 +++++++++++++++
 include/linux/ethtool.h                       | 14 ++++++++++++++
 include/uapi/linux/ethtool_netlink.h          |  2 ++
 net/ethtool/linkstate.c                       | 19 ++++++++++++++++++-
 5 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d578b8bcd8a4..5454aa6c013c 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -491,6 +491,7 @@ any attributes.
   ``ETHTOOL_A_LINKSTATE_SQI_MAX``       u32     Max support SQI value
   ``ETHTOOL_A_LINKSTATE_EXT_STATE``     u8      link extended state
   ``ETHTOOL_A_LINKSTATE_EXT_SUBSTATE``  u8      link extended substate
+  ``ETHTOOL_A_LINKSTATE_EXT_DOWN_CNT``  u64     count of link down events
   ====================================  ======  ============================
 
 For most NIC drivers, the value of ``ETHTOOL_A_LINKSTATE_LINK`` returns
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index cc89e5eabcb9..d5957ed00759 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4112,6 +4112,20 @@ static void bnxt_get_rmon_stats(struct net_device *dev,
 	*ranges = bnxt_rmon_ranges;
 }
 
+static void bnxt_get_link_ext_stats(struct net_device *dev,
+				    struct ethtool_link_ext_stats *stats)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	u64 *rx;
+
+	if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS_EXT))
+		return;
+
+	rx = bp->rx_port_stats_ext.sw_stats;
+	stats->LinkDownEvents =
+		*(rx + BNXT_RX_STATS_EXT_OFFSET(link_down_events));
+}
+
 void bnxt_ethtool_free(struct bnxt *bp)
 {
 	kfree(bp->test_info);
@@ -4161,6 +4175,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.get_eeprom             = bnxt_get_eeprom,
 	.set_eeprom		= bnxt_set_eeprom,
 	.get_link		= bnxt_get_link,
+	.get_link_ext_stats	= bnxt_get_link_ext_stats,
 	.get_eee		= bnxt_get_eee,
 	.set_eee		= bnxt_set_eee,
 	.get_module_info	= bnxt_get_module_info,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 99dc7bfbcd3c..3d8480da7896 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -125,6 +125,17 @@ struct ethtool_link_ext_state_info {
 	};
 };
 
+struct ethtool_link_ext_stats {
+	/* Custom Linux statistic for PHY level link down events.
+	 * In a simpler world it should be equal to netdev->carrier_down_count
+	 * unfortunately netdev also counts local reconfigurations which don't
+	 * actually take the physical link down, not to mention NC-SI which,
+	 * if present, keeps the link up regardless of host state.
+	 * This statistic counts when PHY _actually_ went down, or lost link.
+	 */
+	u64 LinkDownEvents;
+};
+
 /**
  * ethtool_rxfh_indir_default - get default value for RX flow hash indirection
  * @index: Index in RX flow hash indirection table
@@ -481,6 +492,7 @@ struct ethtool_module_power_mode_params {
  *	do not attach ext_substate attribute to netlink message). If link_ext_state
  *	and link_ext_substate are unknown, return -ENODATA. If not implemented,
  *	link_ext_state and link_ext_substate will not be sent to userspace.
+ * @get_link_ext_stats: Read extra link-related counters.
  * @get_eeprom_len: Read range of EEPROM addresses for validation of
  *	@get_eeprom and @set_eeprom requests.
  *	Returns 0 if device does not support EEPROM access.
@@ -652,6 +664,8 @@ struct ethtool_ops {
 	u32	(*get_link)(struct net_device *);
 	int	(*get_link_ext_state)(struct net_device *,
 				      struct ethtool_link_ext_state_info *);
+	void	(*get_link_ext_stats)(struct net_device *,
+				      struct ethtool_link_ext_stats *);
 	int	(*get_eeprom_len)(struct net_device *);
 	int	(*get_eeprom)(struct net_device *,
 			      struct ethtool_eeprom *, u8 *);
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index bb57084ac524..8167848983d0 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -262,6 +262,8 @@ enum {
 	ETHTOOL_A_LINKSTATE_SQI_MAX,		/* u32 */
 	ETHTOOL_A_LINKSTATE_EXT_STATE,		/* u8 */
 	ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,	/* u8 */
+	ETHTOOL_A_LINKSTATE_PAD,
+	ETHTOOL_A_LINKSTATE_EXT_DOWN_CNT,	/* u64 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKSTATE_CNT,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index fb676f349455..c84d9b5afd53 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -13,6 +13,7 @@ struct linkstate_reply_data {
 	int					link;
 	int					sqi;
 	int					sqi_max;
+	struct ethtool_link_ext_stats		link_stats;
 	bool					link_ext_state_provided;
 	struct ethtool_link_ext_state_info	ethtool_link_ext_state_info;
 };
@@ -22,7 +23,7 @@ struct linkstate_reply_data {
 
 const struct nla_policy ethnl_linkstate_get_policy[] = {
 	[ETHTOOL_A_LINKSTATE_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_stats),
 };
 
 static int linkstate_get_sqi(struct net_device *dev)
@@ -107,6 +108,13 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 			goto out;
 	}
 
+	ethtool_stats_init((u64 *)&data->link_stats,
+			   sizeof(data->link_stats) / 8);
+
+	if (req_base->flags & ETHTOOL_FLAG_STATS &&
+	    dev->ethtool_ops->get_link_ext_stats)
+		dev->ethtool_ops->get_link_ext_stats(dev, &data->link_stats);
+
 	ret = 0;
 out:
 	ethnl_ops_complete(dev);
@@ -134,6 +142,9 @@ static int linkstate_reply_size(const struct ethnl_req_info *req_base,
 	if (data->ethtool_link_ext_state_info.__link_ext_substate)
 		len += nla_total_size(sizeof(u8)); /* LINKSTATE_EXT_SUBSTATE */
 
+	if (data->link_stats.LinkDownEvents != ETHTOOL_STAT_NOT_SET)
+		len += nla_total_size_64bit(sizeof(u64));
+
 	return len;
 }
 
@@ -166,6 +177,12 @@ static int linkstate_fill_reply(struct sk_buff *skb,
 			return -EMSGSIZE;
 	}
 
+	if (data->link_stats.LinkDownEvents != ETHTOOL_STAT_NOT_SET)
+		if (nla_put_u64_64bit(skb, ETHTOOL_A_LINKSTATE_EXT_DOWN_CNT,
+				      data->link_stats.LinkDownEvents,
+				      ETHTOOL_A_LINKSTATE_PAD))
+			return -EMSGSIZE;
+
 	return 0;
 }
 
-- 
2.37.3

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Oleksij Rempel]

Hi Jakub,

On Tue, Oct 25, 2022 at 07:09:48PM -0700, Jakub Kicinski wrote:
> The previous attempt to augment carrier_down (see Link)
> was not met with much enthusiasm so let's do the simple
> thing of exposing what some devices already maintain.
> Add a common ethtool statistic for link going down.
> Currently users have to maintain per-driver mapping
> to extract the right stat from the vendor-specific ethtool -S
> stats. carrier_down does not fit the bill because it counts
> a lot of software related false positives.
> 
> Add the statistic to the extended link state API to steer
> vendors towards implementing all of it.
> 
> Implement for bnxt. mlx5 and (possibly) enic also have
> a counter for this but I leave the implementation to their
> maintainers.
> 
> Link: https://lore.kernel.org/r/20220520004500.2250674-1-kuba@kernel.org
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: michael.chan@broadcom.com
> CC: huangguangbin2@huawei.com
> CC: chenhao288@hisilicon.com
> CC: moshet@nvidia.com
> CC: linux@rempel-privat.de
> CC: linux-doc@vger.kernel.org
> ---
>  Documentation/networking/ethtool-netlink.rst  |  1 +
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 15 +++++++++++++++
>  include/linux/ethtool.h                       | 14 ++++++++++++++
>  include/uapi/linux/ethtool_netlink.h          |  2 ++
>  net/ethtool/linkstate.c                       | 19 ++++++++++++++++++-
>  5 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index d578b8bcd8a4..5454aa6c013c 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -491,6 +491,7 @@ any attributes.
>    ``ETHTOOL_A_LINKSTATE_SQI_MAX``       u32     Max support SQI value
>    ``ETHTOOL_A_LINKSTATE_EXT_STATE``     u8      link extended state
>    ``ETHTOOL_A_LINKSTATE_EXT_SUBSTATE``  u8      link extended substate
> +  ``ETHTOOL_A_LINKSTATE_EXT_DOWN_CNT``  u64     count of link down events
>    ====================================  ======  ============================
>  
>  For most NIC drivers, the value of ``ETHTOOL_A_LINKSTATE_LINK`` returns
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index cc89e5eabcb9..d5957ed00759 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4112,6 +4112,20 @@ static void bnxt_get_rmon_stats(struct net_device *dev,
>  	*ranges = bnxt_rmon_ranges;
>  }
>  
> +static void bnxt_get_link_ext_stats(struct net_device *dev,
> +				    struct ethtool_link_ext_stats *stats)
> +{
> +	struct bnxt *bp = netdev_priv(dev);
> +	u64 *rx;
> +
> +	if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS_EXT))
> +		return;
> +
> +	rx = bp->rx_port_stats_ext.sw_stats;
> +	stats->LinkDownEvents =
> +		*(rx + BNXT_RX_STATS_EXT_OFFSET(link_down_events));
> +}
> +

s/LinkDownEvents/link_down_events.

What is the best way to implement it on devices without dedicated HW
counter? I assume in most cases only PHY driver would not real state of
link.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Andrew Lunn]

On Tue, Oct 25, 2022 at 07:09:48PM -0700, Jakub Kicinski wrote:
> The previous attempt to augment carrier_down (see Link)
> was not met with much enthusiasm so let's do the simple
> thing of exposing what some devices already maintain.
> Add a common ethtool statistic for link going down.
> Currently users have to maintain per-driver mapping
> to extract the right stat from the vendor-specific ethtool -S
> stats. carrier_down does not fit the bill because it counts
> a lot of software related false positives.
> 
> Add the statistic to the extended link state API to steer
> vendors towards implementing all of it.
> 
> Implement for bnxt. mlx5 and (possibly) enic also have
> a counter for this but I leave the implementation to their
> maintainers.

> +struct ethtool_link_ext_stats {
> +	/* Custom Linux statistic for PHY level link down events.
> +	 * In a simpler world it should be equal to netdev->carrier_down_count
> +	 * unfortunately netdev also counts local reconfigurations which don't
> +	 * actually take the physical link down, not to mention NC-SI which,
> +	 * if present, keeps the link up regardless of host state.
> +	 * This statistic counts when PHY _actually_ went down, or lost link.
> +	 */
> +	u64 LinkDownEvents;
> +};

You might want to consider a generic implementation in phylib. You
should then have over 60 drivers implementing this, enough momentum it
might actually get used.

      Andrew

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Andrew Lunn]

> What is the best way to implement it on devices without dedicated HW
> counter? I assume in most cases only PHY driver would not real state of
> link.

I would hook into the PHY state machine. PHY_RUNNING means the link is
up. If it transitions to PHY_NOLINK the link has been lost.

    Andrew

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Jakub Kicinski]

On Wed, 26 Oct 2022 14:19:08 +0200 Andrew Lunn wrote:
> You might want to consider a generic implementation in phylib.

Would bumping a counter in phy_link_down() be suitable?
I'm slightly unsure about the cable test behavior but 
I think increments during cable tests are quite fair.

> You should then have over 60 drivers implementing this, enough
> momentum it might actually get used.

Depends on your definition of "get used", I can guarantee it will 
get used according to my definition :)

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Jakub Kicinski]

On Wed, 26 Oct 2022 09:40:32 +0200 Oleksij Rempel wrote:
> s/LinkDownEvents/link_down_events.

Fair point, my brain got wedged on the IEEE stats naming.

Re: [PATCH net-next] ethtool: linkstate: add a statistic for PHY down events

[Andrew Lunn]

On Wed, Oct 26, 2022 at 09:10:15AM -0700, Jakub Kicinski wrote:
> On Wed, 26 Oct 2022 14:19:08 +0200 Andrew Lunn wrote:
> > You might want to consider a generic implementation in phylib.
> 
> Would bumping a counter in phy_link_down() be suitable?

Yes, that seems reasonable.

> I'm slightly unsure about the cable test behavior but 
> I think increments during cable tests are quite fair.

Me too. The interface is not usable, and the link will do an auto-neg
once the cable test is finished. So it does seem reasonable to count
such events.

> > You should then have over 60 drivers implementing this, enough
> > momentum it might actually get used.
> 
> Depends on your definition of "get used", I can guarantee it will 
> get used according to my definition :)

We have too many features which only one driver implements. So yes,
you might use it, with one driver. But will it get used by others?  If
it magically starts working for more than 1/2 the kernel drivers, it
seems much more likely it will find more widespread us.

     Andrew