| Message ID | 20210823135229.36581-6-john.efstathiades@pebblebay.com |
|---|---|
| State | New |
| Headers | show |
| Series | LAN7800 driver improvements | expand |
On Mon, 23 Aug 2021 14:52:24 +0100 John Efstathiades wrote: > Disable USB3 link power state transitions from U0 (Fully Powered) to > U1 (Standby with Fast Recovery) or U2 (Standby with Slow Recovery). > > The device can initiate U1 and U2 state transitions when there is no > activity on the bus which can save power. However, testing with some > USB3 hosts and hubs showed erratic ping response time due to the time > required to transition back to U0 state. > > In the following example the outgoing packets were delayed until the > device transitioned from U2 back to U0 giving the misleading > response time. > > console:/data/local # ping 192.168.73.1 > PING 192.168.73.1 (192.168.73.1) 56(84) bytes of data. > 64 bytes from 192.168.73.1: icmp_seq=1 ttl=64 time=466 ms > 64 bytes from 192.168.73.1: icmp_seq=2 ttl=64 time=225 ms > 64 bytes from 192.168.73.1: icmp_seq=3 ttl=64 time=155 ms > 64 bytes from 192.168.73.1: icmp_seq=4 ttl=64 time=7.07 ms > 64 bytes from 192.168.73.1: icmp_seq=5 ttl=64 time=141 ms > 64 bytes from 192.168.73.1: icmp_seq=6 ttl=64 time=152 ms > 64 bytes from 192.168.73.1: icmp_seq=7 ttl=64 time=51.9 ms > 64 bytes from 192.168.73.1: icmp_seq=8 ttl=64 time=136 ms > > The following shows the behaviour when the U1 and U2 transitions > were disabled. > > console:/data/local # ping 192.168.73.1 > PING 192.168.73.1 (192.168.73.1) 56(84) bytes of data. > 64 bytes from 192.168.73.1: icmp_seq=1 ttl=64 time=6.66 ms > 64 bytes from 192.168.73.1: icmp_seq=2 ttl=64 time=2.97 ms > 64 bytes from 192.168.73.1: icmp_seq=3 ttl=64 time=2.02 ms > 64 bytes from 192.168.73.1: icmp_seq=4 ttl=64 time=2.42 ms > 64 bytes from 192.168.73.1: icmp_seq=5 ttl=64 time=2.47 ms > 64 bytes from 192.168.73.1: icmp_seq=6 ttl=64 time=2.55 ms > 64 bytes from 192.168.73.1: icmp_seq=7 ttl=64 time=2.43 ms > 64 bytes from 192.168.73.1: icmp_seq=8 ttl=64 time=2.13 ms > > Signed-off-by: John Efstathiades <john.efstathiades@pebblebay.com> > --- > drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 39 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index 746aeeaa9d6e..3181753b1621 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -430,6 +430,12 @@ struct lan78xx_net { > #define PHY_LAN8835 (0x0007C130) > #define PHY_KSZ9031RNX (0x00221620) > > +/* 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.. Was linux-usb consulted? Adding the list to Cc. > /* use ethtool to change the level for any given device */ > static int msg_level = -1; > module_param(msg_level, int, 0); > @@ -1173,7 +1179,7 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) > /* clear LAN78xx interrupt status */ > ret = lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_); > if (unlikely(ret < 0)) > - return -EIO; > + return ret; > > mutex_lock(&phydev->lock); > phy_read_status(phydev); > @@ -1186,11 +1192,11 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) > /* 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. > del_timer(&dev->stat_monitor); > } else if (link && !dev->link_on) {
> Do you expect the device-initiated transitions to always be causing > trouble or are there scenarios where they are useful? It's a particular problem on Android devices. > 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. I like the sound of the device match list but I don't know what you mean. Is there a driver or other reference you could point me at that provides additional info? > > > 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. Understood. > > > 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). Thanks, I am working on this and have incorporated the error propagation changes from patch 7.
On Tue, 24 Aug 2021 15:33:13 +0100 John Efstathiades wrote: > > Do you expect the device-initiated transitions to always be causing > > trouble or are there scenarios where they are useful? > > It's a particular problem on Android devices. > > > 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. > > I like the sound of the device match list but I don't know what you mean. > Is there a driver or other reference you could point me at that provides > additional info? Depends on what the discriminator is. If problems happen with a particular ASIC revisions driver needs to read the revision out and make a match. If it's product by product you can use struct usb_device_id :: driver_info to attach metadata per device ID. If it's related to the platform things like DMI matching are sometimes used. I have very limited experience with Android / embedded ARM so not sure what would work there.
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 746aeeaa9d6e..3181753b1621 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -430,6 +430,12 @@ struct lan78xx_net { #define PHY_LAN8835 (0x0007C130) #define PHY_KSZ9031RNX (0x00221620) +/* 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; + /* use ethtool to change the level for any given device */ static int msg_level = -1; module_param(msg_level, int, 0); @@ -1173,7 +1179,7 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) /* clear LAN78xx interrupt status */ ret = lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_); if (unlikely(ret < 0)) - return -EIO; + return ret; mutex_lock(&phydev->lock); phy_read_status(phydev); @@ -1186,11 +1192,11 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) /* 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; del_timer(&dev->stat_monitor); } else if (link && !dev->link_on) { @@ -1198,23 +1204,49 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) phy_ethtool_ksettings_get(phydev, &ecmd); - if (dev->udev->speed == USB_SPEED_SUPER) { + if (enable_link_power_states && + dev->udev->speed == USB_SPEED_SUPER) { if (ecmd.base.speed == 1000) { /* disable U2 */ ret = lan78xx_read_reg(dev, USB_CFG1, &buf); + if (ret < 0) + return ret; buf &= ~USB_CFG1_DEV_U2_INIT_EN_; ret = lan78xx_write_reg(dev, USB_CFG1, buf); + if (ret < 0) + return ret; /* enable U1 */ ret = lan78xx_read_reg(dev, USB_CFG1, &buf); + if (ret < 0) + return ret; buf |= USB_CFG1_DEV_U1_INIT_EN_; ret = lan78xx_write_reg(dev, USB_CFG1, buf); + if (ret < 0) + return ret; } else { /* enable U1 & U2 */ ret = lan78xx_read_reg(dev, USB_CFG1, &buf); + if (ret < 0) + return ret; buf |= USB_CFG1_DEV_U2_INIT_EN_; buf |= USB_CFG1_DEV_U1_INIT_EN_; ret = lan78xx_write_reg(dev, USB_CFG1, buf); + if (ret < 0) + return ret; } + } else { + /* Disabling initiation of U1 and U2 transitions + * prevents erratic ping times when connected to + * some USB3 hubs. + */ + ret = lan78xx_read_reg(dev, USB_CFG1, &buf); + if (ret < 0) + return ret; + buf &= ~USB_CFG1_DEV_U2_INIT_EN_; + buf &= ~USB_CFG1_DEV_U1_INIT_EN_; + ret = lan78xx_write_reg(dev, USB_CFG1, buf); + if (ret < 0) + return ret; } ladv = phy_read(phydev, MII_ADVERTISE); @@ -1231,6 +1263,8 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) ret = lan78xx_update_flowcontrol(dev, ecmd.base.duplex, ladv, radv); + if (ret < 0) + return ret; if (!timer_pending(&dev->stat_monitor)) { dev->delta = 1; @@ -1241,7 +1275,7 @@ static int lan78xx_link_reset(struct lan78xx_net *dev) tasklet_schedule(&dev->bh); } - return ret; + return 0; } /* some work can't be done in tasklets, so we use keventd
Disable USB3 link power state transitions from U0 (Fully Powered) to U1 (Standby with Fast Recovery) or U2 (Standby with Slow Recovery). The device can initiate U1 and U2 state transitions when there is no activity on the bus which can save power. However, testing with some USB3 hosts and hubs showed erratic ping response time due to the time required to transition back to U0 state. In the following example the outgoing packets were delayed until the device transitioned from U2 back to U0 giving the misleading response time. console:/data/local # ping 192.168.73.1 PING 192.168.73.1 (192.168.73.1) 56(84) bytes of data. 64 bytes from 192.168.73.1: icmp_seq=1 ttl=64 time=466 ms 64 bytes from 192.168.73.1: icmp_seq=2 ttl=64 time=225 ms 64 bytes from 192.168.73.1: icmp_seq=3 ttl=64 time=155 ms 64 bytes from 192.168.73.1: icmp_seq=4 ttl=64 time=7.07 ms 64 bytes from 192.168.73.1: icmp_seq=5 ttl=64 time=141 ms 64 bytes from 192.168.73.1: icmp_seq=6 ttl=64 time=152 ms 64 bytes from 192.168.73.1: icmp_seq=7 ttl=64 time=51.9 ms 64 bytes from 192.168.73.1: icmp_seq=8 ttl=64 time=136 ms The following shows the behaviour when the U1 and U2 transitions were disabled. console:/data/local # ping 192.168.73.1 PING 192.168.73.1 (192.168.73.1) 56(84) bytes of data. 64 bytes from 192.168.73.1: icmp_seq=1 ttl=64 time=6.66 ms 64 bytes from 192.168.73.1: icmp_seq=2 ttl=64 time=2.97 ms 64 bytes from 192.168.73.1: icmp_seq=3 ttl=64 time=2.02 ms 64 bytes from 192.168.73.1: icmp_seq=4 ttl=64 time=2.42 ms 64 bytes from 192.168.73.1: icmp_seq=5 ttl=64 time=2.47 ms 64 bytes from 192.168.73.1: icmp_seq=6 ttl=64 time=2.55 ms 64 bytes from 192.168.73.1: icmp_seq=7 ttl=64 time=2.43 ms 64 bytes from 192.168.73.1: icmp_seq=8 ttl=64 time=2.13 ms Signed-off-by: John Efstathiades <john.efstathiades@pebblebay.com> --- drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)