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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Oct 2022 19:50:35 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        corbet@....net, michael.chan@...adcom.com, andrew@...n.ch,
        hkallweit1@...il.com, linux@...linux.org.uk,
        huangguangbin2@...wei.com, chenhao288@...ilicon.com,
        moshet@...dia.com, linux@...pel-privat.de,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH net-next v2] ethtool: linkstate: add a statistic for PHY
 down events



On 10/27/2022 6:27 PM, 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 and all Linux-controlled PHYs. 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@...nel.org>
> ---
[snip]

Looks good just one nit/comment, feel free to respin or add:

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>

> @@ -723,6 +724,8 @@ struct phy_device {
>   
>   	int pma_extable;
>   
> +	unsigned int link_down_events;

Should not this be an u64 to match what the extended link state can 
report? Not that I would hope that anyone had a chance to witness 4 
billion link down events using PHYLIB.
-- 
Florian