Re: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

Re: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[Jakub Kicinski]

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) {

RE: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[John Efstathiades]

> > +/* 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. 

> 
> Was linux-usb consulted? Adding the list to Cc.
> 
No, they weren't, but the change was discussed with the driver maintainer at
Microchip.

> >  		/* 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?

Re: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[Jakub Kicinski]

On Tue, 24 Aug 2021 09:59:12 +0100 John Efstathiades wrote:
> > > +/* 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.

> > 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.

> > >  		/* 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).

RE: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[John Efstathiades]

> 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.

Re: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[Jakub Kicinski]

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.

RE: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

[John Efstathiades]

> > 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.

Thanks. Based on that I will remove this change from the patch set and
discuss again with the driver maintainer.