Date:   Tue, 7 Feb 2023 14:49:40 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     Kalle Valo <kvalo@kernel.org>, Chia-Yuan Li <leo.li@realtek.com>,
        Chin-Yen Lee <timlee@realtek.com>,
        Po-Hao Huang <phhuang@realtek.com>,
        linux-wireless@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and
 L1ss for 8852c
Message-ID: <20230207204940.GA2373732@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220819064811.37700-5-pkshih@realtek.com>
Precedence: bulk

On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> From: Chin-Yen Lee <timlee@realtek.com>
> 
> 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> instead, so change them accordingly.

> ...
>  static void rtw89_pci_l1ss_set(struct rtw89_dev *rtwdev, bool enable)
>  {
> +	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
>  	int ret;
>  
> -	if (enable)
> -		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_TIMER_CTRL,
> -						RTW89_PCIE_BIT_L1SUB);
> -	else
> -		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_TIMER_CTRL,
> -						RTW89_PCIE_BIT_L1SUB);
> -	if (ret)
> -		rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> -			  enable ? "set" : "unset", ret);
> +	if (chip_id == RTL8852A || chip_id == RTL8852B) {
> +		if (enable)
> +			ret = rtw89_pci_config_byte_set(rtwdev,
> +							RTW89_PCIE_TIMER_CTRL,
> +							RTW89_PCIE_BIT_L1SUB);
> +		else
> +			ret = rtw89_pci_config_byte_clr(rtwdev,
> +							RTW89_PCIE_TIMER_CTRL,
> +							RTW89_PCIE_BIT_L1SUB);
> +		if (ret)
> +			rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> +				  enable ? "set" : "unset", ret);
> +	} else if (chip_id == RTL8852C) {
> +		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1SS_STS_V1,
> +						RTW89_PCIE_BIT_ASPM_L11 |
> +						RTW89_PCIE_BIT_PCI_L11);
> +		if (ret)
> +			rtw89_warn(rtwdev, "failed to unset ASPM L1.1, ret=%d", ret);
> +		if (enable)
> +			rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> +					  B_AX_L1SUB_DISABLE);
> +		else
> +			rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> +					  B_AX_L1SUB_DISABLE);
> +	}
>  }

We get here via this path:

  rtw89_pci_probe
    rtw89_pci_l1ss_cfg
      pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
      if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
	rtw89_pci_l1ss_set(rtwdev, true);

This looks like it might be a problem because L1SS configuration is
owned by the PCI core, not by the device driver.  The PCI core
provides sysfs user interfaces that can enable and disable L1SS at
run-time without notification to the driver (see [1]).

The user may enable or disable L1SS using those sysfs interfaces, and
this code in the rtw89 driver will not be called.

Bjorn

P.S. rtw89_pci_l1ss_set() is only called from rtw89_pci_l1ss_cfg()
which always supplies "enable == true", so it looks like that
parameter is not needed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.1#n410