linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/2] provide KAPI for SQI
@ 2020-05-20  6:29 Oleksij Rempel
  2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Oleksij Rempel @ 2020-05-20  6:29 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Jonathan Corbet, Michal Kubecek
  Cc: Oleksij Rempel, David Jander, kernel, linux-kernel, netdev,
	Russell King, mkl, Marek Vasut, Christian Herber

This patches are extending ethtool netlink interface to export Signal
Quality Index (SQI). SQI provided by 100Base-T1 PHYs and can be used for
cable diagnostic. Compared to a typical cable tests, this value can be
only used after link is established.

changes v3:
- rename __ethtool_get_sqi* to linkstate_get_sqi*. And move this
  functions to the net/ethtool/linkstate.c
- protect linkstate_get_sqi* with locking

changes v2:
- use u32 instead of u8 for SQI
- add SQI_MAX field and callbacks
- some style fixes in the rst.
- do not convert index to shifted index.

Oleksij Rempel (2):
  ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  net: phy: tja11xx: add SQI support

 Documentation/networking/ethtool-netlink.rst |  6 +-
 drivers/net/phy/nxp-tja11xx.c                | 26 +++++++
 include/linux/phy.h                          |  2 +
 include/uapi/linux/ethtool_netlink.h         |  2 +
 net/ethtool/linkstate.c                      | 75 +++++++++++++++++++-
 5 files changed, 108 insertions(+), 3 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20  6:29 [PATCH net-next v3 0/2] provide KAPI for SQI Oleksij Rempel
