On Tue, 24 Aug 2021 09:59:12 +0100 John Efstathiades wrote:

quoted

quoted

+/* Enabling link power state transitions will reduce power consumption
+ * when the link is idle. However, this can cause problems with some
+ * USB3 hubs resulting in erratic packet flow.
+ */
+static bool enable_link_power_states;  

How is the user supposed to control this? Are similar issues not
addressed at the USB layer? There used to be a "no autosuspend"
flag that all netdev drivers set..  

The change is specific to U1 and U2 transitions initiated by the device
itself and does not affect ability of the device to respond to
host-initiated U1 and U2 transitions.

There is no user access to this. The driver would have to be recompiled to
change the default. 

Do you expect the device-initiated transitions to always be causing
trouble or are there scenarios where they are useful?

Having to recompile the driver is a middle ground rarely chosen
upstream. If the code has very low chance of being useful - let's
remove it (git will hold it forever if needed); if there are reasonable
chances someone will find it useful it should be configurable from user
space, or preferably automatically enabled based on some device match
list.

quoted

Was linux-usb consulted? Adding the list to Cc.

 
No, they weren't, but the change was discussed with the driver maintainer at
Microchip.

Good to hear manufacturer is advising but the Linux USB community 
may have it's own preferences / experience.

quoted

quoted

 		/* reset MAC */
 		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
 		if (unlikely(ret < 0))
-			return -EIO;
+			return ret;
 		buf |= MAC_CR_RST_;
 		ret = lan78xx_write_reg(dev, MAC_CR, buf);
 		if (unlikely(ret < 0))
-			return -EIO;
+			return ret;  

Please split the ret code changes to a separate, earlier patch.  

There are ret code changes in later patches in this set. As a general, rule
should ret code changes be put into their own patch?

It's case by case, in this patch the ret code changes and error
propagation does not seem to be inherently related to the main 
change the patch is making. I think you're referring to patch 7 -
similar comment indeed applies there. I'd recommend taking the 
error propagation changes into a separate patch (can be a single 
one for code extracted from both patches).