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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 28 Mar 2019 20:51:34 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Petr Machata <petrm@...lanox.com>
Cc:     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

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.

	 Andrew