> -----Original Message----- > From: Amit Cohen <am...@mellanox.com> > Sent: Thursday, June 25, 2020 6:35 AM > To: Keller, Jacob E <jacob.e.kel...@intel.com> > Cc: Ido Schimmel <ido...@idosch.org>; netdev@vger.kernel.org; > da...@davemloft.net; k...@kernel.org; j...@mellanox.com; > pe...@mellanox.com; ml...@mellanox.com; mkube...@suse.cz; > and...@lunn.ch; f.faine...@gmail.com; li...@rempel-privat.de; Ido Schimmel > <ido...@mellanox.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 <am...@mellanox.com> > >> > >> Add link extended state attributes. > >> > >> Signed-off-by: Amit Cohen <am...@mellanox.com> > >> Reviewed-by: Petr Machata <pe...@mellanox.com> > >> Reviewed-by: Jiri Pirko <j...@mellanox.com> > >> Signed-off-by: Ido Schimmel <ido...@mellanox.com> > > > > I really like this concept. > > > > Reviewed-by: Jacob Keller <jacob.e.kel...@intel.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
