netdev - Re: [PATCH RFC 6/6] r8169: remove ASPM restrictions now that ASPM is disabled during NAPI poll

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] [day] [month] [year] [list]

Date:   Fri, 10 Mar 2023 22:41:44 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Kai-Heng Feng <kai.heng.feng@...onical.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 6/6] r8169: remove ASPM restrictions now that ASPM is
 disabled during NAPI poll

On 10.03.2023 22:19, Bjorn Helgaas wrote:
> On Sat, Feb 25, 2023 at 10:47:32PM +0100, Heiner Kallweit wrote:
>> Now that  ASPM is disabled during NAPI poll, we can remove all ASPM
>> restrictions. This allows for higher power savings if the network
>> isn't fully loaded.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 27 +----------------------
>>  1 file changed, 1 insertion(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 2897b9bf2..6563e4c6a 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -620,7 +620,6 @@ struct rtl8169_private {
>>  	int cfg9346_usage_count;
>>  
>>  	unsigned supports_gmii:1;
>> -	unsigned aspm_manageable:1;
>>  	dma_addr_t counters_phys_addr;
>>  	struct rtl8169_counters *counters;
>>  	struct rtl8169_tc_offsets tc_offset;
>> @@ -2744,8 +2743,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>>  	if (tp->mac_version < RTL_GIGA_MAC_VER_32)
>>  		return;
>>  
>> -	/* Don't enable ASPM in the chip if OS can't control ASPM */
>> -	if (enable && tp->aspm_manageable) {
>> +	if (enable) {
>>  		rtl_mod_config5(tp, 0, ASPM_en);
>>  		rtl_mod_config2(tp, 0, ClkReqEn);
>>  
>> @@ -5221,16 +5219,6 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
>>  	rtl_rar_set(tp, mac_addr);
>>  }
>>  
>> -/* register is set if system vendor successfully tested ASPM 1.2 */
>> -static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
>> -{
>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_61 &&
>> -	    r8168_mac_ocp_read(tp, 0xc0b2) & 0xf)
>> -		return true;
>> -
>> -	return false;
>> -}
>> -
>>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>>  	struct rtl8169_private *tp;
>> @@ -5302,19 +5290,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  
>>  	tp->mac_version = chipset;
>>  
>> -	/* Disable ASPM L1 as that cause random device stop working
>> -	 * problems as well as full system hangs for some PCIe devices users.
>> -	 * Chips from RTL8168h partially have issues with L1.2, but seem
>> -	 * to work fine with L1 and L1.1.
>> -	 */
>> -	if (rtl_aspm_is_safe(tp))
>> -		rc = 0;
>> -	else if (tp->mac_version >= RTL_GIGA_MAC_VER_46)
>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>> -	else
>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>> -	tp->aspm_manageable = !rc;
> 
> This is beautiful.  But since this series still enables/disables ASPM
> using the chip-specific knobs, I have to ask whether this is all safe
> with respect to simultaneous arbitrary ASPM enable/disable via the
> sysfs knobs.  For example, it should be safe to run these loops
> indefinitely while the NIC is operating and doing NAPI polls:
> 
Good question, but: Realtek doesn't provide any chip data sheets,
therefore I can't say what the chip-specific knobs actually do.

>   DEV=/sys/bus/pci/devices/...
>   while /bin/true; do
>     echo 1 > $DEV/link/l1_2_aspm
>     echo 0 > $DEV/link/l1_2_aspm
>   done
>   while /bin/true; do
>     echo 1 > $DEV/link/l1_1_aspm
>     echo 0 > $DEV/link/l1_1_aspm
>   done
>   while /bin/true; do
>     echo 1 > $DEV/link/l1_aspm
>     echo 0 > $DEV/link/l1_aspm
>   done
> 
> Bjorn