[PATCH] net: usb: pegasus: ignore the return value from set_registers();

[PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Petko Manolov]

The return value need to be either ignored or acted upon, otherwise 'deadstore'
clang check would yell at us.  I think it's better to just ignore what this
particular call of set_registers() returns.  The adapter defaults are sane and
it would be operational even if the register write fail.

Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
---
 drivers/net/usb/pegasus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 652e9fcf0b77..49cfc720d78f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 	data[2] = loopback ? 0x09 : 0x01;
 
 	memcpy(pegasus->eth_regs, data, sizeof(data));
-	ret = set_registers(pegasus, EthCtrl0, 3, data);
+	set_registers(pegasus, EthCtrl0, 3, data);
 
 	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
 	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
-- 
2.30.2

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Jakub Kicinski]

On Thu, 12 Aug 2021 11:23:51 +0300 Petko Manolov wrote:
> The return value need to be either ignored or acted upon, otherwise 'deadstore'
> clang check would yell at us.  I think it's better to just ignore what this
> particular call of set_registers() returns.  The adapter defaults are sane and
> it would be operational even if the register write fail.
> 
> Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
> ---
>  drivers/net/usb/pegasus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 652e9fcf0b77..49cfc720d78f 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
>  	data[2] = loopback ? 0x09 : 0x01;
>  
>  	memcpy(pegasus->eth_regs, data, sizeof(data));
> -	ret = set_registers(pegasus, EthCtrl0, 3, data);
> +	set_registers(pegasus, EthCtrl0, 3, data);
>  
>  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||

This one is not added by the recent changes as I initially thought, 
the driver has always checked this return value. The recent changes 
did this:

        ret = set_registers(pegasus, EthCtrl0, 3, data);
 
        if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
            usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
            usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
                u16 auxmode;
-               read_mii_word(pegasus, 0, 0x1b, &auxmode);
+               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
+               if (ret < 0)
+                       goto fail;
                auxmode |= 4;
                write_mii_word(pegasus, 0, 0x1b, &auxmode);
        }
 
+       return 0;
+fail:
+       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
        return ret;
}

now the return value of set_registeres() is ignored. 

Seems like  a better fix would be to bring back the error checking, 
why not?

Please remember to add a fixes tag.

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Pavel Skripkin]

On 8/14/21 2:24 AM, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 11:23:51 +0300 Petko Manolov wrote:
>> The return value need to be either ignored or acted upon, otherwise 'deadstore'
>> clang check would yell at us.  I think it's better to just ignore what this
>> particular call of set_registers() returns.  The adapter defaults are sane and
>> it would be operational even if the register write fail.
>> 
>> Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
>> ---
>>  drivers/net/usb/pegasus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
>> index 652e9fcf0b77..49cfc720d78f 100644
>> --- a/drivers/net/usb/pegasus.c
>> +++ b/drivers/net/usb/pegasus.c
>> @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
>>  	data[2] = loopback ? 0x09 : 0x01;
>>  
>>  	memcpy(pegasus->eth_regs, data, sizeof(data));
>> -	ret = set_registers(pegasus, EthCtrl0, 3, data);
>> +	set_registers(pegasus, EthCtrl0, 3, data);
>>  
>>  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>>  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
> 
> This one is not added by the recent changes as I initially thought,
> the driver has always checked this return value. The recent changes
> did this:
> 
>          ret = set_registers(pegasus, EthCtrl0, 3, data);
>   
>          if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>              usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
>              usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
>                  u16 auxmode;
> -               read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +               if (ret < 0)
> +                       goto fail;
>                  auxmode |= 4;
>                  write_mii_word(pegasus, 0, 0x1b, &auxmode);
>          }
>   
> +       return 0;
> +fail:
> +       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
>          return ret;
> }
> 
> now the return value of set_registeres() is ignored.
> 
> Seems like  a better fix would be to bring back the error checking,
> why not?
> 
> Please remember to add a fixes tag.
> 

Hi, Jakub!

I've suggested to handle this error, but Petko said that device won't 
stop working, it will just get in non-optimal state.

https://lore.kernel.org/lkml/YRF1t5kx6hTrv5LC@carbon/


With regards,
Pavel Skripkin

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Petko Manolov]

