| Message ID | 20200519075200.24631-1-o.rempel@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v1,1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI) | expand |
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;
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
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
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
> --- 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
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.
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
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; }
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(-)