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

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

[Oleksij Rempel]

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@pengutronix.de>
---
 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
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
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 2881af411f761..1fd80ec0c952f 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -232,6 +232,7 @@ enum {
 	ETHTOOL_A_LINKSTATE_UNSPEC,
 	ETHTOOL_A_LINKSTATE_HEADER,		/* nest - _A_HEADER_* */
 	ETHTOOL_A_LINKSTATE_LINK,		/* u8 */
+	ETHTOOL_A_LINKSTATE_SQI,		/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKSTATE_CNT,
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)
+		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;
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index a62f68ccc43ab..a251040d967db 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -30,6 +30,7 @@ extern const char ts_tx_type_names[][ETH_GSTRING_LEN];
 extern const char ts_rx_filter_names[][ETH_GSTRING_LEN];
 
 int __ethtool_get_link(struct net_device *dev);
+int __ethtool_get_sqi(struct net_device *dev);
 
 bool convert_legacy_settings_to_link_ksettings(
 	struct ethtool_link_ksettings *link_ksettings,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 2740cde0a182b..5013ee275176d 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -10,6 +10,7 @@ struct linkstate_req_info {
 struct linkstate_reply_data {
 	struct ethnl_reply_data		base;
 	int				link;
+	u8				sqi;
 };
 
 #define LINKSTATE_REPDATA(__reply_base) \
@@ -20,6 +21,7 @@ linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1] = {
 	[ETHTOOL_A_LINKSTATE_UNSPEC]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKSTATE_HEADER]		= { .type = NLA_NESTED },
 	[ETHTOOL_A_LINKSTATE_LINK]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_LINKSTATE_SQI]		= { .type = NLA_REJECT },
 };
 
 static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
@@ -34,6 +36,15 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 	data->link = __ethtool_get_link(dev);
+
+	ret = __ethtool_get_sqi(dev);
+	if (ret == -EOPNOTSUPP)
+		ret = SQI_STATE_UNSUPPORTED;
+	if (ret < 0)
+		return ret;
+
+	data->sqi = ret;
+
 	ethnl_ops_complete(dev);
 
 	return 0;
@@ -42,8 +53,16 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 static int linkstate_reply_size(const struct ethnl_req_info *req_base,
 				const struct ethnl_reply_data *reply_base)
 {
-	return nla_total_size(sizeof(u8)) /* LINKSTATE_LINK */
+	struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
+	int len;
+
+	len = nla_total_size(sizeof(u8)) /* LINKSTATE_LINK */
 		+ 0;
+
+	if (data->sqi != SQI_STATE_UNSUPPORTED)
+		len += nla_total_size(sizeof(u8));
+
+	return len;
 }
 
 static int linkstate_fill_reply(struct sk_buff *skb,
@@ -56,6 +75,10 @@ static int linkstate_fill_reply(struct sk_buff *skb,
 	    nla_put_u8(skb, ETHTOOL_A_LINKSTATE_LINK, !!data->link))
 		return -EMSGSIZE;
 
+	if (data->sqi != SQI_STATE_UNSUPPORTED &&
+	    nla_put_u8(skb, ETHTOOL_A_LINKSTATE_SQI, data->sqi))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
-- 
2.26.2

[PATCH net-next v1 2/2] net: phy: tja11xx: add SQI support

[Oleksij Rempel]

This patch implements reading of the Signal Quality Index for better
cable/link troubleshooting.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/nxp-tja11xx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 0d4f9067ca715..ed588caa563f4 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -53,6 +53,7 @@
 
 #define MII_COMMSTAT			23
 #define MII_COMMSTAT_LINK_UP		BIT(15)
+#define MII_COMMSTAT_SQI_STATE		GENMASK(7, 5)
 
 #define MII_GENSTAT			24
 #define MII_GENSTAT_PLL_LOCKED		BIT(14)
@@ -329,6 +330,17 @@ static int tja11xx_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+static int tja11xx_get_sqi(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_COMMSTAT);
+	if (ret < 0)
+		return ret;
+
+	return FIELD_GET(MII_COMMSTAT_SQI_STATE, ret) + 1;
+}
+
 static int tja11xx_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(tja11xx_hw_stats);
@@ -683,6 +695,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
+		.get_sqi	= tja11xx_get_sqi,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
@@ -699,6 +712,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
+		.get_sqi	= tja11xx_get_sqi,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
@@ -715,6 +729,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
+		.get_sqi	= tja11xx_get_sqi,
 		.match_phy_device = tja1102_p0_match_phy_device,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
@@ -736,6 +751,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
+		.get_sqi	= tja11xx_get_sqi,
 		.match_phy_device = tja1102_p1_match_phy_device,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-- 
2.26.2

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@pengutronix.de>
> ---
>  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;

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

[Oleksij Rempel]

On Tue, May 19, 2020 at 10:55:20AM +0200, Michal Kubecek wrote:
> 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@pengutronix.de>
> > ---
> >  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"?

