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