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

[Bjorn Helgaas <helgaas@kernel.org>]

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@gmail.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:

  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