From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Douthit Subject: Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus Date: Mon, 3 Dec 2018 17:59:28 +0000 Message-ID: References: <20181203163227.5107-1-stephend@silicom-usa.com> <20181203163227.5107-2-stephend@silicom-usa.com> <20181203165445.GI25748@lunn.ch> <70350444-f05f-c57f-d4d8-0e059b4d896c@silicom-usa.com> <20181203172116.GJ25748@lunn.ch> Reply-To: Steve Douthit Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: Jeff Kirsher , "David S. Miller" , "intel-wired-lan@lists.osuosl.org" , "netdev@vger.kernel.org" , Florian Fainelli To: Andrew Lunn Return-path: Received: from mail-eopbgr130102.outbound.protection.outlook.com ([40.107.13.102]:46119 "EHLO EUR01-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726401AbeLCR7h (ORCPT ); Mon, 3 Dec 2018 12:59:37 -0500 In-Reply-To: <20181203172116.GJ25748@lunn.ch> Content-Language: en-US Content-ID: <520A880BBBA94C4EA1B9EC296B11D93F@eurprd04.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: T24gMTIvMy8xOCAxMjoyMSBQTSwgQW5kcmV3IEx1bm4gd3JvdGU6DQo+IE9uIE1vbiwgRGVjIDAz LCAyMDE4IGF0IDA1OjAyOjQwUE0gKzAwMDAsIFN0ZXZlIERvdXRoaXQgd3JvdGU6DQo+PiBPbiAx Mi8zLzE4IDExOjU0IEFNLCBBbmRyZXcgTHVubiB3cm90ZToNCj4+Pj4gK3N0YXRpYyBzMzIgaXhn YmVfeDU1MGVtX2FfbWlpX2J1c19yZWFkKHN0cnVjdCBtaWlfYnVzICpidXMsIGludCBhZGRyLA0K Pj4+PiArCQkJCSAgICAgICBpbnQgcmVnbnVtKQ0KPj4+PiArew0KPj4+PiArCXN0cnVjdCBpeGdi ZV9hZGFwdGVyICphZGFwdGVyID0gKHN0cnVjdCBpeGdiZV9hZGFwdGVyICopYnVzLT5wcml2Ow0K Pj4+PiArCXN0cnVjdCBpeGdiZV9odyAqaHcgPSAmYWRhcHRlci0+aHc7DQo+Pj4+ICsJdTMyIGdz c3IgPSBody0+cGh5LnBoeV9zZW1hcGhvcmVfbWFzayB8IElYR0JFX0dTU1JfVE9LRU5fU007DQo+ Pj4+ICsNCj4+Pj4gKwlpZiAoaHctPmJ1cy5sYW5faWQpDQo+Pj4+ICsJCWdzc3IgfD0gSVhHQkVf R1NTUl9QSFkxX1NNOw0KPj4+PiArCWVsc2UNCj4+Pj4gKwkJZ3NzciB8PSBJWEdCRV9HU1NSX1BI WTBfU007DQo+Pj4NCj4+PiBIaSBTdGV2ZQ0KPj4+DQo+Pj4gSWYgeW91IG9ubHkgaGF2ZSBvbmUg YnVzLCBkbyB5b3Ugc3RpbGwgbmVlZCB0aGlzPyBPbmUgc2VtYXBob3JlIGlzIGFsbA0KPj4+IHlv dSBuZWVkLiBBbmQgaSdtIG5vdCBldmVuIHN1cmUgeW91IG5lZWQgdGhhdC4gVGhlIE1ESU8gbGF5 ZXIgd2lsbA0KPj4+IHBlcmZvcm0gbG9ja2luZywgYXNzdW1pbmcgZXZlcnl0aGluZyBnb2VzIHRo cm91Z2ggdGhlIE1ESU8gbGF5ZXIuDQo+Pg0KPj4gQUZBSUsgSSBzdGlsbCBuZWVkIHBhcnQgb2Yg aXQuICBUaGVyZSdzIGEgUEhZIHBvbGxpbmcgdW5pdCBpbiB0aGUgY2FyZA0KPj4gdGhhdCB3ZSBu ZWVkIHRvIHN5bmMgd2l0aCBpbmRlcGVuZGVudCBvZiB0aGUgbG9ja2luZyBpbiB0aGUgTURJTyBs YXllci4NCj4+IEkgY2FuIGRyb3AgdGhlIGh3LT5idXMubGFuX2lkIGNoZWNrIHRob3VnaCBhbmQg dW5jb25kaXRpb25hbGx5IE9SIGluIHRoZQ0KPj4gSVhHQkVfR1NTUl9QSFkwX1NNIHNpbmNlIHdl IGFsd2F5cyByZWdpc3RlciBvbiBmdW5jdGlvbiAwIG5vdy4NCj4gDQo+IEhpIFN0ZXZlDQo+IA0K PiBJbiBnZW5lcmFsLCB0aGlzIGlzIG5vdCBlbm91Z2ggdG8gbWFrZSBhIFBIWSBwb2xsaW5nIHVu aXQgc2FmZS4gQXQNCj4gbGVhc3Qgbm90IHdpdGggQzIyIGRldmljZXMuIFRoZXkgb2Z0ZW4gaGF2 ZSBhIHJlZ2lzdGVyIHdoaWNoIGNhbiBiZQ0KPiB1c2VkIHRvIHNlbGVjdCBhIGRpZmZlcmVudCBw YWdlLiBUaGUgc29mdHdhcmUgZHJpdmVyIGNhbiBkbyBhIHdyaXRlIHRvDQo+IHN3YXAgdGhlIHBh Z2UgYXQgYW55IHRpbWUsIGUuZy4gdG8gc2VsZWN0IHRoZSBwYWdlIHdpdGggdGhlDQo+IHRlbXBl cmF0dXJlIHNlbnNvci4gQWZ0ZXIgaXQgcmVsZWFzZXMgdGhlIHNlbWFwaG9yZSBmb3IgdGhhdCB3 cml0ZQ0KPiBjaGFuZ2luZyB0aGUgcGFnZSwgdGhlIGhhcmR3YXJlIGNvdWxkIHRoZW4gcG9sbCB0 aGUgUEhZLCBhbmQgcmVhZCBhDQo+IHRlbXBlcmF0dXJlIHNlbnNvciByZWdpc3RlciBpbnN0ZWFk IG9mIHRoZSBsaW5rIHN0YXR1cywgYW5kIHRoZW4gYmFkDQo+IHRoaW5ncyBoYXBwZW4uDQo+IA0K PiBXaGVuIHVzaW5nIEludGVsJ3MgUEhZIGRyaXZlcnMgZW1iZWRkZWQgd2l0aGluIHRoZSBJbnRl bCBNQUMgZHJpdmVyLA0KPiB0aGlzIGlzIG5vdCBhIHByb2JsZW0uIFRoZXkgY2FuIGF2b2lkIHN3 YXBwaW5nIHRvIGRpZmZlcmVudA0KPiBwYWdlcy4gSG93ZXZlciwgeW91IGFyZSBvbiB0aGUgcGF0 aCB0byBhbGxvdyB0aGUgTGludXggUEhZIGRyaXZlcnMgdG8NCj4gYmUgdXNlZCwgYW5kIHNvbWUg b2YgdGhlbSB3aWxsIGNoYW5nZSB0aGUgcGFnZS4gQW5kIHdpdGggdGhlIGVtYmVkZGVkDQo+IFNP QyB5b3UgYXJlIHdvcmtpbmcgb24sIGkgd291bGQgbm90IGJlIHRvbyBzdXJwcmlzZWQgdG8gc2Vl IHNvbWVib2R5DQo+IGJ1aWxkIGEgc3lzdGVtIHVzaW5nIGEgZGlmZmVyZW50IFBIWSB3aGljaCB0 aGUgSW50ZWwgUEhZIGRyaXZlcnMgZG9uJ3QNCj4gc3VwcG9ydCwgYnV0IGRvZXMgaGF2ZSBhIExp bnV4IFBIWSBkcml2ZXIuDQoNCkFncmVlZCwgYnV0IEknZCBhcmd1ZSBpdCdzIHRoZSBzYW1lIGJl aGF2aW9yIHdlIGhhdmUgdG9kYXkgd2l0aCB0aGUNCmV4aXN0aW5nIE1JSSBpb2N0bHMgaW4gdGhp cyBkcml2ZXIuICBUaGF0J3Mgbm90IHRvIHNheSB0aGlzIGlzIGdvb2QsDQppdCdzIGp1c3Qgbm90 IGFueSBsZXNzIGJyb2tlbiB0aGFuIHRoZSBjdXJyZW50IHN0YXRlIG9mIHRoaW5ncy4gIEkgZG9u J3QNCmtub3cgd2hhdCB0aGUgc2NvcGUgb2YgdGhlIGxvY2tpbmcgaXMgZm9yIHRoZSBzb2Z0d2Fy ZS9maXJtd2FyZQ0Kc2VtYXBob3JlIG9uIHRoZSBmaXJtd2FyZSBzaWRlLiAgTWF5YmUgc29tZW9u ZSBmcm9tIEludGVsIGhhcyBtb3JlDQpkZXRhaWxzIG9uIHRoZSBsb2NraW5nIHRoYXQgd291bGQg aGVscCBmaW5kIGEgY2xldmVyIGxvY2tpbmcgc29sdXRpb24/DQoNCj4gWW91IGFsc28gbmVlZCB0 byB3YXRjaCBvdXQgZm9yIHlvdXIgdXNlIGNhc2Ugb2YgTWFydmVsbA0KPiBtdjg4ZTYzOTAuIFBv bGxpbmcgdGhhdCBtYWtlcyBubyBzZW5zZS4gRG9lcyB0aGUgaGFyZHdhcmUgYWN0dWFsbHkNCj4g cmVjb2duaXNlIGl0IGlzIG5vdCBhIFBIWT8NCg0KQUZBSUNUIHRoZSBwb2xsaW5nIGhhcmR3YXJl IG9ubHkgcG9rZXMgdGhlIGRldmljZSBhZGRyZXNzIHRoYXQgdGhlDQpkcml2ZXIgc3RvcmVzIGlu ICdody0+cGh5Lm1kaW8ucHJ0YWQnLCBzbyB0aGUgUEhZIHBvbGxpbmcgdW5pdCB3b3VsZA0KbmV2 ZXIgc2VlIHRoZSBzd2l0Y2gsIGlmIGl0J3MgZXZlbiBwb2xsaW5nIGF0IGFsbC4gIFNvbWUgb2Yg dGhlIE1BQw0KY29uZmlndXJhdGlvbnMgd2lsbCBzdG9yZSBNRElPX1BSVEFEX05PTkUsIGluIHdo aWNoIGNhc2UgSSB3b3VsZG4ndA0KZXhwZWN0IHRoZSBwb2xsaW5nIHVuaXQgdG8gYmUgYWN0aXZl LiAgSXQncyB1cCB0byB0aGUgYm9hcmQgZGVzaWduZXIgdG8NCmVuc3VyZSB0aGVyZSdzIG5vIGFk ZHJlc3MgY29uZmxpY3RzIG9uIHRoZSBidXMuDQoNCkknbSBtYWtpbmcgc29tZSBhc3N1bXB0aW9u cyBhYm91dCB0aGUgaW5uZXIgd29ya2luZ3Mgb2YgdGhlIGhhcmR3YXJlDQpoZXJlLCBzbyBzb21l b25lIGZyb20gSW50ZWwgd291bGQgbmVlZCB0byBjb25maXJtIHRob3NlIG9yIHNldCBtZQ0Kc3Ry YWlnaHQuDQoNCkhvdyBhYm91dCBpZiBJIGtlZXAgdHJhY2sgb2Ygd2hpY2ggUEhZIGFkZHJlc3Nl cyB0aGUgZHJpdmVyIHdhbnRzIGZvcg0KdGhlIHg1NTBlbV9hIGRldmljZXMsIHRoZW4gY2xlYXIg dGhvc2UgYml0cyBmcm9tIHRoZSBwaHlfbWFzayBpbnN0ZWFkIG9mDQorCWJ1cy0+cGh5X21hc2sg PSBHRU5NQVNLKDMxLCAwKTsNCg0KVGhlbiBpbiB0aGUgaW9jdGwgY29kZSwgaW4gYWRkaXRpb24g dG8gY2hlY2tpbmcgdGhlIG1paV9idXMgaXMNCmF2YWlsYWJsZSwgYWxzbyBjaGVjayB0aGF0IHRo ZSByZXF1ZXN0ZWQgYWRkcmVzcyBpcyBvbmUgdGhhdCBwaHlfbWFzaw0Kc2F5cyBtaWlfYnVzIG93 bnMsIG90aGVyd2lzZSB3ZSBmYWxsIHRocm91Z2ggdG8gdGhlIG9sZCBjb2RlLg0KDQpJIHRoaW5r IHRoYXQga2VlcHMgdGhlIGRzYSBhbmQgUEhZIHBvbGxpbmcgYXMgc2VwYXJhdGUgYXMgaXQgaXMg dG9kYXkNCm9uIHRoZSBpb2N0bCBzaWRlLCBhbmQgaGlkZXMgdGhlIFBIWShzKSB0aGF0IHRoZSBp eGdiZSBkcml2ZXIgd2FudHMgdG8NCm1hbmFnZSBleGNsdXNpdmVseSBmcm9tIHRoZSBtaWlfYnVz IGFsdG9nZXRoZXIuDQoNCkxldCBtZSBrbm93IHdoYXQgeW91IHRoaW5rLiAgSXQnbGwgbWFrZSB0 aGUgcHJvYmluZyBjb2RlIGEgYml0IG1vcmUNCmNvbXBsaWNhdGVkLCBidXQgSSB0aGluayBpdCBh ZGRyZXNzZXMgeW91ciBjb25jZXJucy4NCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Douthit Date: Mon, 3 Dec 2018 17:59:28 +0000 Subject: [Intel-wired-lan] [PATCH net-next v2 1/2] ixgbe: register a mdiobus In-Reply-To: <20181203172116.GJ25748@lunn.ch> References: <20181203163227.5107-1-stephend@silicom-usa.com> <20181203163227.5107-2-stephend@silicom-usa.com> <20181203165445.GI25748@lunn.ch> <70350444-f05f-c57f-d4d8-0e059b4d896c@silicom-usa.com> <20181203172116.GJ25748@lunn.ch> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On 12/3/18 12:21 PM, Andrew Lunn wrote: > On Mon, Dec 03, 2018 at 05:02:40PM +0000, Steve Douthit wrote: >> On 12/3/18 11:54 AM, Andrew Lunn wrote: >>>> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr, >>>> + int regnum) >>>> +{ >>>> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv; >>>> + struct ixgbe_hw *hw = &adapter->hw; >>>> + u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM; >>>> + >>>> + if (hw->bus.lan_id) >>>> + gssr |= IXGBE_GSSR_PHY1_SM; >>>> + else >>>> + gssr |= IXGBE_GSSR_PHY0_SM; >>> >>> Hi Steve >>> >>> If you only have one bus, do you still need this? One semaphore is all >>> you need. And i'm not even sure you need that. The MDIO layer will >>> perform locking, assuming everything goes through the MDIO layer. >> >> AFAIK I still need part of it. There's a PHY polling unit in the card >> that we need to sync with independent of the locking in the MDIO layer. >> I can drop the hw->bus.lan_id check though and unconditionally OR in the >> IXGBE_GSSR_PHY0_SM since we always register on function 0 now. > > Hi Steve > > In general, this is not enough to make a PHY polling unit safe. At > least not with C22 devices. They often have a register which can be > used to select a different page. The software driver can do a write to > swap the page at any time, e.g. to select the page with the > temperature sensor. After it releases the semaphore for that write > changing the page, the hardware could then poll the PHY, and read a > temperature sensor register instead of the link status, and then bad > things happen. > > When using Intel's PHY drivers embedded within the Intel MAC driver, > this is not a problem. They can avoid swapping to different > pages. However, you are on the path to allow the Linux PHY drivers to > be used, and some of them will change the page. And with the embedded > SOC you are working on, i would not be too surprised to see somebody > build a system using a different PHY which the Intel PHY drivers don't > support, but does have a Linux PHY driver. Agreed, but I'd argue it's the same behavior we have today with the existing MII ioctls in this driver. That's not to say this is good, it's just not any less broken than the current state of things. I don't know what the scope of the locking is for the software/firmware semaphore on the firmware side. Maybe someone from Intel has more details on the locking that would help find a clever locking solution? > You also need to watch out for your use case of Marvell > mv88e6390. Polling that makes no sense. Does the hardware actually > recognise it is not a PHY? AFAICT the polling hardware only pokes the device address that the driver stores in 'hw->phy.mdio.prtad', so the PHY polling unit would never see the switch, if it's even polling at all. Some of the MAC configurations will store MDIO_PRTAD_NONE, in which case I wouldn't expect the polling unit to be active. It's up to the board designer to ensure there's no address conflicts on the bus. I'm making some assumptions about the inner workings of the hardware here, so someone from Intel would need to confirm those or set me straight. How about if I keep track of which PHY addresses the driver wants for the x550em_a devices, then clear those bits from the phy_mask instead of + bus->phy_mask = GENMASK(31, 0); Then in the ioctl code, in addition to checking the mii_bus is available, also check that the requested address is one that phy_mask says mii_bus owns, otherwise we fall through to the old code. I think that keeps the dsa and PHY polling as separate as it is today on the ioctl side, and hides the PHY(s) that the ixgbe driver wants to manage exclusively from the mii_bus altogether. Let me know what you think. It'll make the probing code a bit more complicated, but I think it addresses your concerns.