Patchwork [1/2] r8169: Enable WOL from Magic Packet by default

login
register
mail settings
Submitter Sameer Nanda
Date Feb. 14, 2012, 6:27 p.m.
Message ID <1329244079-27842-1-git-send-email-snanda@chromium.org>
Download mbox | patch
Permalink /patch/141166/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Sameer Nanda - Feb. 14, 2012, 6:27 p.m.
Set the WOL config registers to only enable WOL from magic packet by
default. Without this change in place, the WOL config register
settings on warm reboot come up in an inconsistent state since these
registers don't get reset on a warm reboot.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
---
 drivers/net/ethernet/realtek/r8169.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
Sameer Nanda - Feb. 14, 2012, 6:38 p.m.
Fixed Rafael's email address.  Sorry about the dupe emails.

On Tue, Feb 14, 2012 at 10:33 AM, Sameer Nanda <snanda@chromium.org> wrote:
> Fixed Francois Romieu email address.
>
> On Tue, Feb 14, 2012 at 10:27 AM, Sameer Nanda <snanda@chromium.org> wrote:
>>
>> Set the WOL config registers to only enable WOL from magic packet by
>> default. Without this change in place, the WOL config register
>> settings on warm reboot come up in an inconsistent state since these
>> registers don't get reset on a warm reboot.
>>
>> Signed-off-by: Sameer Nanda <snanda@chromium.org>
>> ---
>>  drivers/net/ethernet/realtek/r8169.c |   11 ++++++-----
>>  1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 7a0c800..a6921b7 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -4073,12 +4073,13 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>        tp->txd_version = rtl_chip_infos[chipset].txd_version;
>>
>>        RTL_W8(Cfg9346, Cfg9346_Unlock);
>> +
>> +       /* Enable WOL from Magic Packet by default */
>>        RTL_W8(Config1, RTL_R8(Config1) | PMEnable);
>> -       RTL_W8(Config5, RTL_R8(Config5) & PMEStatus);
>> -       if ((RTL_R8(Config3) & (LinkUp | MagicPacket)) != 0)
>> -               tp->features |= RTL_FEATURE_WOL;
>> -       if ((RTL_R8(Config5) & (UWF | BWF | MWF)) != 0)
>> -               tp->features |= RTL_FEATURE_WOL;
>> +       RTL_W8(Config3, MagicPacket);
>> +       RTL_W8(Config5, PMEStatus);
>> +       tp->features |= RTL_FEATURE_WOL;
>> +
>>        tp->features |= rtl_try_msi(tp, cfg);
>>        RTL_W8(Cfg9346, Cfg9346_Lock);
>>
>> --
>> 1.7.7.3
>>
>
>
>
> --
> Sameer
françois romieu - Feb. 15, 2012, 12:13 a.m.
Sameer Nanda <snanda@chromium.org> :
> Set the WOL config registers to only enable WOL from magic packet by
> default. Without this change in place, the WOL config register
> settings on warm reboot come up in an inconsistent state since these
> registers don't get reset on a warm reboot.

I am not completely convinced, especially as the change of behavior
could be noticed.

Can you elaborate why the previous WoL settings should be ignored ?
Sameer Nanda - Feb. 15, 2012, 1 a.m.
On Tue, Feb 14, 2012 at 4:13 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Sameer Nanda <snanda@chromium.org> :
>> Set the WOL config registers to only enable WOL from magic packet by
>> default. Without this change in place, the WOL config register
>> settings on warm reboot come up in an inconsistent state since these
>> registers don't get reset on a warm reboot.
>
> I am not completely convinced, especially as the change of behavior
> could be noticed.

Agreed that this change could be noticed.  Maybe a module parameter
might be a better way to handle this?

>
> Can you elaborate why the previous WoL settings should be ignored ?

With runtime PM, the "previous" settings may not be what the user had
set up since runtime PM mucks around with WOL settings.  Therefore,
the user could see inconsistent WOL settings upon booting up.

My second patch in this series mitigates this to a large extent by
restoring the saved WOL options in rtl_shutdown.  One case it doesn't
handle is non-graceful shutdown since rtl_shutdown may not be invoked.

The issue we ran into is this: disconnect network cable, reboot
system.  This will cause WOL on PHY to be enabled on next boot.  If
you connect the cable and then transition the system to S3 state or
halt the system, the system wakes right back up since the PHY state
changes.  Not good.

>
> --
> Ueimor
David Miller - Feb. 15, 2012, 3:37 a.m.
From: Sameer Nanda <snanda@chromium.org>
Date: Tue, 14 Feb 2012 17:00:04 -0800

> On Tue, Feb 14, 2012 at 4:13 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
>> Sameer Nanda <snanda@chromium.org> :
>>> Set the WOL config registers to only enable WOL from magic packet by
>>> default. Without this change in place, the WOL config register
>>> settings on warm reboot come up in an inconsistent state since these
>>> registers don't get reset on a warm reboot.
>>
>> I am not completely convinced, especially as the change of behavior
>> could be noticed.
> 
> Agreed that this change could be noticed.  Maybe a module parameter
> might be a better way to handle this?

Please no random module parameters, something ethtool based is
what you should shoot for.
--
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
Sameer Nanda - Feb. 16, 2012, 8:47 p.m.
On Tue, Feb 14, 2012 at 7:37 PM, David Miller <davem@davemloft.net> wrote:
> From: Sameer Nanda <snanda@chromium.org>
> Date: Tue, 14 Feb 2012 17:00:04 -0800
>
>> On Tue, Feb 14, 2012 at 4:13 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
>>> Sameer Nanda <snanda@chromium.org> :
>>>> Set the WOL config registers to only enable WOL from magic packet by
>>>> default. Without this change in place, the WOL config register
>>>> settings on warm reboot come up in an inconsistent state since these
>>>> registers don't get reset on a warm reboot.
>>>
>>> I am not completely convinced, especially as the change of behavior
>>> could be noticed.
>>
>> Agreed that this change could be noticed.  Maybe a module parameter
>> might be a better way to handle this?
>
> Please no random module parameters, something ethtool based is
> what you should shoot for.

With the existing code, WOL from PHY, unicast, multicast and broadcast
packets may get accidentally enabled.  The probability of seeing such
packets/events in the wild is quite high and this can cause unintended
wakes from S3 or reboots.

The probability of seeing matching MagicPackets in the wild is
vanishingly small.  Therefore, setting MagicPacket as the only default
WOL mechanism seems like the safer option.

Since ethtool already supports setting of WOL options from userland, I
guess we don't need a new module parameter as the user can set WOL
options according to his own desires.

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7a0c800..a6921b7 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4073,12 +4073,13 @@  rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->txd_version = rtl_chip_infos[chipset].txd_version;
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
+
+	/* Enable WOL from Magic Packet by default */
 	RTL_W8(Config1, RTL_R8(Config1) | PMEnable);
-	RTL_W8(Config5, RTL_R8(Config5) & PMEStatus);
-	if ((RTL_R8(Config3) & (LinkUp | MagicPacket)) != 0)
-		tp->features |= RTL_FEATURE_WOL;
-	if ((RTL_R8(Config5) & (UWF | BWF | MWF)) != 0)
-		tp->features |= RTL_FEATURE_WOL;
+	RTL_W8(Config3, MagicPacket);
+	RTL_W8(Config5, PMEStatus);
+	tp->features |= RTL_FEATURE_WOL;
+
 	tp->features |= rtl_try_msi(tp, cfg);
 	RTL_W8(Cfg9346, Cfg9346_Lock);