On 21-08-13 16:24:39, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 11:23:51 +0300 Petko Manolov wrote:
> > The return value need to be either ignored or acted upon, otherwise 'deadstore'
> > clang check would yell at us.  I think it's better to just ignore what this
> > particular call of set_registers() returns.  The adapter defaults are sane and
> > it would be operational even if the register write fail.
> > 
> > Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
> > ---
> >  drivers/net/usb/pegasus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> > index 652e9fcf0b77..49cfc720d78f 100644
> > --- a/drivers/net/usb/pegasus.c
> > +++ b/drivers/net/usb/pegasus.c
> > @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
> >  	data[2] = loopback ? 0x09 : 0x01;
> >  
> >  	memcpy(pegasus->eth_regs, data, sizeof(data));
> > -	ret = set_registers(pegasus, EthCtrl0, 3, data);
> > +	set_registers(pegasus, EthCtrl0, 3, data);
> >  
> >  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> >  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
> 
> This one is not added by the recent changes as I initially thought, 
> the driver has always checked this return value. The recent changes 
> did this:
> 
>         ret = set_registers(pegasus, EthCtrl0, 3, data);
>  
>         if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>             usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
>             usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
>                 u16 auxmode;
> -               read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +               if (ret < 0)
> +                       goto fail;
>                 auxmode |= 4;
>                 write_mii_word(pegasus, 0, 0x1b, &auxmode);
>         }
>  
> +       return 0;
> +fail:
> +       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
>         return ret;
> }
> 
> now the return value of set_registeres() is ignored. 
> 
> Seems like  a better fix would be to bring back the error checking, 
> why not?

Mostly because for this particular adapter checking the read failure makes much
more sense than write failure.

Checking the return value of set_register(s) is often usless because device's
default register values are sane enough to get a working ethernet adapter even
without much prodding.  There are exceptions, though, one of them being
set_ethernet_addr().

You could read the discussing in the netdev ML, but the essence of it is that
set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
driver should assign a valid, even if random, MAC address.

It is much the same situation with enable_net_traffic() - it should continue
regardless.  There are two options to resolve this: a) remove the error check
altogether; b) do the check and print a debug message.  I prefer a), but i am
also not strongly opposed to b).  Comments?

> Please remember to add a fixes tag.

Will do.


cheers,
Petko

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Jakub Kicinski]

On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:
> > > @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
> > >  	data[2] = loopback ? 0x09 : 0x01;
> > >  
> > >  	memcpy(pegasus->eth_regs, data, sizeof(data));
> > > -	ret = set_registers(pegasus, EthCtrl0, 3, data);
> > > +	set_registers(pegasus, EthCtrl0, 3, data);
> > >  
> > >  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> > >  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||  
> > 
> > This one is not added by the recent changes as I initially thought, 
> > the driver has always checked this return value. The recent changes 
> > did this:
> > 
> >         ret = set_registers(pegasus, EthCtrl0, 3, data);
> >  
> >         if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> >             usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
> >             usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
> >                 u16 auxmode;
> > -               read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > +               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > +               if (ret < 0)
> > +                       goto fail;
> >                 auxmode |= 4;
> >                 write_mii_word(pegasus, 0, 0x1b, &auxmode);
> >         }
> >  
> > +       return 0;
> > +fail:
> > +       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> >         return ret;
> > }
> > 
> > now the return value of set_registeres() is ignored. 
> > 
> > Seems like  a better fix would be to bring back the error checking, 
> > why not?  
> 
> Mostly because for this particular adapter checking the read failure makes much
> more sense than write failure.

This is not an either-or choice.

> Checking the return value of set_register(s) is often usless because device's
> default register values are sane enough to get a working ethernet adapter even
> without much prodding.  There are exceptions, though, one of them being
> set_ethernet_addr().
> 
> You could read the discussing in the netdev ML, but the essence of it is that
> set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
> driver should assign a valid, even if random, MAC address.
> 
> It is much the same situation with enable_net_traffic() - it should continue
> regardless.  There are two options to resolve this: a) remove the error check
> altogether; b) do the check and print a debug message.  I prefer a), but i am
> also not strongly opposed to b).  Comments?

c) keep propagating the error like the driver used to.

I don't understand why that's not the most obvious option.

The driver used to propagate the errors from the set_registers() call
in enable_net_traffic() since the beginning of the git era. This is 
_not_ one of the error checking that you recently added.

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Petko Manolov]

