| Message ID | 20240704054007.969557-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1,1/1] ethtool: netlink: do not return SQI value if link is down | expand |
On Thu, Jul 04, 2024 at 07:40:07AM +0200, Oleksij Rempel wrote: > Do not attach SQI value if link is down. "SQI values are only valid if link-up > condition is present" per OpenAlliance specification of 100Base-T1 > Interoperability Test suite [1]. The same rule would apply for other link > types. > > [1] https://opensig.org/automotive-ethernet-specifications/# > > Fixes: 8066021915924 ("ethtool: provide UAPI for PHY Signal Quality Index (SQI)") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > net/ethtool/linkstate.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c > index b2de2108b356a..370ae628b13a4 100644 > --- a/net/ethtool/linkstate.c > +++ b/net/ethtool/linkstate.c > @@ -37,6 +37,8 @@ static int linkstate_get_sqi(struct net_device *dev) > mutex_lock(&phydev->lock); > if (!phydev->drv || !phydev->drv->get_sqi) > ret = -EOPNOTSUPP; > + else if (!phydev->link) > + ret = -ENETDOWN; > else > ret = phydev->drv->get_sqi(phydev); > mutex_unlock(&phydev->lock); > @@ -55,6 +57,8 @@ static int linkstate_get_sqi_max(struct net_device *dev) > mutex_lock(&phydev->lock); > if (!phydev->drv || !phydev->drv->get_sqi_max) > ret = -EOPNOTSUPP; > + else if (!phydev->link) > + ret = -ENETDOWN; > else > ret = phydev->drv->get_sqi_max(phydev); > mutex_unlock(&phydev->lock); I guess this part is optional. I think i've always seen hard coded values. But this is O.K. > @@ -93,12 +97,12 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base, > data->link = __ethtool_get_link(dev); > > ret = linkstate_get_sqi(dev); > - if (ret < 0 && ret != -EOPNOTSUPP) > + if (ret < 0 && ret != -EOPNOTSUPP && ret != -ENETDOWN) > goto out; > data->sqi = ret; So data->sqi becomes -ENETDOWN > - if (data->sqi != -EOPNOTSUPP && > + if (data->sqi != -EOPNOTSUPP && data->sqi != -ENETDOWN && > nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI, data->sqi)) > return -EMSGSIZE; Thinking about the old code, if the driver returned something other than -EOPNOTSUPP, it looks like the error code would make it to user space. Is ethtool/iproute2 setup to correctly handle this? If it is, maybe pass the -ENETDOWN to user space? Andrew
On Thu, Jul 04, 2024 at 04:01:31PM +0200, Andrew Lunn wrote: ... > > ret = linkstate_get_sqi(dev); > > - if (ret < 0 && ret != -EOPNOTSUPP) > > + if (ret < 0 && ret != -EOPNOTSUPP && ret != -ENETDOWN) > > goto out; > > data->sqi = ret; > > So data->sqi becomes -ENETDOWN > > > > - if (data->sqi != -EOPNOTSUPP && > > + if (data->sqi != -EOPNOTSUPP && data->sqi != -ENETDOWN && > > nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI, data->sqi)) > > return -EMSGSIZE; > > Thinking about the old code, if the driver returned something other > than -EOPNOTSUPP, it looks like the error code would make it to user > space. Is ethtool/iproute2 setup to correctly handle this? If it is, > maybe pass the -ENETDOWN to user space? In current state with ethtool v6.5 i'll get following results. If no -ENETDOWN is returned: Settings for spe4: Supported ports: [ TP ] Supported link modes: 100baseT1/Full Supported pause frame use: No Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: 100baseT1/Full Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: 100Mb/s Duplex: Full Auto-negotiation: off master-slave cfg: forced slave master-slave status: unknown Port: Twisted Pair PHYAD: 6 Transceiver: external MDI-X: Unknown Supports Wake-on: d Wake-on: d Link detected: no If -ENETDOWN is returned: Settings for spe4: Supported ports: [ TP ] Supported link modes: 100baseT1/Fulli Supported pause frame use: No Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: 100baseT1/Full Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: 100Mb/s Duplex: Full Auto-negotiation: off master-slave cfg: forced slave master-slave status: unknown Port: Twisted Pair PHYAD: 6 Transceiver: external MDI-X: Unknown Supports Wake-on: d Wake-on: d netlink error: Network is down Instead of "Link detected: no", we will get netlink error. Regards, Oleksij
On Thu, 4 Jul 2024 07:40:07 +0200 Oleksij Rempel wrote: > if (!phydev->drv || !phydev->drv->get_sqi) > ret = -EOPNOTSUPP; > + else if (!phydev->link) > + ret = -ENETDOWN; Can we stick to EOPNOTSUPP for the link down case as well? We're consuming the error, the exact value doesn't matter. Or let's add a helper which checks the int sqi in all it's incarnations for validity: static bool linkstate_sqi_no_data(int sqi) { return sqi == -EOPNOTSUPP || sqi == -ENETDOWN; }
> If -ENETDOWN is returned: > Settings for spe4: > Supported ports: [ TP ] > Supported link modes: 100baseT1/Fulli > Supported pause frame use: No > Supports auto-negotiation: No > Supported FEC modes: Not reported > Advertised link modes: 100baseT1/Full > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 100Mb/s > Duplex: Full > Auto-negotiation: off > master-slave cfg: forced slave > master-slave status: unknown > Port: Twisted Pair > PHYAD: 6 > Transceiver: external > MDI-X: Unknown > Supports Wake-on: d > Wake-on: d > netlink error: Network is down > > Instead of "Link detected: no", we will get netlink error. Thanks. This is not great. There was a slim chance it looked at each individual return value, and would of put "SQI: Network is down", but it does not. So not including the value does seem the best. Andrew
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c index b2de2108b356a..370ae628b13a4 100644 --- a/net/ethtool/linkstate.c +++ b/net/ethtool/linkstate.c @@ -37,6 +37,8 @@ static int linkstate_get_sqi(struct net_device *dev) mutex_lock(&phydev->lock); if (!phydev->drv || !phydev->drv->get_sqi) ret = -EOPNOTSUPP; + else if (!phydev->link) + ret = -ENETDOWN; else ret = phydev->drv->get_sqi(phydev); mutex_unlock(&phydev->lock); @@ -55,6 +57,8 @@ static int linkstate_get_sqi_max(struct net_device *dev) mutex_lock(&phydev->lock); if (!phydev->drv || !phydev->drv->get_sqi_max) ret = -EOPNOTSUPP; + else if (!phydev->link) + ret = -ENETDOWN; else ret = phydev->drv->get_sqi_max(phydev); mutex_unlock(&phydev->lock); @@ -93,12 +97,12 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base, data->link = __ethtool_get_link(dev); ret = linkstate_get_sqi(dev); - if (ret < 0 && ret != -EOPNOTSUPP) + if (ret < 0 && ret != -EOPNOTSUPP && ret != -ENETDOWN) goto out; data->sqi = ret; ret = linkstate_get_sqi_max(dev); - if (ret < 0 && ret != -EOPNOTSUPP) + if (ret < 0 && ret != -EOPNOTSUPP && ret != -ENETDOWN) goto out; data->sqi_max = ret; @@ -136,10 +140,10 @@ static int linkstate_reply_size(const struct ethnl_req_info *req_base, len = nla_total_size(sizeof(u8)) /* LINKSTATE_LINK */ + 0; - if (data->sqi != -EOPNOTSUPP) + if (data->sqi != -EOPNOTSUPP && data->sqi != -ENETDOWN) len += nla_total_size(sizeof(u32)); - if (data->sqi_max != -EOPNOTSUPP) + if (data->sqi_max != -EOPNOTSUPP && data->sqi_max != -ENETDOWN) len += nla_total_size(sizeof(u32)); if (data->link_ext_state_provided) @@ -164,11 +168,11 @@ 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 && + if (data->sqi != -EOPNOTSUPP && data->sqi != -ENETDOWN && nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI, data->sqi)) return -EMSGSIZE; - if (data->sqi_max != -EOPNOTSUPP && + if (data->sqi_max != -EOPNOTSUPP && data->sqi_max != -ENETDOWN && nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI_MAX, data->sqi_max)) return -EMSGSIZE;
Do not attach SQI value if link is down. "SQI values are only valid if link-up condition is present" per OpenAlliance specification of 100Base-T1 Interoperability Test suite [1]. The same rule would apply for other link types. [1] https://opensig.org/automotive-ethernet-specifications/# Fixes: 8066021915924 ("ethtool: provide UAPI for PHY Signal Quality Index (SQI)") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- net/ethtool/linkstate.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)