netdev - RE: [PATCH net-next 04/10] Documentation: networking: ethtool-netlink: Add link extended state

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] [day] [month] [year] [list]

Message-ID: <02874ECE860811409154E81DA85FBB58C263AE4C@fmsmsx101.amr.corp.intel.com>
Date:   Thu, 25 Jun 2020 22:57:48 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Amit Cohen <amitc@...lanox.com>
CC:     Ido Schimmel <idosch@...sch.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "jiri@...lanox.com" <jiri@...lanox.com>,
        "petrm@...lanox.com" <petrm@...lanox.com>,
        "mlxsw@...lanox.com" <mlxsw@...lanox.com>,
        "mkubecek@...e.cz" <mkubecek@...e.cz>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "linux@...pel-privat.de" <linux@...pel-privat.de>,
        Ido Schimmel <idosch@...lanox.com>
Subject: RE: [PATCH net-next 04/10] Documentation: networking:
 ethtool-netlink: Add link extended state



> -----Original Message-----
> From: Amit Cohen <amitc@...lanox.com>
> Sent: Thursday, June 25, 2020 6:35 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: Ido Schimmel <idosch@...sch.org>; netdev@...r.kernel.org;
> davem@...emloft.net; kuba@...nel.org; jiri@...lanox.com;
> petrm@...lanox.com; mlxsw@...lanox.com; mkubecek@...e.cz;
> andrew@...n.ch; f.fainelli@...il.com; linux@...pel-privat.de; Ido Schimmel
> <idosch@...lanox.com>
> Subject: Re: [PATCH net-next 04/10] Documentation: networking: ethtool-
> netlink: Add link extended state
> 
> On 24-Jun-20 22:07, Jacob Keller wrote:
> >
> >
> > On 6/24/2020 1:19 AM, Ido Schimmel wrote:
> >> From: Amit Cohen <amitc@...lanox.com>
> >>
> >> Add link extended state attributes.
> >>
> >> Signed-off-by: Amit Cohen <amitc@...lanox.com>
> >> Reviewed-by: Petr Machata <petrm@...lanox.com>
> >> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
> >> Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> >
> > I really like this concept.
> >
> > Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> >
> >> ---
> >>  Documentation/networking/ethtool-netlink.rst | 110
> ++++++++++++++++++-
> >>  1 file changed, 106 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/networking/ethtool-netlink.rst
> b/Documentation/networking/ethtool-netlink.rst
> >> index 82470c36c27a..a7cc53f905f5 100644
> >> --- a/Documentation/networking/ethtool-netlink.rst
> >> +++ b/Documentation/networking/ethtool-netlink.rst
> >> @@ -443,10 +443,11 @@ supports.
> >>  LINKSTATE_GET
> >>  =============
> >>
> >> -Requests link state information. At the moment, only link up/down flag (as
> >> -provided by ``ETHTOOL_GLINK`` ioctl command) is provided but some future
> >> -extensions are planned (e.g. link down reason). This request does not have
> any
> >> -attributes.
> >> +Requests link state information. Link up/down flag (as provided by
> >> +``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended state
> might
> >> +be provided as well. In general, extended state describes reasons for why a
> port
> >> +is down, or why it operates in some non-obvious mode. This request does
> not have
> >> +any attributes.
> >>
> >
> > Ok, so basically in addition to the standard ETHTOOL_GLINK data, we also
> > add additional optional extended attributes over the netlink interface.
> > Neat.
> >
> >>  Request contents:
> >>
> >> @@ -461,16 +462,117 @@ Kernel response contents:
> >>    ``ETHTOOL_A_LINKSTATE_LINK``          bool    link state (up/down)
> >>    ``ETHTOOL_A_LINKSTATE_SQI``           u32     Current Signal Quality Index
> >>    ``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
> >>    ====================================  ======
> ============================
> >
> > Ok so we have categories (state) and each category can have sub-states
> > indicating the specific failure. Good.
> >
> >>
> >>  For most NIC drivers, the value of ``ETHTOOL_A_LINKSTATE_LINK`` returns
> >>  carrier flag provided by ``netif_carrier_ok()`` but there are drivers which
> >>  define their own handler.
> >>
> >> +``ETHTOOL_A_LINKSTATE_EXT_STATE`` and
> ``ETHTOOL_A_LINKSTATE_EXT_SUBSTATE`` are
> >> +optional values. ethtool core can provide either both
> >> +``ETHTOOL_A_LINKSTATE_EXT_STATE`` and
> ``ETHTOOL_A_LINKSTATE_EXT_SUBSTATE``,
> >> +or only ``ETHTOOL_A_LINKSTATE_EXT_STATE``, or none of them.
> >> +
> >>  ``LINKSTATE_GET`` allows dump requests (kernel returns reply messages for
> all
> >>  devices supporting the request).
> >>
> >
> > Good to clarify that it is allowed to specify only the main state, if a
> > substate isn't known.
> >
> 
> I described above all the options:
> "ethtool core can provide either (1) both ``ETHTOOL_A_LINKSTATE_EXT_STATE``
> and ``ETHTOOL_A_LINKSTATE_EXT_SUBSTATE``, or (2) only
> ``ETHTOOL_A_LINKSTATE_EXT_STATE``,
> or (3) none of them"
> 
> I think that #2 is what you want, no?
> 

I meant this as a "excellent, I'm glad this is done", not as a "please do something".

Thanks,
Jake