Re: [PATCH net-next v1 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)

[Michal Kubecek]

On Tue, May 19, 2020 at 09:51:59AM +0200, Oleksij Rempel wrote:
> Signal Quality Index is a mandatory value required by "OPEN Alliance
> SIG" for the 100Base-T1 PHYs [1]. This indicator can be used for cable
> integrity diagnostic and investigating other noise sources and
> implement by at least two vendors: NXP[2] and TI[3].
> 
> [1] http://www.opensig.org/download/document/218/Advanced_PHY_features_for_automotive_Ethernet_V1.0.pdf
> [2] https://www.nxp.com/docs/en/data-sheet/TJA1100.pdf
> [3] https://www.ti.com/product/DP83TC811R-Q1
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  Documentation/networking/ethtool-netlink.rst |  1 +
>  include/linux/phy.h                          |  1 +
>  include/uapi/linux/ethtool.h                 | 11 +++++++++
>  include/uapi/linux/ethtool_netlink.h         |  1 +
>  net/ethtool/common.c                         | 10 ++++++++
>  net/ethtool/common.h                         |  1 +
>  net/ethtool/linkstate.c                      | 25 +++++++++++++++++++-
>  7 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index eed46b6aa07df..4485e622182fc 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -457,6 +457,7 @@ Kernel response contents:
>    ====================================  ======  ==========================
>    ``ETHTOOL_A_LINKSTATE_HEADER``        nested  reply header
>    ``ETHTOOL_A_LINKSTATE_LINK``          bool    link state (up/down)
> +  ``ETHTOOL_A_LINKSTATE_SQI``           u8      Current Signal Quality Index
>    ====================================  ======  ==========================
>  
>  For most NIC drivers, the value of ``ETHTOOL_A_LINKSTATE_LINK`` returns

IIRC you need to update table markers (the "===" lines) so that cell
text does not overflow. Did you check it with "make htmldocs"?

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 59344db43fcb1..b2fd230460d77 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -706,6 +706,7 @@ struct phy_driver {
>  			    struct ethtool_tunable *tuna,
>  			    const void *data);
>  	int (*set_loopback)(struct phy_device *dev, bool enable);
> +	int (*get_sqi)(struct phy_device *dev);
>  };
>  #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
>  				      struct phy_driver, mdiodrv)
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f4662b3a9e1ef..e55caacd1886c 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1678,6 +1678,17 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  #define MASTER_SLAVE_STATE_SLAVE		3
>  #define MASTER_SLAVE_STATE_ERR			4
>  
> +#define SQI_STATE_UNSUPPORTED			0
> +#define SQI_STATE_0				1
> +#define SQI_STATE_1				2
> +#define SQI_STATE_2				3
> +#define SQI_STATE_3				4
> +#define SQI_STATE_4				5
> +#define SQI_STATE_5				6
> +#define SQI_STATE_6				7
> +#define SQI_STATE_7				8
> +#define SQI_STATE_8				9
> +
>  /* Which connector port. */
>  #define PORT_TP			0x00
>  #define PORT_AUI		0x01

The shift by one between actual SQI values and attribute values is IMHO
quite confusing for anyone looking at the messages. As the UNSUPPORTED
value is only internal (the attribute is omitted in reply message in
such case), perhaps we could use int for linkstate_reply_data::sqi and
e.g. -1 for "unsupported". Then we could use native 0-7 range
(SQI_STATE_8=9 is probably a mistake).

I'm also a bit worried about hardcoding the 0-7 value range. While I
understand that it's defined by standard for 100base-T1, we my want to
provide such information for other devices in the future. I tried to
search if there is something like that for 1000base-T1 and found this:

  http://www.sigent.cn/wp-content/uploads/2019/12/TE-1400_User-Manual_1000BASE-T1-EMC-Converter_v3.3.pdf

The screenshot on page 10 shows "SQI Value: 00015". It's probably not
standardized (yet?) but it seems to indicate we may expect other devices
providing SQI information with different value range.

Would it make sense to add ETHTOOL_A_LINKSTATE_SQI_MAX attribute telling
userspace what the range is?

[...]
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 423e640e3876d..f3c905e59124f 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -310,6 +310,16 @@ int __ethtool_get_link(struct net_device *dev)
>  	return netif_running(dev) && dev->ethtool_ops->get_link(dev);
>  }
>  
> +int __ethtool_get_sqi(struct net_device *dev)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev->drv->get_sqi)

You should check phydev for NULL first.

Michal

> +		return -EOPNOTSUPP;
> +
> +	return phydev->drv->get_sqi(phydev);
> +}
> +
>  int ethtool_get_max_rxfh_channel(struct net_device *dev, u32 *max)
>  {
>  	u32 dev_size, current_max = 0;