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