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

[Petko Manolov <petko.manolov@konsulko.com>]

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