| Message ID | 20111108223502.GA20437@electric-eye.fr.zoreil.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Wednesday, November 09, 2011 6:35 AM > To: netdev@vger.kernel.org > Cc: Stefan Becker; David Miller; Hayeswang > Subject: [PATCH] r8169: more driver shutdown WoL regression. > > Almost the same narrative as 649b3b8c4e8681de443b4dc9e387c3036369e02e > but with more experimental data. > > Stefan Becker has reported that the same kind of fix as the one > introduced in 649b3b8c4e8681de443b4dc9e387c3036369e02e ("r8169: fix > driver shutdown WoL regression") before 3.1 was released is required > for his 8168c (RTL_GIGA_MAC_VER_22). > > I have tested a few chipsets as well: > - without patch, shutdown + WoL works fine for : > o RTL_GIGA_MAC_VER_30 (8105e and 8105evc) > o RTL_GIGA_MAC_VER_33 (8168ed) > o RTL_GIGA_MAC_VER_34 (8168evl) > o RTL_GIGA_MAC_VER_35 (8168f) > o RTL_GIGA_MAC_VER_06 (plain old PCI 8169sc) > - without patch, shutdown + WoL is broken with : > o RTL_GIGA_MAC_VER_26 (8168d-vb-gr) > o RTL_GIGA_MAC_VER_25 (8168d-gr) > o RTL_GIGA_MAC_VER_12 (8168b) > o RTL_GIGA_MAC_VER_09 (both 8102e-vb-gr and 8103e-gr) > I am confused with your results. According to the information from hw and my tests, the chips which need enable RxConfig for WOL are 8105e series, 8168e series, 8168evl, and 8168f series. The previous chips, include 8168d, work fine for WOL without enabling RxConfig. PS. I test 8111d (RTL_GIGA_MAC_VER_25) and 8111e (RTL_GIGA_MAC_VER_33) with kernel 3.1.0. Besides, I find rtl_shutdown would call rtl8169_net_suspend, rtl8169_net_suspend would call rtl_pll_power_down, and rtl_pll_power_down would call r810x_pll_power_down or r8168_pll_power_down. Finally, I find rtl_wol_suspend_quirk would be called. Is it necessary to call rtl_wol_suspend_quirk again in rtl_shutdown? > I have widened rtl_wol_suspend_quirk a bit beyond those data > to include > a broader subset of chipsets from the same families, thus > including the > 8168cp and 8168dp. > > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> > Tested-by: Stefan Becker <chemobejk@gmail.com> > Cc: Hayes <hayeswang@realtek.com> > --- > > Hayes, any insight ? > > drivers/net/ethernet/realtek/r8169.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 92b45f0..829674d 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -3496,6 +3496,18 @@ static void > rtl_wol_suspend_quirk(struct rtl8169_private *tp) > void __iomem *ioaddr = tp->mmio_addr; > > switch (tp->mac_version) { > + case RTL_GIGA_MAC_VER_07: > + case RTL_GIGA_MAC_VER_08: > + case RTL_GIGA_MAC_VER_09: > + case RTL_GIGA_MAC_VER_11: > + case RTL_GIGA_MAC_VER_12: > + case RTL_GIGA_MAC_VER_17: > + case RTL_GIGA_MAC_VER_19: > + case RTL_GIGA_MAC_VER_20: > + case RTL_GIGA_MAC_VER_21: > + case RTL_GIGA_MAC_VER_22: > + case RTL_GIGA_MAC_VER_25: > + case RTL_GIGA_MAC_VER_26: > case RTL_GIGA_MAC_VER_29: > case RTL_GIGA_MAC_VER_30: > case RTL_GIGA_MAC_VER_32: -- 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
hayeswang <hayeswang@realtek.com> : [...] > I am confused with your results. According to the information from hw and my > tests, the chips which need enable RxConfig for WOL are 8105e series, 8168e > series, 8168evl, and 8168f series. The previous chips, include 8168d, work fine > for WOL without enabling RxConfig. > PS. I test 8111d (RTL_GIGA_MAC_VER_25) and 8111e (RTL_GIGA_MAC_VER_33) with > kernel 3.1.0. I tested in the following order RTL_GIGA_MAC_VER_26 (ko) RTL_GIGA_MAC_VER_25 (ko) RTL_GIGA_MAC_VER_12 (ko) RTL_GIGA_MAC_VER_34 (ok) RTL_GIGA_MAC_VER_33 (ok) RTL_GIGA_MAC_VER_35 (ok) RTL_GIGA_MAC_VER_09 (ko) RTL_GIGA_MAC_VER_09 (ko) RTL_GIGA_MAC_VER_30 (ok) RTL_GIGA_MAC_VER_30 (ok) RTL_GIGA_MAC_VER_06 (ok) Each test: - proceeded from a fresh boot - dmesg | grep XID; ip link show -> check MAC address (devices were set through udev long beforehand) - ip link set dev xyz up - ethtool xyz -> check link status up - ethtool peer -> check link status up, 1000 Mbps or 100 Mbps - ethtool -s xyz wol g; ethtool xyz - telinit 0 - ethtool peer -> check link status up, 10 Mbps - ether-wake -i peer mac_addr (triple check the mac address here) (stop, power off, unplug, plug, power on, repeat) Once the whole set of chipset was tested, I patched, built, and test again. It was post 3.1. I'll sample again a few offenders with 3.2-rc1 tomorrow but it is not only me : Stefan noticed the same behavior with his 8168c (I have neither 8168c nor 8168cp). > Besides, I find rtl_shutdown would call rtl8169_net_suspend, rtl8169_net_suspend > would call rtl_pll_power_down, and rtl_pll_power_down would call > r810x_pll_power_down or r8168_pll_power_down. Finally, I find > rtl_wol_suspend_quirk would be called. Is it necessary to call > rtl_wol_suspend_quirk again in rtl_shutdown? Yes, due to rtl8169_hw_reset in rtl_shutdown. static void rtl_shutdown(struct pci_dev *pdev) { struct net_device *dev = pci_get_drvdata(pdev); struct rtl8169_private *tp = netdev_priv(dev); rtl8169_net_suspend(dev); [...] rtl8169_hw_reset(tp); <- resets RxConfig spin_unlock_irq(&tp->lock); if (system_state == SYSTEM_POWER_OFF) { if (__rtl8169_get_wol(tp) & WAKE_ANY) { rtl_wol_suspend_quirk(tp); rtl_wol_shutdown_quirk(tp); } I'll add a netif_device_detach + netif_stop_queue helper for both rtl8169_net_suspend and rtl_shutdown to avoid the first call to rtl_wol_suspend_quirk in rtl_shutdown now that -next is open.
Francois Romieu [mailto:romieu@fr.zoreil.com] [...] > > I tested in the following order > RTL_GIGA_MAC_VER_26 (ko) > RTL_GIGA_MAC_VER_25 (ko) > RTL_GIGA_MAC_VER_12 (ko) > RTL_GIGA_MAC_VER_34 (ok) > RTL_GIGA_MAC_VER_33 (ok) > RTL_GIGA_MAC_VER_35 (ok) > RTL_GIGA_MAC_VER_09 (ko) > RTL_GIGA_MAC_VER_09 (ko) > RTL_GIGA_MAC_VER_30 (ok) > RTL_GIGA_MAC_VER_30 (ok) > RTL_GIGA_MAC_VER_06 (ok) > > Each test: > - proceeded from a fresh boot > - dmesg | grep XID; ip link show > -> check MAC address (devices were set through udev long beforehand) > - ip link set dev xyz up > - ethtool xyz > -> check link status up > - ethtool peer > -> check link status up, 1000 Mbps or 100 Mbps > - ethtool -s xyz wol g; ethtool xyz > - telinit 0 > - ethtool peer > -> check link status up, 10 Mbps > - ether-wake -i peer mac_addr (triple check the mac address here) > > (stop, power off, unplug, plug, power on, repeat) > I find that the magic packet which I send is the broadcast packet, and the one which you send is the unicast packet. That is, you could wake up the system by using broadcast magic packet for the previous chips without the patch. However, if you prefer to unicast magic packet, this patch is necessary. Besides, no matter broadcast or unicast magic packet, the patch is necessary for 8105, 8168e, and later chips. Further, it may be dangerous to enable both rx_enable (ChipCmd bit 3) and RxConfig for 8168b for WOL, because the hw would try to write the rx buffer. Best Regards, Hayes -- 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
On Thu, Nov 10, 2011 at 6:11 AM, hayeswang <hayeswang@realtek.com> wrote: > > I find that the magic packet which I send is the broadcast packet, and the one > which you send is the unicast packet. That is, you could wake up the system by > using broadcast magic packet for the previous chips without the patch. Confirmed. If I use the "-b" option in ether-wake WoL works for my system with plain 3.1 kernel, i.e. without the patch. [ 8.357467] r8169 0000:02:00.0: eth0: RTL8168c/8111c at 0xf37be000, 00:1f:d0:5a:22:77, XID 1c4000c0 IRQ 43 > However, > if you prefer to unicast magic packet, this patch is necessary. As "-b" is not the default for ether-wake it will come as a surprise for the user that WoL is no longer working. Maybe we should reword the patch subject to: r8169: driver shutdown unicast WoL regression Regards, Stefan -- 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
hayeswang <hayeswang@realtek.com> : [...] > I find that the magic packet which I send is the broadcast packet, and the one > which you send is the unicast packet. That is, you could wake up the system by > using broadcast magic packet for the previous chips without the patch. However, > if you prefer to unicast magic packet, this patch is necessary. Besides, no > matter broadcast or unicast magic packet, the patch is necessary for 8105, > 8168e, and later chips. Ok, it makes some sense now. I am inclined to enable a broad understanding of ethtool WAKE_MAGIC feature as AMD's magic packet white paper does not limit it to broadcast packets and explicitely quotes unicast and multicast. Ben (and others), any opinion ? Hayes, should I consider similar cross-behaviors between RxConfig and WoL ConfigX bits with different configurations ? I.e., assuming Config5.UWF is active and Config3.MagicPacket is not, can RxConfig.AcceptMyPhys make a difference to the WoL function ? > Further, it may be dangerous to enable both rx_enable (ChipCmd bit 3) and > RxConfig for 8168b for WOL, because the hw would try to write the rx buffer. Ok.
On Thu, 2011-11-10 at 11:41 +0100, Francois Romieu wrote: > hayeswang <hayeswang@realtek.com> : > [...] > > I find that the magic packet which I send is the broadcast packet, and the one > > which you send is the unicast packet. That is, you could wake up the system by > > using broadcast magic packet for the previous chips without the patch. However, > > if you prefer to unicast magic packet, this patch is necessary. Besides, no > > matter broadcast or unicast magic packet, the patch is necessary for 8105, > > 8168e, and later chips. > > Ok, it makes some sense now. > > I am inclined to enable a broad understanding of ethtool WAKE_MAGIC > feature as AMD's magic packet white paper does not limit it to > broadcast packets and explicitely quotes unicast and multicast. > Ben (and others), any opinion ? [...] Sorry, I've never looked into WoL in detail so I'm not sure quite what the intended semantics of WAKE_MAGIC are. Ben.
Francois Romieu [mailto:romieu@fr.zoreil.com] [...] > > Hayes, should I consider similar cross-behaviors between > RxConfig and WoL > ConfigX bits with different configurations ? > > I.e., assuming Config5.UWF is active and Config3.MagicPacket > is not, can > RxConfig.AcceptMyPhys make a difference to the WoL function ? > Yes. The RxConfig influences if you could receive unicast, multicast, or broadcast packets. If you don't enable the relative bits of RxConfig, the WOL feature won't work no matter which WOL mode you set. > > Further, it may be dangerous to enable both rx_enable > (ChipCmd bit 3) and > > RxConfig for 8168b for WOL, because the hw would try to > write the rx buffer. > > Ok. > Best Regards, Hayes -- 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/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 92b45f0..829674d 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -3496,6 +3496,18 @@ static void rtl_wol_suspend_quirk(struct rtl8169_private *tp) void __iomem *ioaddr = tp->mmio_addr; switch (tp->mac_version) { + case RTL_GIGA_MAC_VER_07: + case RTL_GIGA_MAC_VER_08: + case RTL_GIGA_MAC_VER_09: + case RTL_GIGA_MAC_VER_11: + case RTL_GIGA_MAC_VER_12: + case RTL_GIGA_MAC_VER_17: + case RTL_GIGA_MAC_VER_19: + case RTL_GIGA_MAC_VER_20: + case RTL_GIGA_MAC_VER_21: + case RTL_GIGA_MAC_VER_22: + case RTL_GIGA_MAC_VER_25: + case RTL_GIGA_MAC_VER_26: case RTL_GIGA_MAC_VER_29: case RTL_GIGA_MAC_VER_30: case RTL_GIGA_MAC_VER_32:
Almost the same narrative as 649b3b8c4e8681de443b4dc9e387c3036369e02e but with more experimental data. Stefan Becker has reported that the same kind of fix as the one introduced in 649b3b8c4e8681de443b4dc9e387c3036369e02e ("r8169: fix driver shutdown WoL regression") before 3.1 was released is required for his 8168c (RTL_GIGA_MAC_VER_22). I have tested a few chipsets as well: - without patch, shutdown + WoL works fine for : o RTL_GIGA_MAC_VER_30 (8105e and 8105evc) o RTL_GIGA_MAC_VER_33 (8168ed) o RTL_GIGA_MAC_VER_34 (8168evl) o RTL_GIGA_MAC_VER_35 (8168f) o RTL_GIGA_MAC_VER_06 (plain old PCI 8169sc) - without patch, shutdown + WoL is broken with : o RTL_GIGA_MAC_VER_26 (8168d-vb-gr) o RTL_GIGA_MAC_VER_25 (8168d-gr) o RTL_GIGA_MAC_VER_12 (8168b) o RTL_GIGA_MAC_VER_09 (both 8102e-vb-gr and 8103e-gr) I have widened rtl_wol_suspend_quirk a bit beyond those data to include a broader subset of chipsets from the same families, thus including the 8168cp and 8168dp. Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> Tested-by: Stefan Becker <chemobejk@gmail.com> Cc: Hayes <hayeswang@realtek.com> --- Hayes, any insight ? drivers/net/ethernet/realtek/r8169.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)