On 21-08-16 13:18:15, Jakub Kicinski wrote:
> On Mon, 16 Aug 2021 22:14:47 +0300 Petko Manolov wrote:
> > On 21-08-16 07:06:40, Jakub Kicinski wrote:
> > > On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:
> > > > 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.
> >
> > If you carefully read the code, which dates back to at least 2005, you'll see
> > that on line 436 (v5.14-rc6) 'ret' is assigned with the return value of
> > set_registers(), but 'ret' is never evaluated and thus not acted upon.
>
> It's no longer evaluated because of your commit 8a160e2e9aeb ("net:
> usb: pegasus: Check the return value of get_geristers() and friends;")
> IOW v5.14-rc6 has your recent patch. Which I quoted earlier in this
> thread. That commit was on Aug 3 2021. The error checking (now
> accidentally removed) was introduced somewhere in 2.6.x days.
>
> If you disagree with that please show me the code you're referring to,
> because I just don't see it.
>
> > > I don't understand why that's not the most obvious option.
> >
> > Which part of "this is not a fatal error" you did not understand?
>
> That's not the point. The error checking was removed accidentally, it should
> be brought back in net to avoid introducing regressions.
Not introducing regressions is the argument that sold me on. If i don't get too
lazy i may further change this part of the code, but that's in the future. I've
sent a patch that restores the original behavior in addition of a few small
tweaks.
Petko