On 21-08-16 07:06:40, Jakub Kicinski wrote:
> On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:
> > > > @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
> > > >  	data[2] = loopback ? 0x09 : 0x01;
> > > >  
> > > >  	memcpy(pegasus->eth_regs, data, sizeof(data));
> > > > -	ret = set_registers(pegasus, EthCtrl0, 3, data);
> > > > +	set_registers(pegasus, EthCtrl0, 3, data);
> > > >  
> > > >  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> > > >  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||  
> > > 
> > > This one is not added by the recent changes as I initially thought, 
> > > the driver has always checked this return value. The recent changes 
> > > did this:
> > > 
> > >         ret = set_registers(pegasus, EthCtrl0, 3, data);
> > >  
> > >         if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> > >             usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
> > >             usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
> > >                 u16 auxmode;
> > > -               read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > > +               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > > +               if (ret < 0)
> > > +                       goto fail;
> > >                 auxmode |= 4;
> > >                 write_mii_word(pegasus, 0, 0x1b, &auxmode);
> > >         }
> > >  
> > > +       return 0;
> > > +fail:
> > > +       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> > >         return ret;
> > > }
> > > 
> > > now the return value of set_registeres() is ignored. 
> > > 
> > > Seems like  a better fix would be to bring back the error checking, 
> > > why not?  
> > 
> > Mostly because for this particular adapter checking the read failure makes much
> > more sense than write failure.
> 
> This is not an either-or choice.
> 
> > Checking the return value of set_register(s) is often usless because device's
> > default register values are sane enough to get a working ethernet adapter even
> > without much prodding.  There are exceptions, though, one of them being
> > set_ethernet_addr().
> > 
> > You could read the discussing in the netdev ML, but the essence of it is that
> > set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
> > driver should assign a valid, even if random, MAC address.
> > 
> > It is much the same situation with enable_net_traffic() - it should continue
> > regardless.  There are two options to resolve this: a) remove the error check
> > altogether; b) do the check and print a debug message.  I prefer a), but i am
> > also not strongly opposed to b).  Comments?
> 
> c) keep propagating the error like the driver used to.

If you carefully read the code, which dates back to at least 2005, you'll see
that on line 436 (v5.14-rc6) 'ret' is assigned with the return value of
set_registers(), but 'ret' is never evaluated and thus not acted upon.

> I don't understand why that's not the most obvious option.

Which part of "this is not a fatal error" you did not understand?

> The driver used to propagate the errors from the set_registers() call in
> enable_net_traffic() since the beginning of the git era. This is _not_ one of
> the error checking that you recently added.

The driver hasn't propagated an error at this particular location in the last 16
years.  So how exactly removing this assignment will make the driver worse than
it is now?

Anyway, i'll add a warn() and be done with it.


		Petko

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Jakub Kicinski]

On Mon, 16 Aug 2021 22:14:47 +0300 Petko Manolov wrote:
> On 21-08-16 07:06:40, Jakub Kicinski wrote:
> > On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:  
> > > Mostly because for this particular adapter checking the read failure makes much
> > > more sense than write failure.  
> > 
> > This is not an either-or choice.
> >   
> > > Checking the return value of set_register(s) is often usless because device's
> > > default register values are sane enough to get a working ethernet adapter even
> > > without much prodding.  There are exceptions, though, one of them being
> > > set_ethernet_addr().
> > > 
> > > You could read the discussing in the netdev ML, but the essence of it is that
> > > set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
> > > driver should assign a valid, even if random, MAC address.
> > > 
> > > It is much the same situation with enable_net_traffic() - it should continue
> > > regardless.  There are two options to resolve this: a) remove the error check
> > > altogether; b) do the check and print a debug message.  I prefer a), but i am
> > > also not strongly opposed to b).  Comments?  
> > 
> > c) keep propagating the error like the driver used to.  
> 
> If you carefully read the code, which dates back to at least 2005, you'll see
> that on line 436 (v5.14-rc6) 'ret' is assigned with the return value of
> set_registers(), but 'ret' is never evaluated and thus not acted upon.

It's no longer evaluated because of your commit 8a160e2e9aeb ("net:
usb: pegasus: Check the return value of get_geristers() and friends;")
IOW v5.14-rc6 has your recent patch. Which I quoted earlier in this
thread. That commit was on Aug 3 2021. The error checking (now
accidentally removed) was introduced somewhere in 2.6.x days.

If you disagree with that please show me the code you're referring to,
because I just don't see it.

> > I don't understand why that's not the most obvious option.  
> 
> Which part of "this is not a fatal error" you did not understand?

