[PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[Oleksij Rempel]

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(-)

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,
-- 
2.39.2

Re: [PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[Andrew Lunn]

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

RE: [PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[Woojung.Huh]

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

Re: [PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[Oleksij Rempel]

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
-- 
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 |

RE: [PATCH net v2 1/1] ethtool: netlink: do not return SQI value if link is down

[Woojung.Huh]

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