yes. Seems to look OK.

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

OK

> (SQI_STATE_8=9 is probably a mistake).

yes.

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

Nice, screenshot based reverse engineering :)

> It's probably not
> standardized (yet?) but it seems to indicate we may expect other devices
> providing SQI information with different value range.

what maximal range do we wont to export? u8, u16 or u32?

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

sounds plausible.

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

ok.

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

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Michal Kubecek]

On Tue, May 19, 2020 at 12:58:55PM +0200, Oleksij Rempel wrote:
> On Tue, May 19, 2020 at 10:55:20AM +0200, Michal Kubecek wrote:
> > 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".
> 
> Nice, screenshot based reverse engineering :)
> 
> > It's probably not
> > standardized (yet?) but it seems to indicate we may expect other devices
> > providing SQI information with different value range.
> 
> what maximal range do we wont to export? u8, u16 or u32?

As the userspace API is "cast in stone" and there no actual space saved
by using u8 or u16 due to padding (attributes are always padded to
a multiple of 32 bits), I would suggest to go with u32 in uapi.
Internally, we can use a smaller type for now if it is more convenient.

Michal

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

[Andrew Lunn]

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

Hi Oleksij

With a multi part patch set, please always include a cover note,
describing what the patchset as a whole does.

> +int __ethtool_get_sqi(struct net_device *dev)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	if (!phydev->drv->get_sqi)
> +		return -EOPNOTSUPP;
> +
> +	return phydev->drv->get_sqi(phydev);
> +}

You are not doing any locking here, which you should. Due to modules
vs built in, it can be a bit tricky getting this right. Take a look at
how ethtool ioctl.c uses phy_ethtool_get_stats() and that inline
function itself.

    Andrew

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

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

On Tue, May 19, 2020 at 03:26:30PM +0200, Andrew Lunn wrote:
> 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].
> 
> Hi Oleksij
> 
> With a multi part patch set, please always include a cover note,
> describing what the patchset as a whole does.

ok

> > +int __ethtool_get_sqi(struct net_device *dev)
> > +{
> > +	struct phy_device *phydev = dev->phydev;
> > +
> > +	if (!phydev->drv->get_sqi)
> > +		return -EOPNOTSUPP;
> > +
> > +	return phydev->drv->get_sqi(phydev);
> > +}
> 
> You are not doing any locking here, which you should. Due to modules
> vs built in, it can be a bit tricky getting this right. Take a look at
> how ethtool ioctl.c uses phy_ethtool_get_stats() and that inline
> function itself.

ok.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andrew Lunn]

> --- 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)
> +		return -EOPNOTSUPP;
> +
> +	return phydev->drv->get_sqi(phydev);
> +}
> +

You are only providing access via netlink ethtool? There is no ioctl
method to get this. If so, i wonder if common.c is the correct place
for this, or if it should be moved into linkstate.c. You can then drop
the __.

Michal?

	Andrew

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

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

On Tue, May 19, 2020 at 04:03:48PM +0200, Andrew Lunn wrote:
> > --- 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)
> > +		return -EOPNOTSUPP;
> > +
> > +	return phydev->drv->get_sqi(phydev);
> > +}
> > +
> 
> You are only providing access via netlink ethtool? There is no ioctl
> method to get this.

ack

> If so, i wonder if common.c is the correct place
> for this, or if it should be moved into linkstate.c. You can then drop
> the __.

ok

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Christian Herber]

On Tue, May 19, 2020 at 12:58:55PM +0200, Oleksij Rempel wrote:
> On Tue, May 19, 2020 at 10:55:20AM +0200, Michal Kubecek wrote:
> > On Tue, May 19, 2020 at 09:51:59AM +0200, Oleksij Rempel wrote:

> > 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:
> >
> > The screenshot on page 10 shows "SQI Value: 00015".
>
> Nice, screenshot based reverse engineering :)
>
> > It's probably not
> > standardized (yet?) but it seems to indicate we may expect other devices
> > providing SQI information with different value range.
>
> what maximal range do we wont to export? u8, u16 or u32?
>
> > Would it make sense to add ETHTOOL_A_LINKSTATE_SQI_MAX attribute telling
> > userspace what the range is?
>
> sounds plausible.

SQI is not yet standardized for 1000BASE-T1 to my knowledge. But looking at the definition of SQI register for 100BASE-T1 gives some good clues.
[0] reserved
[3:1] current SQI value
[4] reserved
[7:5] worst case SQI value since last read

So, the registers are implemented in a way that another LSB can be added.
This would increase the resolution of SQI measurement, not the range.
I.e. the resulting value range could be seen as [0, 0.5, 1, ..., 7.5] or [0, 1, ..., 15].
For the screenshot, I wouldn’t assume the reserved bit is implemented, it could just be that the SQI value was not shifted.

Regards,

Christian