[PATCH] r8169: more driver shutdown WoL regression.

[PATCH] r8169: more driver shutdown WoL regression.

[Francois Romieu]

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(-)

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:
-- 
1.7.6.4

RE: [PATCH] r8169: more driver shutdown WoL regression.

[hayeswang]

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:

Re: [PATCH] r8169: more driver shutdown WoL regression.

[Francois Romieu]

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.

-- 
Ueimor

RE: [PATCH] r8169: more driver shutdown WoL regression.

[hayeswang]

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

Re: [PATCH] r8169: more driver shutdown WoL regression.

[Stefan Becker]

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

Re: [PATCH] r8169: more driver shutdown WoL regression.

[Francois Romieu]

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.

-- 
Ueimor

Re: [PATCH] r8169: more driver shutdown WoL regression.

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [PATCH] r8169: more driver shutdown WoL regression.

[hayeswang]

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