HI Heiner,

On Wed, 30 Sep 2020 17:47:15 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 30.09.2020 11:04, Hans de Goede wrote:
> > Hi,
> > 
> > On 9/29/20 10:35 PM, Heiner Kallweit wrote:  
> >> On 29.09.2020 22:08, Hans de Goede wrote:  
> > 
> > <snip>
> >   
> >>> Also some remarks about this while I'm being a bit grumpy about
> >>> all this anyways (sorry):
> >>>
> >>> 1. 9f0b54cd167219 ("r8169: move switching optional clock on/off
> >>> to pll power functions") commit's message does not seem to really
> >>> explain why this change was made...
> >>>
> >>> 2. If a git blame would have been done to find the commit adding
> >>> the clk support: commit c2f6f3ee7f22 ("r8169: Get and enable optional ether_clk clock")
> >>> then you could have known that the clk in question is an external
> >>> clock for the entire chip, the commit message pretty clearly states
> >>> this (although "the entire" part is implied only) :
> >>>
> >>> "On some boards a platform clock is used as clock for the r8169 chip,
> >>> this commit adds support for getting and enabling this clock (assuming
> >>> it has an "ether_clk" alias set on it).
> >>>  
> >> Even if the missing clock would stop the network chip completely,
> >> this shouldn't freeze the system as described by Petr.
> >> In some old RTL8169S spec an external 25MHz clock is mentioned,
> >> what matches the MII bus frequency. Therefore I'm not 100% convinced
> >> that the clock is needed for basic chip operation, but due to
> >> Realtek not releasing datasheets I can't verify this.  
> > 
> > Well if that 25 MHz is the only clock the chip has, then it basically
> > has to be on all the time since all clocked digital ASICs cannot work
> > without a clock.  Now pci-e is a packet-switched point-to-point bus,
> > so the ethernet chip not working should not freeze the entire system,
> > but I'm not really surprised that even though it should not do that,
> > that it still does.
> >   
> >> But yes, if reverting this change avoids the issue on Petr's system,
> >> then we should do it. A simple mechanical revert wouldn't work because
> >> source file structure has changed since then, so I'll prepare a patch
> >> that effectively reverts the change.  
> > 
> > Great, thank you.
> > 
> > Regards,
> > 
> > Hans
> >   
> 
> Petr,
> in the following I send two patches. First one is supposed to fix the freeze.
> It also fixes another issue that existed before my ether_clk change:
> ether_clk was disabled on suspend even if WoL is enabled. And the network
> chip most likely needs the clock to check for WoL packets.

Should I also check whether WoL works? Plus, it would be probably good
if I could check whether it indeed didn't work before the change. ;-)

> Please let me know whether it fixes the freeze, then I'll add your Tested-by.

Will do.

> Second patch is a re-send of the one I sent before, it should fix
> the rx issues after resume from suspend for you.

All right, going to rebuild the kernel and see how it goes.

Stay tuned,
Petr T


> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6c7c004c2..72351c5b0 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2236,14 +2236,10 @@ static void rtl_pll_power_down(struct rtl8169_private *tp)
>  	default:
>  		break;
>  	}
> -
> -	clk_disable_unprepare(tp->clk);
>  }
>  
>  static void rtl_pll_power_up(struct rtl8169_private *tp)
>  {
> -	clk_prepare_enable(tp->clk);
> -
>  	switch (tp->mac_version) {
>  	case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
>  	case RTL_GIGA_MAC_VER_37:
> @@ -4820,29 +4816,39 @@ static void rtl8169_net_suspend(struct rtl8169_private *tp)
>  
>  #ifdef CONFIG_PM
>  
> +static int rtl8169_net_resume(struct rtl8169_private *tp)
> +{
> +	rtl_rar_set(tp, tp->dev->dev_addr);
> +
> +	if (tp->TxDescArray)
> +		rtl8169_up(tp);
> +
> +	netif_device_attach(tp->dev);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused rtl8169_suspend(struct device *device)
>  {
>  	struct rtl8169_private *tp = dev_get_drvdata(device);
>  
>  	rtnl_lock();
>  	rtl8169_net_suspend(tp);
> +	if (!device_may_wakeup(tp_to_dev(tp)))
> +		clk_disable_unprepare(tp->clk);
>  	rtnl_unlock();
>  
>  	return 0;
>  }
>  
> -static int rtl8169_resume(struct device *device)
> +static int __maybe_unused rtl8169_resume(struct device *device)
>  {
>  	struct rtl8169_private *tp = dev_get_drvdata(device);
>  
> -	rtl_rar_set(tp, tp->dev->dev_addr);
> -
> -	if (tp->TxDescArray)
> -		rtl8169_up(tp);
> +	if (!device_may_wakeup(tp_to_dev(tp)))
> +		clk_prepare_enable(tp->clk);
>  
> -	netif_device_attach(tp->dev);
> -
> -	return 0;
> +	return rtl8169_net_resume(tp);
>  }
>  
>  static int rtl8169_runtime_suspend(struct device *device)
> @@ -4868,7 +4874,7 @@ static int rtl8169_runtime_resume(struct device *device)
>  
>  	__rtl8169_set_wol(tp, tp->saved_wolopts);
>  
> -	return rtl8169_resume(device);
> +	return rtl8169_net_resume(tp);
>  }
>  
>  static int rtl8169_runtime_idle(struct device *device)