netdev - Re: [PATCH net] r8169: fix handling ether

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <835d1207-0b97-f241-f332-fe49ac70d920@redhat.com>
Date:   Thu, 1 Oct 2020 09:48:20 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Petr Tesarik <ptesarik@...e.cz>
Subject: Re: [PATCH net] r8169: fix handling ether_clk

Hi,

On 10/1/20 8:44 AM, Heiner Kallweit wrote:
> Petr reported that system freezes on r8169 driver load on a system
> using ether_clk. The original change was done under the assumption
> that the clock isn't needed for basic operations like chip register
> access. But obviously that was wrong.
> Therefore effectively revert the original change, and in addition
> leave the clock active when suspending and WoL is enabled. Chip may
> not be able to process incoming packets otherwise.
> 
> Fixes: 9f0b54cd1672 ("r8169: move switching optional clock on/off to pll power functions")
> Reported-by: Petr Tesarik <ptesarik@...e.cz>
> Tested-by: Petr Tesarik <ptesarik@...e.cz>
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>

Thank you.

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

Regards,

Hans



> ---
>   drivers/net/ethernet/realtek/r8169_main.c | 32 ++++++++++++++---------
>   1 file changed, 19 insertions(+), 13 deletions(-)
> 
> 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)
>