@ 2020-05-20  6:29 ` Oleksij Rempel
  2020-05-20 14:29   ` Andrew Lunn
  2020-05-20 14:45   ` Michal Kubecek
  2020-05-20  6:29 ` [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support Oleksij Rempel
  2020-05-22  0:18 ` [PATCH net-next v3 0/2] provide KAPI for SQI David Miller
  2 siblings, 2 replies; 11+ messages in thread
From: Oleksij Rempel @ 2020-05-20  6:29 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Jonathan Corbet, Michal Kubecek
  Cc: Oleksij Rempel, David Jander, kernel, linux-kernel, netdev,
	Russell King, mkl, Marek Vasut, Christian Herber

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 |  6 +-
 include/linux/phy.h                          |  2 +
 include/uapi/linux/ethtool_netlink.h         |  2 +
 net/ethtool/linkstate.c                      | 75 +++++++++++++++++++-
 4 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index eed46b6aa07df..7e651ea33eabb 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -454,10 +454,12 @@ Request contents:
 
 Kernel response contents:
 
-  ====================================  ======  ==========================
+  ====================================  ======  ============================
   ``ETHTOOL_A_LINKSTATE_HEADER``        nested  reply header
   ``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
+  ====================================  ======  ============================
 
 For most NIC drivers, the value of ``ETHTOOL_A_LINKSTATE_LINK`` returns
 carrier flag provided by ``netif_carrier_ok()`` but there are drivers which
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 59344db43fcb1..950ba479754bd 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -706,6 +706,8 @@ 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);
+	int (*get_sqi_max)(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_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 2881af411f761..e6f109b76c9aa 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -232,6 +232,8 @@ enum {
 	ETHTOOL_A_LINKSTATE_UNSPEC,
 	ETHTOOL_A_LINKSTATE_HEADER,		/* nest - _A_HEADER_* */
 	ETHTOOL_A_LINKSTATE_LINK,		/* u8 */
+	ETHTOOL_A_LINKSTATE_SQI,		/* u32 */
+	ETHTOOL_A_LINKSTATE_SQI_MAX,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKSTATE_CNT,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 2740cde0a182b..7f47ba89054e1 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -2,6 +2,7 @@
 
 #include "netlink.h"
 #include "common.h"
+#include <linux/phy.h>
 
 struct linkstate_req_info {
 	struct ethnl_req_info		base;
@@ -10,6 +11,8 @@ struct linkstate_req_info {
 struct linkstate_reply_data {
 	struct ethnl_reply_data		base;
 	int				link;
+	int				sqi;
+	int				sqi_max;
 };
 
 #define LINKSTATE_REPDATA(__reply_base) \
@@ -20,8 +23,46 @@ 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 },
+	[ETHTOOL_A_LINKSTATE_SQI_MAX]		= { .type = NLA_REJECT },
 };
 
+static int linkstate_get_sqi(struct net_device *dev)
+{
+	struct phy_device *phydev = dev->phydev;
+	int ret;
+
+	if (!phydev)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phydev->lock);
+	if (!phydev->drv || !phydev->drv->get_sqi)
+		ret = -EOPNOTSUPP;
+	else
+		ret = phydev->drv->get_sqi(phydev);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static int linkstate_get_sqi_max(struct net_device *dev)
+{
+	struct phy_device *phydev = dev->phydev;
+	int ret;
+
+	if (!phydev)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phydev->lock);
+	if (!phydev->drv || !phydev->drv->get_sqi_max)
+		ret = -EOPNOTSUPP;
+	else
+		ret = phydev->drv->get_sqi_max(phydev);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
 static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 				  struct ethnl_reply_data *reply_base,
 				  struct genl_info *info)
@@ -34,6 +75,19 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 	data->link = __ethtool_get_link(dev);
+
+	ret = linkstate_get_sqi(dev);
+	if (ret < 0 && ret != -EOPNOTSUPP)
+		return ret;
+
+	data->sqi = ret;
+
+	ret = linkstate_get_sqi_max(dev);
+	if (ret < 0 && ret != -EOPNOTSUPP)
+		return ret;
+
+	data->sqi_max = ret;
+
 	ethnl_ops_complete(dev);
 
 	return 0;
@@ -42,8 +96,19 @@ 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 != -EOPNOTSUPP)
+		len += nla_total_size(sizeof(u32));
+
+	if (data->sqi_max != -EOPNOTSUPP)
+		len += nla_total_size(sizeof(u32));
+
+	return len;
 }
 
 static int linkstate_fill_reply(struct sk_buff *skb,
@@ -56,6 +121,14 @@ static int linkstate_fill_reply(struct sk_buff *skb,
 	    nla_put_u8(skb, ETHTOOL_A_LINKSTATE_LINK, !!data->link))
 		return -EMSGSIZE;
 
+	if (data->sqi != -EOPNOTSUPP &&
+	    nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI, data->sqi))
+		return -EMSGSIZE;
+
+	if (data->sqi_max != -EOPNOTSUPP &&
+	    nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI_MAX, data->sqi_max))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support
  2020-05-20  6:29 [PATCH net-next v3 0/2] provide KAPI for SQI Oleksij Rempel
  2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
@ 2020-05-20  6:29 ` Oleksij Rempel
  2020-05-20 14:29   ` Andrew Lunn
  2020-05-22  0:18 ` [PATCH net-next v3 0/2] provide KAPI for SQI David Miller
  2 siblings, 1 reply; 11+ messages in thread
From: Oleksij Rempel @ 2020-05-20  6:29 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Jonathan Corbet, Michal Kubecek
  Cc: Oleksij Rempel, David Jander, kernel, linux-kernel, netdev,
	Russell King, mkl, Marek Vasut, Christian Herber

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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 0d4f9067ca715..1e79c30ca81a5 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -53,6 +53,8 @@
 
 #define MII_COMMSTAT			23
 #define MII_COMMSTAT_LINK_UP		BIT(15)
+#define MII_COMMSTAT_SQI_STATE		GENMASK(7, 5)
+#define MII_COMMSTAT_SQI_MAX		7
 
 #define MII_GENSTAT			24
 #define MII_GENSTAT_PLL_LOCKED		BIT(14)
@@ -329,6 +331,22 @@ 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);
+}
+
+static int tja11xx_get_sqi_max(struct phy_device *phydev)
+{
+	return MII_COMMSTAT_SQI_MAX;
+}
+
 static int tja11xx_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(tja11xx_hw_stats);
@@ -683,6 +701,8 @@ 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,
+		.get_sqi_max	= tja11xx_get_sqi_max,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
@@ -699,6 +719,8 @@ 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,
+		.get_sqi_max	= tja11xx_get_sqi_max,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
@@ -715,6 +737,8 @@ 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,
+		.get_sqi_max	= tja11xx_get_sqi_max,
 		.match_phy_device = tja1102_p0_match_phy_device,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
@@ -736,6 +760,8 @@ 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,
+		.get_sqi_max	= tja11xx_get_sqi_max,
 		.match_phy_device = tja1102_p1_match_phy_device,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
@ 2020-05-20 14:29   ` Andrew Lunn
  2020-05-20 14:45   ` Michal Kubecek
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2020-05-20 14:29 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Jonathan Corbet, Michal Kubecek, David Jander,
	kernel, linux-kernel, netdev, Russell King, mkl, Marek Vasut,
	Christian Herber

On Wed, May 20, 2020 at 08:29:14AM +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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support
  2020-05-20  6:29 ` [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support Oleksij Rempel
@ 2020-05-20 14:29   ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2020-05-20 14:29 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Jonathan Corbet, Michal Kubecek, David Jander,
	kernel, linux-kernel, netdev, Russell King, mkl, Marek Vasut,
	Christian Herber

On Wed, May 20, 2020 at 08:29:15AM +0200, Oleksij Rempel wrote:
> This patch implements reading of the Signal Quality Index for better
> cable/link troubleshooting.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
  2020-05-20 14:29   ` Andrew Lunn
