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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0c6bc63e-2bc8-2479-ed22-ba174493478e@aquantia.com>
Date:   Mon, 8 Oct 2018 14:12:59 +0000
From:   Igor Russkikh <Igor.Russkikh@...antia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "David S . Miller" <davem@...emloft.net>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on
 LAN by MAGIC packet

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.


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

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

Regards,
  Igor