| Message ID | 20240706054900.1288111-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/1] ethtool: netlink: do not return SQI value if link is down | expand |
On Sat, Jul 06, 2024 at 07:49:00AM +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: 806602191592 ("ethtool: provide UAPI for PHY Signal Quality Index (SQI)") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Oleksij, > diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c > index b2de2108b356a..4efd327ba5d92 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); > @@ -62,6 +66,16 @@ static int linkstate_get_sqi_max(struct net_device *dev) > return ret; > }; > > +static bool linkstate_sqi_critical_error(int sqi) > +{ > + return sqi < 0 && sqi != -EOPNOTSUPP && sqi != -ENETDOWN; > +} > + > +static bool linkstate_sqi_valid(struct linkstate_reply_data *data) > +{ > + return data->sqi >= 0 && data->sqi_max >= 0; If PHY driver has get_sqi, but not get_sqi_max, then data->sqi could have a valid value, but data->sqi_max will have -EOPNOTSUPP. In this case, linkstate_sqi_valid() will return FALSE and not getting SQI value at all. If both APIs are required, then we could add another condition of data->sqi <= data->sqi_max in linkstate_sqi_valid() And, beside this, calling linkstate_get_sqi and linkstate_get_sqi_max could be moved under "if (dev->flags & IFF_UP)" with setting default value to data->sqi & data->sqi_max. > +} > + Thanks. Woojung
Hi Woojung, On Mon, Jul 08, 2024 at 04:31:53PM +0000, Woojung.Huh@microchip.com wrote: > Hi Oleksij, > > > diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c > > index b2de2108b356a..4efd327ba5d92 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); > > @@ -62,6 +66,16 @@ static int linkstate_get_sqi_max(struct net_device *dev) > > return ret; > > }; > > > > +static bool linkstate_sqi_critical_error(int sqi) > > +{ > > + return sqi < 0 && sqi != -EOPNOTSUPP && sqi != -ENETDOWN; > > +} > > + > > +static bool linkstate_sqi_valid(struct linkstate_reply_data *data) > > +{ > > + return data->sqi >= 0 && data->sqi_max >= 0; > > If PHY driver has get_sqi, but not get_sqi_max, then data->sqi could have > a valid value, but data->sqi_max will have -EOPNOTSUPP. > In this case, linkstate_sqi_valid() will return FALSE and not getting > SQI value at all. SQI without max will not able to describe quality of the link, it is just value saying nothing to the user. > If both APIs are required, then we could add another condition of > data->sqi <= data->sqi_max in linkstate_sqi_valid() Ack. I was thinking about it, but was not sure if it is a good idea. This will silently filer our a bag. Passing a baggy value to the users space is not good too. I'll fix. > And, beside this, calling linkstate_get_sqi and linkstate_get_sqi_max > could be moved under "if (dev->flags & IFF_UP)" with setting default > value to data->sqi & data->sqi_max. IFF_UP is administrative up state, it is not the link/L1 up. sqi_max and sqi should be initialized anyway, otherwise we will show 0/0 if interface is in admin down. Regards, Oleksij
Hi Oleksij, > > > diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c > > > index b2de2108b356a..4efd327ba5d92 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); > > > @@ -62,6 +66,16 @@ static int linkstate_get_sqi_max(struct net_device > *dev) > > > return ret; > > > }; > > > > > > +static bool linkstate_sqi_critical_error(int sqi) > > > +{ > > > + return sqi < 0 && sqi != -EOPNOTSUPP && sqi != -ENETDOWN; > > > +} > > > + > > > +static bool linkstate_sqi_valid(struct linkstate_reply_data *data) > > > +{ > > > + return data->sqi >= 0 && data->sqi_max >= 0; > > > > If PHY driver has get_sqi, but not get_sqi_max, then data->sqi could have > > a valid value, but data->sqi_max will have -EOPNOTSUPP. > > In this case, linkstate_sqi_valid() will return FALSE and not getting > > SQI value at all. > > SQI without max will not able to describe quality of the link, it is > just value saying nothing to the user. > Honestly, I'm not 100% confident that max is really needed because SQI range shall be 0 (worst) and 7 (best) per OpenAlliance specification. On the other side, some devices could not go up to 7 and limit by max. So, agree that both APIs are needed here. > > If both APIs are required, then we could add another condition of > > data->sqi <= data->sqi_max in linkstate_sqi_valid() > > Ack. I was thinking about it, but was not sure if it is a good idea. This > will silently filer our a bag. Passing a baggy value to the users space > is not good too. I'll fix. > Thanks. Will reply in v3. > > And, beside this, calling linkstate_get_sqi and linkstate_get_sqi_max > > could be moved under "if (dev->flags & IFF_UP)" with setting default > > value to data->sqi & data->sqi_max. > > IFF_UP is administrative up state, it is not the link/L1 up. sqi_max and > sqi should be initialized anyway, otherwise we will show 0/0 if > interface is in admin down. Thanks for correcting me. Woojung
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c index b2de2108b356a..4efd327ba5d92 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); @@ -62,6 +66,16 @@ static int linkstate_get_sqi_max(struct net_device *dev) return ret; }; +static bool linkstate_sqi_critical_error(int sqi) +{ + return sqi < 0 && sqi != -EOPNOTSUPP && sqi != -ENETDOWN; +} + +static bool linkstate_sqi_valid(struct linkstate_reply_data *data) +{ + return data->sqi >= 0 && data->sqi_max >= 0; +} + static int linkstate_get_link_ext_state(struct net_device *dev, struct linkstate_reply_data *data) { @@ -93,12 +107,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 (linkstate_sqi_critical_error(ret)) goto out; data->sqi = ret; ret = linkstate_get_sqi_max(dev); - if (ret < 0 && ret != -EOPNOTSUPP) + if (linkstate_sqi_critical_error(ret)) goto out; data->sqi_max = ret; @@ -136,11 +150,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) - len += nla_total_size(sizeof(u32)); - - if (data->sqi_max != -EOPNOTSUPP) - len += nla_total_size(sizeof(u32)); + if (linkstate_sqi_valid(data)) { + len += nla_total_size(sizeof(u32)); /* LINKSTATE_SQI */ + len += nla_total_size(sizeof(u32)); /* LINKSTATE_SQI_MAX */ + } if (data->link_ext_state_provided) len += nla_total_size(sizeof(u8)); /* LINKSTATE_EXT_STATE */ @@ -164,13 +177,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 (linkstate_sqi_valid(data)) { + if (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; + if (nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI_MAX, + data->sqi_max)) + return -EMSGSIZE; + } if (data->link_ext_state_provided) { if (nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_STATE,
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: 806602191592 ("ethtool: provide UAPI for PHY Signal Quality Index (SQI)") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- changes v2: - add and reuse helper functions linkstate_sqi_critical_error() and linkstate_sqi_valid() - make sure we set sqi and sqi_max vals only if both are valid --- net/ethtool/linkstate.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)