netdev - Re: [PATCH net-next] r8169: add rtl_disable_exit

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: <ee4f20bd-f32d-ae2d-3767-b927cae9ef7f@gmail.com>
Date:   Fri, 28 Jan 2022 11:40:36 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Hau <hau@...ltek.com>, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        nic_swsd <nic_swsd@...ltek.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "grundler@...omium.org" <grundler@...omium.org>
Subject: Re: [PATCH net-next] r8169: add rtl_disable_exit_l1()

On 28.01.2022 09:09, Hau wrote:
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit [mailto:hkallweit1@...il.com]
>> Sent: Friday, January 28, 2022 6:15 AM
>> To: Jakub Kicinski <kuba@...nel.org>; David Miller <davem@...emloft.net>;
>> nic_swsd <nic_swsd@...ltek.com>
>> Cc: netdev@...r.kernel.org; Hau <hau@...ltek.com>
>> Subject: [PATCH net-next] r8169: add rtl_disable_exit_l1()
>>
>> Add rtl_disable_exit_l1() for ensuring that the chip doesn't inadvertently exit
>> ASPM L1 when being in a low-power mode.
>> The new function is called from rtl_prepare_power_down() which has to be
>> moved in the code to avoid a forward declaration.
>>
>> According to Realtek OCP register 0xc0ac shadows ERI register 0xd4 on
>> RTL8168 versions from RTL8168g. This allows to simplify the code a little.
>>
>> Suggested-by: Chun-Hao Lin <hau@...ltek.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 65 ++++++++++++++---------
>>  1 file changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>> b/drivers/net/ethernet/realtek/r8169_main.c
>> index 3c3d1506b..104ebc0fb 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -2231,28 +2231,6 @@ static int rtl_set_mac_address(struct net_device
>> *dev, void *p)
>>  	return 0;
>>  }
>>
>> -static void rtl_wol_enable_rx(struct rtl8169_private *tp) -{
>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_25)
>> -		RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) |
>> -			AcceptBroadcast | AcceptMulticast | AcceptMyPhys);
>> -}
>> -
>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>> -	if (tp->dash_type != RTL_DASH_NONE)
>> -		return;
>> -
>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>> -
>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>> -		phy_speed_down(tp->phydev, false);
>> -		rtl_wol_enable_rx(tp);
>> -	}
>> -}
>> -
>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>  	switch (tp->mac_version) {
>> @@ -2667,10 +2645,7 @@ static void rtl_enable_exit_l1(struct
>> rtl8169_private *tp)
>>  	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>  		rtl_eri_set_bits(tp, 0xd4, 0x0c00);
>>  		break;
>> -	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>> -		rtl_eri_set_bits(tp, 0xd4, 0x1f80);
>> -		break;
>> -	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63:
>>  		r8168_mac_ocp_modify(tp, 0xc0ac, 0, 0x1f80);
>>  		break;
>>  	default:
>> @@ -2678,6 +2653,20 @@ static void rtl_enable_exit_l1(struct
>> rtl8169_private *tp)
>>  	}
>>  }
>>
>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>> +	switch (tp->mac_version) {
>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38:
>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>> +		break;
>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63:
>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool
>> enable)  {
>>  	/* Don't enable ASPM in the chip if OS can't control ASPM */ @@ -
>> 4689,6 +4678,30 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
>>  	return 0;
>>  }
>>
>> +static void rtl_wol_enable_rx(struct rtl8169_private *tp) {
>> +	if (tp->mac_version >= RTL_GIGA_MAC_VER_25)
>> +		RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) |
>> +			AcceptBroadcast | AcceptMulticast | AcceptMyPhys); }
>> +
>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
>> +	if (tp->dash_type != RTL_DASH_NONE)
>> +		return;
>> +
>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>> +		rtl_ephy_write(tp, 0x19, 0xff64);
>> +
>> +	if (device_may_wakeup(tp_to_dev(tp))) {
>> +		phy_speed_down(tp->phydev, false);
>> +		rtl_wol_enable_rx(tp);
>> +	} else {
>> +		rtl_disable_exit_l1(tp);
>> +	}
>> +}
>> +
> Hi Heiner,
> 
Hi Hau,

> rtl_disable_exit_l1(tp) can be called before device enter D3 state. I think you don’t need to check wol status.
> You may update the code link following. 

my thought was that if DASH or WoL are enabled then we might miss
something on the bus if not waking from L1 in time.
You think this shouldn't be a problem?

By the way, because I'm no DASH expert:
Should we go to D3 at all if DASH is enabled? Will it still work?

> 
> static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> 	rtl_disable_exit_l1(tp);
> 
> 	if (tp->dash_type != RTL_DASH_NONE)
> 		return;
> 
> 	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> 	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> 		rtl_ephy_write(tp, 0x19, 0xff64);
> 
> 	if (device_may_wakeup(tp_to_dev(tp))) {
> 		phy_speed_down(tp->phydev, false);
> 		rtl_wol_enable_rx(tp);
> 	} 
> }
> 
> Thanks.
>>  static void rtl8169_down(struct rtl8169_private *tp)  {
>>  	/* Clear all task flags */
>> --
>> 2.35.0
>>
>> ------Please consider the environment before printing this e-mail.