NACK/Cmnt: [SRU][N][PATCH v3 1/2] PCI: vmd: Set devices to D0 before enabling PM

[Jacob Martin]

On Wed, Nov 20, 2024 at 02:17:02PM +0800, En-Wei Wu wrote:
> From: Jian-Hong Pan <jhp at endlessos.org>
> 
> BugLink: https://bugs.launchpad.net/bugs/2085092
> 
> The remapped PCIe Root Port and the child device have PM L1 Substates
> capability, but they are disabled originally.
> 
> Here is a failed example on ASUS B1400CEAE:
> 
>   Capabilities: [900 v1] L1 PM Substates
>         L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
>                   PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
>         L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
>                    T_CommonMode=0us LTR1.2_Threshold=101376ns
>         L1SubCtl2: T_PwrOn=50us
> 
> Enable PCI-PM L1 PM Substates for devices below VMD while they are in D0
> (see PCIe r6.0, sec 5.5.4).
> 
> Link: https://lore.kernel.org/r/20241001083438.10070-4-jhp@endlessos.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> Signed-off-by: Jian-Hong Pan <jhp at endlessos.org>
> Signed-off-by: Krzysztof Wilczyński <kwilczynski at kernel.org>
> Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy at linux.intel.com>
> (backported from commit d66041063192497a4a97d21dbf86b79a03a7f4fb linux-next)
> [En-Wei: We have a Kai-Heng's ASPM SAUCE patch so need to modify the code]
> Signed-off-by: En-Wei Wu <en-wei.wu at canonical.com>
> ---
>  drivers/pci/controller/vmd.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e33d213a34fb..e5a0c6c57524 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -757,7 +757,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> 

pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);

The upstream patch moves the `pci_enable_link_state_locked` call to after
out_state_change, though this backport leaves in the original call to
`pci_enable_link_state_locked` and adds a new one after out_state_change. 

Since the purpose of this change is to prevent L1 link states being enabled
before the device is in D0, wouldn't it be correct to match the original patch
and only keep the new call to `pci_enable_link_state_locked`?
 
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
>  	if (!pos)
> -		return 0;
> +		goto out_state_change;
>  
>  	/*
>  	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> @@ -765,7 +765,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	 */
>  	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
>  	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> -		return 0;
> +		goto out_state_change;
>  
>  	/*
>  	 * Set the default values to the maximum required by the platform to
> @@ -777,6 +777,13 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
>  	pci_info(pdev, "VMD: Default LTR value set by driver\n");
>  
> +out_state_change:
> +	/*
> +	 * Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> +	 * PCIe r6.0, sec 5.5.4.
> +	 */
> +	pci_set_power_state_locked(pdev, PCI_D0);
> +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team