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.
