From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet Date: Mon, 8 Oct 2018 16:47:04 +0200 Message-ID: <20181008144704.GS4730@lunn.ch> References: <1a7d4af1b3a6397b76338d543f86bb4c327f5060.1538734658.git.igor.russkikh@aquantia.com> <20181006174921.GG6990@lunn.ch> <0c6bc63e-2bc8-2479-ed22-ba174493478e@aquantia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "David S . Miller" , "linux-usb@vger.kernel.org" , "netdev@vger.kernel.org" , Dmitry Bezrukov To: Igor Russkikh Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:57159 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726445AbeJHV7N (ORCPT ); Mon, 8 Oct 2018 17:59:13 -0400 Content-Disposition: inline In-Reply-To: <0c6bc63e-2bc8-2479-ed22-ba174493478e@aquantia.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [net-next,19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet From: Andrew Lunn Message-Id: <20181008144704.GS4730@lunn.ch> Date: Mon, 8 Oct 2018 16:47:04 +0200 To: Igor Russkikh Cc: "David S . Miller" , "linux-usb@vger.kernel.org" , "netdev@vger.kernel.org" , Dmitry Bezrukov List-ID: T24gTW9uLCBPY3QgMDgsIDIwMTggYXQgMDI6MTI6NTlQTSArMDAwMCwgSWdvciBSdXNza2lraCB3 cm90ZToKPiBIaSBBbmRyZXcsCj4gCj4gPj4gKwkJaWYgKGFxYzExMV9kYXRhLT5kcGEpIHsKPiA+ PiArCQkJYXFjMTExX3NldF9waHlfc3BlZWQoZGV2LCBBVVRPTkVHX0RJU0FCTEUsIFNQRUVEXzEw MCk7Cj4gPiAKPiA+IEkgZG9uJ3QgdGhpbmsgdGhhdCB3b3Jrcy4gWW91IHNob3VsZCBsZWF2ZSBB VVRPTkVHIG9uLCBidXQgb25seQo+ID4gYWR2ZXJ0aXNlIFNQRUVEXzEwMCBhbmQgdHJpZ2dlciBh dXRvLW5lZy4gSWYgeW91IGZvcmNlIGl0IHRvIDEwMCwKPiA+IHRoZXJlIGlzIG5vIGd1YXJhbnRl ZSB0aGUgcGVlciB3aWxsIGZpZ3VyZSBvdXQgd2hhdCB0aGUgbmV3IGxpbmsgc3BlZWQKPiA+IGlz LiBJJ3ZlIG9mdGVuIHNlZW4gZmFpbGVkIGF1dG8tbmV0IHJlc3VsdCBpbiAxMC9IYWxmLiBTbyB5 b3Ugd2lsbAo+ID4gbG9vc2UgdGhlIGxpbmssIG1ha2luZyBXb0wgcG9pbnRsZXNzLgo+IAo+IFBo eSBkb2VzIG5vdCBzdXBwb3J0IDEwTSwgbG93IHBvd2VyIG1vZGUgZXhwbGljaXRseSB1c2VzIDEw ME0KPiBmb3IgcG93ZXIgc2FmZXR5IHJlYXNvbnMuCj4gCj4gSXQgaXMgbWVhbmluZ2xlc3MgaGVy ZSB0byBhZGQgQXV0b25lZyB0byAxMDBNIGJlY2F1c2UgdGhhdHMgdGhlIG9ubHkKPiBzcGVlZG1h c2sgYml0IGFueXdheS4KCklmIHlvdSBoYXZlIEFVVE9ORUdfRElTQUJMRSwgaSB3b3VsZCBhc3N1 bWUgeW91IFBIWSBpcyBub3QgZXZlbiB0cnlpbmcKdG8gYXV0b19uZWcuIFNvIHRoZSBzcGVlZG1h c2sgaXMgaXJyZWxldmVudCwgaXQgaXMgbm90IHNlbnQgdG8gdGhlCnBlZXIuIEFuZCBzaW5jZSB0 aGUgcGVlciBpcyBub3QgcmVjZWl2aW5nIGFueSBhdXRvLW5lZyBpbmZvcm1hdGlvbiwgaXQKd2ls bCBmYWlsIHRvIGF1dG8tbmVnLCBhbmQgbW9zdCBsaWtlbHkgZGVmYXVsdCB0byAxMC9IYWxmLgoK VG8gZG8gdGhpcyByaWdodCwgcGxlYXNlIHRha2UgYSBsb29rIGF0IHRoaXMgY29tbWl0Cgpjb21t aXQgMmI5NjcyZGRiNmYzNDc0NjdkN2IzM2I4NmM1ZGZjNGQ1YzA1MDFhOApBdXRob3I6IEhlaW5l ciBLYWxsd2VpdCA8aGthbGx3ZWl0MUBnbWFpbC5jb20+CkRhdGU6ICAgVGh1IEp1bCAxMiAyMToz Mjo1MyAyMDE4ICswMjAwCgogICAgbmV0OiBwaHk6IGFkZCBwaHlfc3BlZWRfZG93biBhbmQgcGh5 X3NwZWVkX3VwCiAgICAKICAgIFNvbWUgbmV0d29yayBkcml2ZXJzIGluY2x1ZGUgZnVuY3Rpb25h bGl0eSB0byBzcGVlZCBkb3duIHRoZSBQSFkgd2hlbgogICAgc3VzcGVuZGluZyBhbmQganVzdCB3 YWl0aW5nIGZvciBhIFdvTCBwYWNrZXQgYmVjYXVzZSB0aGlzIHNhdmVzIGVuZXJneS4KICAgIFRo aXMgZnVuY3Rpb25hbGl0eSBpcyBxdWl0ZSBnZW5lcmljLCB0aGVyZWZvcmUgbGV0J3MgZmFjdG9y IGl0IG91dCB0bwogICAgcGh5bGliLgogICAgCj4gCj4gPj4gKwlhcWMxMTFfc2V0X3BoeV9zcGVl ZChkZXYsIGFxYzExMV9kYXRhLT5hdXRvbmVnLAo+ID4+ICsJCQkgICAgIGFxYzExMV9kYXRhLT5h ZHZlcnRpc2VkX3NwZWVkKTsKPiA+PiArCj4gPiAKPiA+IFNob3VsZCB0aGF0IGJlIGNvbmRpdGlv bmFsIG9uIGFxYzExMV9kYXRhLT5kcGE/Cj4gCj4gQWN0dWFsbHkgbm8sIGJlY2F1c2Ugc2V0X3Bo eV9zcGVlZCBpbnRlcm5hbGx5IGNoZWNrcyB0aGlzIGZsYWcuCgpTbyB5b3Ugc2hvdWxkIHByb2Jh Ymx5IHJlbW92ZSB0aGUgY2hlY2sgYWJvdmUgd2hlbiBmb3JjaW5nIHRoZSBzcGVlZAp0byAxMDAu IE1ha2UgdGhlIGNvZGUgc3ltbWV0cmljYWwuIAoKPiAKPiA+PiArCXU4IHJzdmRbMjgzXTsKPiA+ PiArfTsKPiA+IAo+ID4gRG8geW91IHJlYWxseSBuZWVkIHRoZXNlIDI4MyBieXRlcz8/Cj4gCj4g Pj4gIAlzdHJ1Y3QgYXFjMTExX3BoeV9vcHRpb25zIHBoeV9vcHM7Cj4gPj4gKwlzdHJ1Y3QgYXFj MTExX3dvbF9jZmcgd29sX2NmZzsKPiA+IAo+ID4gVGhvc2UgMjgzIGJ5dGVzIG1ha2UgdGhpcyB3 aG9sZSBzdHJ1Y3R1cmUgYmlnZ2VyLi4uCj4gCj4gRlcgaW50ZXJmYWNlIGV4cGVjdHMgdGhlIFdP TCBjb25maWcgcmVxdWVzdCBXT0xfQ0ZHX1NJWkUgYnl0ZXMuCj4gVGhlc2UgcmVzZXJ2ZWQgZmll bGRzIGFyZSBqdXN0IG5vdCB1c2VkIG5vdyBieSBsaW51eCBkcml2ZXIuCj4gVGhleSBjb25maWd1 cmUgZXh0cmEgd29sIGZlYXR1cmVzIGxpa2UgYSBzbGVlcCBwcm94eS4KPiBUaHVzLCB3ZSBhbnl3 YXkgaGF2ZSB0byBhbGxvY2F0ZSB0aGlzIHNvbWV3aGVyZS4KCldlbGwsIGkgdGhpbmsgeW91ciBs b3cgbGV2ZWwgZnVuY3Rpb24gZm9yIGFjdHVhbGx5IHNlbmRpbmcgYSBjb21tYW5kCmRvZXMgYSBk dXAgYmVmb3JlIHNlbmRpbmcuIFlvdSBkb24ndCBhY3R1YWxseSBzZW5kIHRoaXMsIHlvdSBzZW5k IGEKY29weS4gTWF5YmUgeW91IGNhbiBwYWQgaXQgb3V0IHRoZW4/CgogICAgICBBbmRyZXcK