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

On Mon, Oct 08, 2018 at 02:12:59PM +0000, Igor Russkikh wrote:

Hi Andrew,

quoted

quoted

+		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 [off-list ref]
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.
    

quoted

quoted

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

quoted

quoted

+	u8 rsvd[283];
+};

Do you really need these 283 bytes??

quoted

quoted

 	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