[PATCH] r8169: remember WOL preferences on driver load

[PATCH] r8169: remember WOL preferences on driver load

[Peter Wu]

Do not clear Broadcast/Multicast/Unicast Wake Flag or LanWake in
Config5. This is necessary to preserve WOL state when the driver is
loaded. The r8168 vendor driver used to write to both Config1 and
Config5, but in recent versions, this is commented out. Here we keep
writing PMEnable to Config1 because there may be older chips where
PMEnable is not sticky.

Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 28af01c..7bdf322 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7093,7 +7093,6 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
 	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)
-- 
1.8.3.4

Re: [PATCH] r8169: remember WOL preferences on driver load

[Francois Romieu]

Peter Wu <lekensteyn@gmail.com> :
> Do not clear Broadcast/Multicast/Unicast Wake Flag or LanWake in
> Config5. This is necessary to preserve WOL state when the driver is
> loaded.

Ok, it makes sense.

> The r8168 vendor driver used to write to both Config1 and Config5, but
> in recent versions, this is commented out. Here we keep writing PMEnable
> to Config1 because there may be older chips where PMEnable is not sticky.

<sneak regression paranoia>
Please include the bits you want to keep in the "&" mask.
</sneak regression paranoia>

Hayes, any advice (especially regarding Config5) ?

[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 28af01c..7bdf322 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -7093,7 +7093,6 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	RTL_W8(Cfg9346, Cfg9346_Unlock);
>  	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)
> -- 
> 1.8.3.4

Re: [PATCH] r8169: remember WOL preferences on driver load

[Peter Wu]

On Tuesday 13 August 2013 23:28:43 Francois Romieu wrote:
> > The r8168 vendor driver used to write to both Config1 and Config5, but
> > in recent versions, this is commented out. Here we keep writing PMEnable
> > to Config1 because there may be older chips where PMEnable is not sticky.
> 
> <sneak regression paranoia>
> Please include the bits you want to keep in the "&" mask.
> </sneak regression paranoia>

I previously tried:

    RTL_W8(Config5, RTL_R8(Config5) & (BWF | MWF | UWF | LanWake | PMEStatus));

but then I realized that the Realtek r8168 vendor driver does not touch this 
register on load. The bits not included in the above mask are 2, 3 and 7. On 
most datasheets for recent hardware, these are marked reserved.

For RTL8100C(L) (datasheet 1.3 from 2007), RTL8139D(L) (datasheet 1.3 from 
2005) and RTL8101L (1.6 from 2008)
- bit 2 is Link Down Power Saving mode (1=disable, 0=enable; loaded from 
EEPROM on RTL8139D(L))
- bit 3 is FIFOAddrPtr (internal use for testing FIFO SRAM, default 0)
- bit 7 is reserved

I'll let Hayes judge which approach is more appropriate.

Regards,
Peter

RE: [PATCH] r8169: remember WOL preferences on driver load

[hayeswang]

Peter Wu [mailto:lekensteyn@gmail.com] 
> Sent: Wednesday, August 14, 2013 8:57 PM
> To: Francois Romieu
> Cc: netdev@vger.kernel.org; nic_swsd; Hayeswang
> Subject: Re: [PATCH] r8169: remember WOL preferences on driver load
> 
> On Tuesday 13 August 2013 23:28:43 Francois Romieu wrote:
> > > The r8168 vendor driver used to write to both Config1 and 
> Config5, but
> > > in recent versions, this is commented out. Here we keep 
> writing PMEnable
> > > to Config1 because there may be older chips where 
> PMEnable is not sticky.
> > 
> > <sneak regression paranoia>
> > Please include the bits you want to keep in the "&" mask.
> > </sneak regression paranoia>
> 
> I previously tried:
> 
>     RTL_W8(Config5, RTL_R8(Config5) & (BWF | MWF | UWF | 
> LanWake | PMEStatus));

I think this way is better and safe.

> but then I realized that the Realtek r8168 vendor driver does 
> not touch this 
> register on load. The bits not included in the above mask are 
> 2, 3 and 7. On 
> most datasheets for recent hardware, these are marked reserved.

Some settings would be in rtl8168_hw_start() for different chips.
We sure only correct bit would be enabled.

The simple way is to clear bit 2, 3 and 7 for all chips. Or set desired
bit which you want.

 
Best Regards,
Hayes

[PATCH] r8169: remember WOL preferences on driver load

[Peter Wu]

From: Peter Wu <lekensteyn@gmail.com>

Do not clear Broadcast/Multicast/Unicast Wake Flag or LanWake in
Config5. This is necessary to preserve WOL state when the driver is
loaded. Although the r8168 vendor driver does not write Config5 (it has
been commented out), Hayes Wang from Realtek said that masking bits like
this is more sensible.

Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 2943916..104eaef 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7092,7 +7092,7 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
 	RTL_W8(Config1, RTL_R8(Config1) | PMEnable);
-	RTL_W8(Config5, RTL_R8(Config5) & PMEStatus);
+	RTL_W8(Config5, RTL_R8(Config5) & (BWF | MWF | UWF | LanWake | PMEStatus));
 	if ((RTL_R8(Config3) & (LinkUp | MagicPacket)) != 0)
 		tp->features |= RTL_FEATURE_WOL;
 	if ((RTL_R8(Config5) & (UWF | BWF | MWF)) != 0)
-- 
1.8.3.4

Re: [PATCH] r8169: remember WOL preferences on driver load

[David Miller]

From: Peter Wu <lekensteyn@gmail.com>
Date: Sat, 17 Aug 2013 11:00:02 +0200

> From: Peter Wu <lekensteyn@gmail.com>
> 
> Do not clear Broadcast/Multicast/Unicast Wake Flag or LanWake in
> Config5. This is necessary to preserve WOL state when the driver is
> loaded. Although the r8168 vendor driver does not write Config5 (it has
> been commented out), Hayes Wang from Realtek said that masking bits like
> this is more sensible.
> 
> Signed-off-by: Peter Wu <lekensteyn@gmail.com>

Can I get some reviews and Ack's for this?

Thanks.

Re: [PATCH] r8169: remember WOL preferences on driver load

[Francois Romieu]

David Miller <davem@davemloft.net> :
[...]
> > Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> 
> Can I get some reviews and Ack's for this?

It's more consistent with the {get / set}_wol parts of the driver and
it's done in a way both Hayes and myself agreed on during the previous
messages.

So you can consider it:

Acked-by: Francois Romieu <romieu@fr.zoreil.com>

-- 
Ueimor

Re: [PATCH] r8169: remember WOL preferences on driver load

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 21 Aug 2013 01:12:01 +0200

> David Miller <davem@davemloft.net> :
> [...]
>> > Signed-off-by: Peter Wu <lekensteyn@gmail.com>
>> 
>> Can I get some reviews and Ack's for this?
> 
> It's more consistent with the {get / set}_wol parts of the driver and
> it's done in a way both Hayes and myself agreed on during the previous
> messages.
> 
> So you can consider it:
> 
> Acked-by: Francois Romieu <romieu@fr.zoreil.com>

Thanks, applied.