@ 2020-05-20 14:45   ` Michal Kubecek
  2020-05-20 15:07     ` Oleksij Rempel
  1 sibling, 1 reply; 11+ messages in thread
From: Michal Kubecek @ 2020-05-20 14:45 UTC (permalink / raw)
  To: netdev
  Cc: Oleksij Rempel, Andrew Lunn, David S. Miller, Florian Fainelli,
	Heiner Kallweit, Jakub Kicinski, Jonathan Corbet, David Jander,
	kernel, linux-kernel, Russell King, mkl, Marek Vasut,
	Christian Herber

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

On Wed, May 20, 2020 at 08:29:14AM +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>
> ---

This looks good to me, there is just one thing I'm not sure about:

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 59344db43fcb1..950ba479754bd 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -706,6 +706,8 @@ 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);
> +	int (*get_sqi_max)(struct phy_device *dev);
>  };
>  #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
>  				      struct phy_driver, mdiodrv)

I'm not sure if it's a good idea to define two separate callbacks. It
means adding two pointers instead of one (for every instance of the
structure, not only those implementing them), doing two calls, running
the same checks twice, locking twice, checking the result twice.

Also, passing a structure pointer would mean less code changed if we
decide to add more related state values later.

What do you think?

If you don't agree, I have no objections so

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20 14:45   ` Michal Kubecek
@ 2020-05-20 15:07     ` Oleksij Rempel
  2020-05-20 15:23       ` Michal Kubecek
  2020-05-20 15:30       ` Andrew Lunn
  0 siblings, 2 replies; 11+ messages in thread
From: Oleksij Rempel @ 2020-05-20 15:07 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: netdev, Marek Vasut, Andrew Lunn, Florian Fainelli,
	Jonathan Corbet, linux-kernel, Russell King, mkl, kernel,
	David Jander, Jakub Kicinski, Christian Herber, David S. Miller,
	Heiner Kallweit

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

On Wed, May 20, 2020 at 04:45:44PM +0200, Michal Kubecek wrote:
> On Wed, May 20, 2020 at 08:29:14AM +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>
> > ---
> 
> This looks good to me, there is just one thing I'm not sure about:
> 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 59344db43fcb1..950ba479754bd 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -706,6 +706,8 @@ 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);
> > +	int (*get_sqi_max)(struct phy_device *dev);
> >  };
> >  #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
> >  				      struct phy_driver, mdiodrv)
> 
> I'm not sure if it's a good idea to define two separate callbacks. It
> means adding two pointers instead of one (for every instance of the
> structure, not only those implementing them), doing two calls, running
> the same checks twice, locking twice, checking the result twice.
> 
> Also, passing a structure pointer would mean less code changed if we
> decide to add more related state values later.
> 
> What do you think?
> 
> If you don't agree, I have no objections so
> 
> Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

I have no strong opinion on it. Should I rework it?


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20 15:07     ` Oleksij Rempel
@ 2020-05-20 15:23       ` Michal Kubecek
  2020-05-20 15:30       ` Andrew Lunn
  1 sibling, 0 replies; 11+ messages in thread
From: Michal Kubecek @ 2020-05-20 15:23 UTC (permalink / raw)
  To: netdev
  Cc: Oleksij Rempel, Marek Vasut, Andrew Lunn, Florian Fainelli,
	Jonathan Corbet, linux-kernel, Russell King, mkl, kernel,
	David Jander, Jakub Kicinski, Christian Herber, David S. Miller,
	Heiner Kallweit

On Wed, May 20, 2020 at 05:07:11PM +0200, Oleksij Rempel wrote:
> On Wed, May 20, 2020 at 04:45:44PM +0200, Michal Kubecek wrote:
> > On Wed, May 20, 2020 at 08:29:14AM +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>
> > > ---
> > 
> > This looks good to me, there is just one thing I'm not sure about:
> > 
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 59344db43fcb1..950ba479754bd 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -706,6 +706,8 @@ 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);
> > > +	int (*get_sqi_max)(struct phy_device *dev);
> > >  };
> > >  #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
> > >  				      struct phy_driver, mdiodrv)
> > 
> > I'm not sure if it's a good idea to define two separate callbacks. It
> > means adding two pointers instead of one (for every instance of the
> > structure, not only those implementing them), doing two calls, running
> > the same checks twice, locking twice, checking the result twice.
> > 
> > Also, passing a structure pointer would mean less code changed if we
> > decide to add more related state values later.
> > 
> > What do you think?
> > 
> > If you don't agree, I have no objections so
> > 
> > Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> 
> I have no strong opinion on it. Should I rework it?

It's up to you. It was a suggestion for possible improvement but I have
no problem with this version being applied.

Michal

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20 15:07     ` Oleksij Rempel
  2020-05-20 15:23       ` Michal Kubecek
@ 2020-05-20 15:30       ` Andrew Lunn
  2020-05-20 17:39         ` Oleksij Rempel
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2020-05-20 15:30 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Michal Kubecek, netdev, Marek Vasut, Florian Fainelli,
	Jonathan Corbet, linux-kernel, Russell King, mkl, kernel,
	David Jander, Jakub Kicinski, Christian Herber, David S. Miller,
	Heiner Kallweit

> > I'm not sure if it's a good idea to define two separate callbacks. It
> > means adding two pointers instead of one (for every instance of the
> > structure, not only those implementing them), doing two calls, running
> > the same checks twice, locking twice, checking the result twice.
> > 
> > Also, passing a structure pointer would mean less code changed if we
> > decide to add more related state values later.
> > 
> > What do you think?
> > 
> > If you don't agree, I have no objections so
> > 
> > Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> 
> I have no strong opinion on it. Should I rework it?

It is an internal API, so we can change it any time we want.

I did wonder if MAX should just be a static value. It seems odd it
would change at run time. But we can re-evaulate this once we got some
more users.

     Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
  2020-05-20 15:30       ` Andrew Lunn
