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
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 <[email protected]>
---
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
This patch implements reading of the Signal Quality Index for better
cable/link troubleshooting.
Signed-off-by: Oleksij Rempel <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
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 <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
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 <[email protected]>
> ---
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 <[email protected]>
Michal
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 <[email protected]>
> > ---
>
> 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 <[email protected]>
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 |
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 <[email protected]>
> > > ---
> >
> > 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 <[email protected]>
>
> 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
> > 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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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 |
From: Oleksij Rempel <[email protected]>
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.