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

On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:

quoted

quoted

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