netdev - Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 23 Apr 2019 15:41:36 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Petr Machata <petrm@...lanox.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>
Subject: Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to
 RTNL messages

Thu, Mar 28, 2019 at 08:51:34PM CET, andrew@...n.ch wrote:
>On Thu, Mar 28, 2019 at 05:59:50PM +0000, Petr Machata wrote:
>
>Hi Petr
>
>> - PHY driver: dev->phydev->drv->get_link_down_reason
>>   Certain PHY drivers might want to have a custom handling for some
>>   PHY-specific insight. Something like:
>> 
>>     modified   include/linux/phy.h
>>     @@ -636,4 +636,7 @@ struct phy_driver {
>>                                 struct ethtool_tunable *tuna,
>>                                 const void *data);
>>             int (*set_loopback)(struct phy_device *dev, bool enable);
>>     +       int (*get_link_down_reason)(struct phy_device *dev,
>>     +                                   enum link_down_reason_major *major,
>>     +                                   u32 *minor);
>>     };
>> 
>>   Return -ENODATA to indicate there's nothing to report.
>> 
>> - Generic PHY
>>   It would be possible to add a function that e.g. consults the PHY
>>   state machine to figure out what went wrong. Phy as such may have to
>>   be extended to keep track of e.g. why the state machine is PHY_HALTED,
>>   or possibly other problems worthy of reporting.
>
>I would suggest changing the order of these two. Put the generic PHY
>first. If it sees the driver has a OP to get more specific detail, it
>would make the call into the driver. You don't violate any layering
>that way, and the locking is done correctly.
>
>The PHY being in state HALTED probably means there is a hardware error
>at the MDIO level. That does not happen very often at all.
>
>What you are more interested is state PHY_NOLINK, which indicates
>there is no link. Maybe because auto-neg didn't complete, if it was
>turned on, or if auto-neg is off and the link is forced, the peer is
>using something different, or is not there at all.
>
>> - phylink
>>   I'm not sure how to generically plug in the phylink library, because
>>   it is a private property of a MAC driver. There are currently only
>>   three drivers using phylink (mvvp2, mvneta, DSA), but it looks as if
>>   the intention is that phylink is used much more widely. So maybe it
>>   would make sense to have an NDO like get_phylink and use that to call
>>   into the phylink library for some generic handling.
>
>We need to consider adding a phylink pointer to the netdev structure
>soon, in the same way there is a phylib structure.
>
>> Speaking specifically, my patch set would only do the first step above,
>> because neither mlxsw nor mlx5 use the PHY subsystem.
>
>Shame. With just mlx, you will get a coverage of ~0. If you did the
>generic phylib work, you might get coverage of > 50% of the MAC
>drivers. It then becomes something useful and really used.

Yeah, that's what it is. But maybe someone would pick up the work and
do the phylib implementation too. Let's see.

>
>	 Andrew