@ 2020-05-20 17:39         ` Oleksij Rempel
  0 siblings, 0 replies; 11+ messages in thread
From: Oleksij Rempel @ 2020-05-20 17:39 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michal Kubecek, netdev, Marek Vasut, Florian Fainelli,
	Jonathan Corbet, linux-kernel, Russell King, mkl, kernel,
	David Jander, Jakub Kicinski, Christian Herber, David S. Miller,
	Heiner Kallweit

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

On Wed, May 20, 2020 at 05:30:01PM +0200, Andrew Lunn wrote:
> > > I'm not sure if it's a good idea to define two separate callbacks. It
> > > means adding two pointers instead of one (for every instance of the
> > > structure, not only those implementing them), doing two calls, running
> > > the same checks twice, locking twice, checking the result twice.
> > > 
> > > Also, passing a structure pointer would mean less code changed if we
> > > decide to add more related state values later.
> > > 
> > > What do you think?
> > > 
> > > If you don't agree, I have no objections so
> > > 
> > > Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> > 
> > I have no strong opinion on it. Should I rework it?
> 
> It is an internal API, so we can change it any time we want.
> 
> I did wonder if MAX should just be a static value. It seems odd it
> would change at run time. But we can re-evaulate this once we got some
> more users.

OK, then let's keep it for now.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next v3 0/2] provide KAPI for SQI
  2020-05-20  6:29 [PATCH net-next v3 0/2] provide KAPI for SQI Oleksij Rempel
  2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
  2020-05-20  6:29 ` [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support Oleksij Rempel
@ 2020-05-22  0:18 ` David Miller
  2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2020-05-22  0:18 UTC (permalink / raw)
  To: o.rempel
  Cc: andrew, f.fainelli, hkallweit1, kuba, corbet, mkubecek, david,
	kernel, linux-kernel, netdev, linux, mkl, marex,
	christian.herber

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: Wed, 20 May 2020 08:29:13 +0200

> This patches are extending ethtool netlink interface to export Signal
> Quality Index (SQI). SQI provided by 100Base-T1 PHYs and can be used for
> cable diagnostic. Compared to a typical cable tests, this value can be
> only used after link is established.
> 
> changes v3:
> - rename __ethtool_get_sqi* to linkstate_get_sqi*. And move this
>   functions to the net/ethtool/linkstate.c
> - protect linkstate_get_sqi* with locking
> 
> changes v2:
> - use u32 instead of u8 for SQI
> - add SQI_MAX field and callbacks
> - some style fixes in the rst.
> - do not convert index to shifted index.

Series applied, thank you.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-22  0:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20  6:29 [PATCH net-next v3 0/2] provide KAPI for SQI Oleksij Rempel
2020-05-20  6:29 ` [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) Oleksij Rempel
2020-05-20 14:29   ` Andrew Lunn
2020-05-20 14:45   ` Michal Kubecek
2020-05-20 15:07     ` Oleksij Rempel
2020-05-20 15:23       ` Michal Kubecek
2020-05-20 15:30       ` Andrew Lunn
2020-05-20 17:39         ` Oleksij Rempel
2020-05-20  6:29 ` [PATCH net-next v3 2/2] net: phy: tja11xx: add SQI support Oleksij Rempel
2020-05-20 14:29   ` Andrew Lunn
2020-05-22  0:18 ` [PATCH net-next v3 0/2] provide KAPI for SQI David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).