| Message ID | 20101216182847.GA14985@ajones-laptop.nbttech.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2010-12-16 at 10:28 -0800, Arthur Jones wrote: > Hi Jeff, Re-sending w/ proper netdev email > address. Sorry about that... > > Arthur > > --- > > On 82571 only, AFAICT, the PHY does not seem to > store the MII control register 0 bit 11 (power > down PHY). So that when we power down the PHY > on dev_close() and then run ethtool on the closed > dev, we end up turning the PHY back on even though > the interface is down. This pretty easy to repro: > > $ ethtool -s eth0 wol d > $ ifconfig eth0 up > $ mii-tool eth0 > eth0: negotiated 100baseTx-FD, link ok > $ ifconfig eth0 down > $ mii-tool eth0 > eth0: no link > $ ethtool -s eth0 autoneg on (doesn't really matter what we do here) > $ mii-tool eth0 > eth0: negotiated 100baseTx-FD, link ok (this should be: eth0: no link) > > We fix this by tracking the power down bit that we > write to the PHY control register and re-inserting > it when we read it. [...] Adding this special case into MDIO access seems like a really nasty hack. Surely the callers that set the control register should take care of this. Ben.
>-----Original Message----- >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On >Behalf Of Allan, Bruce W >Sent: Thursday, December 16, 2010 11:04 AM >To: Ben Hutchings; Arthur Jones >Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org >Subject: RE: [PATCH] e1000e: workaround missing power down mii control bit on >82571 > >>-----Original Message----- >>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On >>Behalf Of Ben Hutchings >>Sent: Thursday, December 16, 2010 10:57 AM >>To: Arthur Jones >>Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org >>Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on >>82571 >> >>Adding this special case into MDIO access seems like a really nasty >>hack. Surely the callers that set the control register should take care >>of this. >> >>Ben. > >Agreed. I am setting up to repro now to see if it is an actual hardware >issue or just a software bug; either way, this patch is not the correct >approach and I'll follow up shortly. > >Bruce. It's the reset in e1000_set_settings() which ignores that we had previously powered off the Phy. I'll go through the rest of the code and fix up this and any other occurrences of similar issues properly. Thanks for reporting this issue Arthur. Bruce.
Hi Bruce, ... On Thu, Dec 16, 2010 at 11:28:18AM -0800, Allan, Bruce W wrote: > >-----Original Message----- > >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > >Behalf Of Allan, Bruce W > >Sent: Thursday, December 16, 2010 11:04 AM > >To: Ben Hutchings; Arthur Jones > >Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org > >Subject: RE: [PATCH] e1000e: workaround missing power down mii control bit on > >82571 > > > >>-----Original Message----- > >>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > >>Behalf Of Ben Hutchings > >>Sent: Thursday, December 16, 2010 10:57 AM > >>To: Arthur Jones > >>Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org > >>Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on > >>82571 > >> > >>Adding this special case into MDIO access seems like a really nasty > >>hack. Surely the callers that set the control register should take care > >>of this. > >> > >>Ben. > > > >Agreed. I am setting up to repro now to see if it is an actual hardware > >issue or just a software bug; either way, this patch is not the correct > >approach and I'll follow up shortly. > > > >Bruce. > > It's the reset in e1000_set_settings() which ignores that we had previously > powered off the Phy. I'll go through the rest of the code and fix up this > and any other occurrences of similar issues properly. Thanks for having a look! We do a read-modify-write there of the PHY control register. We take the rest of the bits as being good, but, for some reason we don't get the power down bit (always reads back zero). Is this a known 82571 issue? On 82574, e.g., we seem to get the power down bit back when we read... Are you sure you want to spread that 82571 specific logic all over the driver? Arthur > > Thanks for reporting this issue Arthur. > > Bruce. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bruce, ... On Thu, Dec 16, 2010 at 12:04:26PM -0800, Arthur Jones wrote: > Hi Bruce, ... > > On Thu, Dec 16, 2010 at 11:28:18AM -0800, Allan, Bruce W wrote: > > >-----Original Message----- > > >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > > >Behalf Of Allan, Bruce W > > >Sent: Thursday, December 16, 2010 11:04 AM > > >To: Ben Hutchings; Arthur Jones > > >Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org > > >Subject: RE: [PATCH] e1000e: workaround missing power down mii control bit on > > >82571 > > > > > >>-----Original Message----- > > >>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > > >>Behalf Of Ben Hutchings > > >>Sent: Thursday, December 16, 2010 10:57 AM > > >>To: Arthur Jones > > >>Cc: Kirsher, Jeffrey T; netdev@vger.kernel.org > > >>Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on > > >>82571 > > >> > > >>Adding this special case into MDIO access seems like a really nasty > > >>hack. Surely the callers that set the control register should take care > > >>of this. > > >> > > >>Ben. > > > > > >Agreed. I am setting up to repro now to see if it is an actual hardware > > >issue or just a software bug; either way, this patch is not the correct > > >approach and I'll follow up shortly. > > > > > >Bruce. > > > > It's the reset in e1000_set_settings() which ignores that we had previously > > powered off the Phy. I'll go through the rest of the code and fix up this > > and any other occurrences of similar issues properly. > > Thanks for having a look! > > We do a read-modify-write there of > the PHY control register. We take > the rest of the bits as being good, > but, for some reason we don't get the > power down bit (always reads back > zero). Is this a known 82571 issue? > On 82574, e.g., we seem to get the > power down bit back when we read... BTW: The 802.3 spec seems to indicate that this bit _should_ be readable even when the PHY is powered down (i.e. this is a PHY bug)... Arthur > > Are you sure you want to spread that > 82571 specific logic all over the driver? > > Arthur > > > > > Thanks for reporting this issue Arthur. > > > > Bruce. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: Arthur Jones [mailto:arthur.jones@riverbed.com] >Sent: Thursday, December 16, 2010 2:14 PM >To: Allan, Bruce W >Cc: Ben Hutchings; Kirsher, Jeffrey T; netdev@vger.kernel.org >Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on >82571 > >> > It's the reset in e1000_set_settings() which ignores that we had previously >> > powered off the Phy. I'll go through the rest of the code and fix up this >> > and any other occurrences of similar issues properly. >> >> Thanks for having a look! >> >> We do a read-modify-write there of >> the PHY control register. We take >> the rest of the bits as being good, >> but, for some reason we don't get the >> power down bit (always reads back >> zero). Is this a known 82571 issue? >> On 82574, e.g., we seem to get the >> power down bit back when we read... > >BTW: The 802.3 spec seems to indicate >that this bit _should_ be readable even >when the PHY is powered down (i.e. this >is a PHY bug)... > >Arthur > >> >> Are you sure you want to spread that >> 82571 specific logic all over the driver? >> >> Arthur No, not a PHY bug. One difference between 82571 and 82574 is during a hardware reset (which is done by the ethtool command in your example repro case), the reset on 82571 is a much more aggressive reset than on 82574 which causes the bit to be cleared automatically. Bruce. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bruce, ... On Thu, Dec 16, 2010 at 05:46:02PM -0800, Allan, Bruce W wrote: > >-----Original Message----- > >From: Arthur Jones [mailto:arthur.jones@riverbed.com] > >Sent: Thursday, December 16, 2010 2:14 PM > >To: Allan, Bruce W > >Cc: Ben Hutchings; Kirsher, Jeffrey T; netdev@vger.kernel.org > >Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on > >82571 > > > >> > It's the reset in e1000_set_settings() which ignores that we had previously > >> > powered off the Phy. I'll go through the rest of the code and fix up this > >> > and any other occurrences of similar issues properly. > >> > >> Thanks for having a look! > >> > >> We do a read-modify-write there of > >> the PHY control register. We take > >> the rest of the bits as being good, > >> but, for some reason we don't get the > >> power down bit (always reads back > >> zero). Is this a known 82571 issue? > >> On 82574, e.g., we seem to get the > >> power down bit back when we read... > > > >BTW: The 802.3 spec seems to indicate > >that this bit _should_ be readable even > >when the PHY is powered down (i.e. this > >is a PHY bug)... > > > >Arthur > > > >> > >> Are you sure you want to spread that > >> 82571 specific logic all over the driver? > >> > >> Arthur > > No, not a PHY bug. One difference between 82571 and 82574 is during a > hardware reset (which is done by the ethtool command in your example > repro case), the reset on 82571 is a much more aggressive reset than on > 82574 which causes the bit to be cleared automatically. That's not what I saw. I saw the bit always read back zero, even right after I set it. Have you tried writing the power down bit and reading it back? Arthur -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On >Behalf Of Arthur Jones >Sent: Friday, December 17, 2010 6:05 AM >To: Allan, Bruce W >Cc: Ben Hutchings; Kirsher, Jeffrey T; netdev@vger.kernel.org >Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on >82571 > >That's not what I saw. I saw the bit always >read back zero, even right after I set it. > >Have you tried writing the power down bit and >reading it back? > >Arthur Yes, and it worked as expected. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bruce, ... On Fri, Dec 17, 2010 at 07:53:08AM -0800, Allan, Bruce W wrote: > >-----Original Message----- > >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > >Behalf Of Arthur Jones > >Sent: Friday, December 17, 2010 6:05 AM > >To: Allan, Bruce W > >Cc: Ben Hutchings; Kirsher, Jeffrey T; netdev@vger.kernel.org > >Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on > >82571 > > > >That's not what I saw. I saw the bit always > >read back zero, even right after I set it. > > > >Have you tried writing the power down bit and > >reading it back? > > > >Arthur > > Yes, and it worked as expected. Interesting! So then you're not able to repro the issue that I describe in the patch then either? This was an 82571? Can you send along the firmware revision and the full lspci for the NIC where it works to read back the power down bit? Very strange... Arthur -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On >Behalf Of Arthur Jones >Sent: Monday, December 20, 2010 7:33 AM >To: Allan, Bruce W >Cc: Ben Hutchings; Kirsher, Jeffrey T; netdev@vger.kernel.org >Subject: Re: [PATCH] e1000e: workaround missing power down mii control bit on >82571 > >Interesting! So then you're not able to >repro the issue that I describe in the >patch then either? > >This was an 82571? Can you send along >the firmware revision and the full lspci >for the NIC where it works to read back >the power down bit? Very strange... No, I _am_ able to reproduce the issue you reported (i.e. Phy powered on after changing settings via ethtool on a down interface) which as I said is caused by the deep reset. However, I do not see any problem with writing the power down bit in the Phy control register. I have a fix for this but want it tested on a number of adapters supported by the driver before sending it out. The 82571 dual-port adapter I am using has firmware version 5.11-2 and its lspci output is: 06:00.1 Ethernet controller: Intel Corporation 82571EB Gigabit Ethernet Controller (rev 06) Subsystem: Intel Corporation PRO/1000 PT Dual Port Server Adapter Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 64 bytes Interrupt: pin B routed to IRQ 55 Region 0: Memory at 48f20000 (32-bit, non-prefetchable) [size=128K] Region 1: Memory at 48f00000 (32-bit, non-prefetchable) [size=128K] Region 2: I/O ports at 4000 [size=32] Expansion ROM at 49a20000 [disabled] [size=128K] Capabilities: [c8] Power Management version 2 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=1 PME- Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+ Address: 00000000fee0300c Data: 41e1 Capabilities: [e0] Express (v1) Endpoint, MSI 00 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset- DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+ RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr+ FatalErr- UnsuppReq+ AuxPwr+ TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x4, ASPM L0s, Latency L0 <4us, L1 <64us ClockPM- Surprise- LLActRep- BwNot- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk- ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- AERCap: First Error Pointer: 14, GenCap- CGenEn- ChkCap- ChkEn- Capabilities: [140 v1] Device Serial Number xx-xx-xx-xx-xx-xx-xx-xx Kernel driver in use: e1000e Kernel modules: e1000e -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/e1000e/hw.h b/drivers/net/e1000e/hw.h index ba302a5..34974f2 100644 --- a/drivers/net/e1000e/hw.h +++ b/drivers/net/e1000e/hw.h @@ -875,6 +875,7 @@ struct e1000_phy_info { u16 cable_length; u16 max_cable_length; u16 min_cable_length; + u16 control_power_down; u8 mdix; diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index c4ca162..ac81596 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -5781,6 +5781,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev, hw->mac.ops.get_bus_info(&adapter->hw); adapter->hw.phy.autoneg_wait_to_complete = 0; + adapter->hw.phy.control_power_down = 0; /* Copper options */ if (adapter->hw.phy.media_type == e1000_media_type_copper) { diff --git a/drivers/net/e1000e/phy.c b/drivers/net/e1000e/phy.c index 3d3dc0c..55b9f78 100644 --- a/drivers/net/e1000e/phy.c +++ b/drivers/net/e1000e/phy.c @@ -224,6 +224,11 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data) e_dbg("MDI Error\n"); return -E1000_ERR_PHY; } + + /* 82571 does not seem to store the power down bit (bit 11) */ + if (offset == PHY_CONTROL && phy->type == e1000_phy_igp_2) + mdic |= phy->control_power_down; + *data = (u16) mdic; return 0; @@ -247,6 +252,10 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data) return -E1000_ERR_PARAM; } + /* 82571 does not seem to store the power down bit (bit 11) */ + if (offset == PHY_CONTROL && phy->type == e1000_phy_igp_2) + phy->control_power_down = data & MII_CR_POWER_DOWN; + /* * Set up Op-code, Phy Address, and register offset in the MDI * Control register. The MAC will take care of interfacing with the
Hi Jeff, Re-sending w/ proper netdev email address. Sorry about that... Arthur --- On 82571 only, AFAICT, the PHY does not seem to store the MII control register 0 bit 11 (power down PHY). So that when we power down the PHY on dev_close() and then run ethtool on the closed dev, we end up turning the PHY back on even though the interface is down. This pretty easy to repro: $ ethtool -s eth0 wol d $ ifconfig eth0 up $ mii-tool eth0 eth0: negotiated 100baseTx-FD, link ok $ ifconfig eth0 down $ mii-tool eth0 eth0: no link $ ethtool -s eth0 autoneg on (doesn't really matter what we do here) $ mii-tool eth0 eth0: negotiated 100baseTx-FD, link ok (this should be: eth0: no link) We fix this by tracking the power down bit that we write to the PHY control register and re-inserting it when we read it. Signed-off-by: Arthur Jones <ajones@riverbed.com> --- drivers/net/e1000e/hw.h | 1 + drivers/net/e1000e/netdev.c | 1 + drivers/net/e1000e/phy.c | 9 +++++++++ 3 files changed, 11 insertions(+), 0 deletions(-)