That's not the point. The error checking was removed accidentally, 
it should be brought back in net to avoid introducing regressions.
If the error checking is not necessary you can remove it in net-next,
no problem.

Perhaps you did not intend commit 8a160e2e9aeb ("net: usb: pegasus:
Check the return value of get_geristers() and friends;") to be applied 
as a fix but it was, and it was backported to stable trees if I'm not
mistaken. 

> > The driver used to propagate the errors from the set_registers() call in
> > enable_net_traffic() since the beginning of the git era. This is _not_ one of
> > the error checking that you recently added.  
> 
> The driver hasn't propagated an error at this particular location in the last 16
> years.  So how exactly removing this assignment will make the driver worse than
> it is now?

This is the relevant code from v5.13:

static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
			 const void *data)
{
	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
				    PEGASUS_REQT_WRITE, 0, indx, data, size,
				    1000, GFP_NOIO);
}

static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
{
	/* [...] */
===>	ret = set_registers(pegasus, EthCtrl0, 3, data);

	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
		u16 auxmode;
		read_mii_word(pegasus, 0, 0x1b, &auxmode);
		auxmode |= 4;
		write_mii_word(pegasus, 0, 0x1b, &auxmode);
	}

===>	return ret;
}


static int pegasus_open(struct net_device *net)
{
	/* [...] */
===>	res = enable_net_traffic(net, pegasus->usb);
===>	if (res < 0) {
		netif_dbg(pegasus, ifup, net,
			  "can't enable_net_traffic() - %d\n", res);
===>		res = -EIO;
		usb_kill_urb(pegasus->rx_urb);
		usb_kill_urb(pegasus->intr_urb);
===>		goto exit;
	}
	set_carrier(net);
	netif_start_queue(net);
	netif_dbg(pegasus, ifup, net, "open\n");
	res = 0;
exit:
===>	return res;
}

https://elixir.bootlin.com/linux/v5.13/source/drivers/net/usb/pegasus.c

Re: [PATCH] net: usb: pegasus: ignore the return value from set_registers();

[Petko Manolov]

On 21-08-16 13:18:15, Jakub Kicinski wrote:
> On Mon, 16 Aug 2021 22:14:47 +0300 Petko Manolov wrote:
> > On 21-08-16 07:06:40, Jakub Kicinski wrote:
> > > On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:  
> > > > Mostly because for this particular adapter checking the read failure makes much
> > > > more sense than write failure.  
> > > 
> > > This is not an either-or choice.
> > >   
> > > > Checking the return value of set_register(s) is often usless because device's
> > > > default register values are sane enough to get a working ethernet adapter even
> > > > without much prodding.  There are exceptions, though, one of them being
> > > > set_ethernet_addr().
> > > > 
> > > > You could read the discussing in the netdev ML, but the essence of it is that
> > > > set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
> > > > driver should assign a valid, even if random, MAC address.
> > > > 
> > > > It is much the same situation with enable_net_traffic() - it should continue
> > > > regardless.  There are two options to resolve this: a) remove the error check
> > > > altogether; b) do the check and print a debug message.  I prefer a), but i am
> > > > also not strongly opposed to b).  Comments?  
> > > 
> > > c) keep propagating the error like the driver used to.  
> > 
> > If you carefully read the code, which dates back to at least 2005, you'll see
> > that on line 436 (v5.14-rc6) 'ret' is assigned with the return value of
> > set_registers(), but 'ret' is never evaluated and thus not acted upon.
> 
> It's no longer evaluated because of your commit 8a160e2e9aeb ("net:
> usb: pegasus: Check the return value of get_geristers() and friends;")
> IOW v5.14-rc6 has your recent patch. Which I quoted earlier in this
> thread. That commit was on Aug 3 2021. The error checking (now
> accidentally removed) was introduced somewhere in 2.6.x days.
> 
> If you disagree with that please show me the code you're referring to,
> because I just don't see it.
> 
> > > I don't understand why that's not the most obvious option.  
> > 
> > Which part of "this is not a fatal error" you did not understand?
> 
> That's not the point. The error checking was removed accidentally, it should
> be brought back in net to avoid introducing regressions.

Not introducing regressions is the argument that sold me on.  If i don't get too
lazy i may further change this part of the code, but that's in the future.  I've
sent a patch that restores the original behavior in addition of a few small
tweaks.


		Petko