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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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 |