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

On 21-08-16 13:18:15, Jakub Kicinski wrote:

On Mon, 16 Aug 2021 22:14:47 +0300 Petko Manolov wrote:

quoted

On 21-08-16 07:06:40, Jakub Kicinski wrote:

quoted

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

quoted

Mostly because for this particular adapter checking the read failure makes much
more sense than write failure.  

This is not an either-or choice.
  

quoted

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.

quoted

quoted

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