| Message ID | 20200520062915.29493-2-o.rempel@pengutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | provide KAPI for SQI | expand |
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
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
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?
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
> > 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
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.
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; }
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(-)