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]

Message-ID: <87zhpeg9rg.fsf@mellanox.com>
Date:   Thu, 28 Mar 2019 17:59:50 +0000
From:   Petr Machata <petrm@...lanox.com>
To:     Andrew Lunn <andrew@...n.ch>
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


Andrew Lunn <andrew@...n.ch> writes:

> I argued this is a PHY layer status information. We don't really want
> to have to modify every MAC driver to call into phylib/phylink. Yes,
> we can have a netdev_ops for those drivers which ignore the Linux PHY
> layer, but ideally we want to transparently call into the PHY layer to
> get this information, if the MAC is not doing odd things.

>From what I see, there are four places where the information could be
collected:

- MAC driver: dev->ethtool_ops->get_link_down_reason
  (At least mlxsw and mlx5 need this.)

    modified   include/linux/ethtool.h
    @@ -395,4 +395,7 @@ struct ethtool_ops {
                                        struct ethtool_fecparam *);
            void    (*get_ethtool_phy_stats)(struct net_device *,
                                            struct ethtool_stats *, u64 *);
    +       int     (*get_link_down_reason)(const struct net_device *,
    +                                       enum link_down_reason_major *,
    +                                       u32 *minor);
    };

  Return -ENODATA to indicate there's nothing to report.

- 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.

- 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.

Speaking specifically, my patch set would only do the first step above,
because neither mlxsw nor mlx5 use the PHY subsystem. But it would be
easy enough to extend for the other cases above.

Thoughts?

Thanks,
Petr