Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Oct 08, 2018 at 02:12:59PM +0000, Igor Russkikh wrote:
> Hi Andrew,
> 
> >> +		if (aqc111_data->dpa) {
> >> +			aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
> > 
> > I don't think that works. You should leave AUTONEG on, but only
> > advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> > there is no guarantee the peer will figure out what the new link speed
> > is. I've often seen failed auto-net result in 10/Half. So you will
> > loose the link, making WoL pointless.
> 
> Phy does not support 10M, low power mode explicitly uses 100M
> for power safety reasons.
> 
> It is meaningless here to add Autoneg to 100M because thats the only
> speedmask bit anyway.

If you have AUTONEG_DISABLE, i would assume you PHY is not even trying
to auto_neg. So the speedmask is irrelevent, it is not sent to the
peer. And since the peer is not receiving any auto-neg information, it
will fail to auto-neg, and most likely default to 10/Half.

To do this right, please take a look at this commit

commit 2b9672ddb6f347467d7b33b86c5dfc4d5c0501a8
Author: Heiner Kallweit <hkallweit1@gmail.com>
Date:   Thu Jul 12 21:32:53 2018 +0200

    net: phy: add phy_speed_down and phy_speed_up
    
    Some network drivers include functionality to speed down the PHY when
    suspending and just waiting for a WoL packet because this saves energy.
    This functionality is quite generic, therefore let's factor it out to
    phylib.
    
> 
> >> +	aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> >> +			     aqc111_data->advertised_speed);
> >> +
> > 
> > Should that be conditional on aqc111_data->dpa?
> 
> Actually no, because set_phy_speed internally checks this flag.

So you should probably remove the check above when forcing the speed
to 100. Make the code symmetrical. 

> 
> >> +	u8 rsvd[283];
> >> +};
> > 
> > Do you really need these 283 bytes??
> 
> >>  	struct aqc111_phy_options phy_ops;
> >> +	struct aqc111_wol_cfg wol_cfg;
> > 
> > Those 283 bytes make this whole structure bigger...
> 
> FW interface expects the WOL config request WOL_CFG_SIZE bytes.
> These reserved fields are just not used now by linux driver.
> They configure extra wol features like a sleep proxy.
> Thus, we anyway have to allocate this somewhere.

Well, i think your low level function for actually sending a command
does a dup before sending. You don't actually send this, you send a
copy. Maybe you can pad it out then?

      Andrew