Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Evan, author of 62f8f5c3bfc2 ("drm/amdgpu: enable ASPM support
for PCIE 7.4.0/7.6.0")]

On Thu, Sep 08, 2022 at 08:53:44AM +0530, Lijo Lazar wrote:
> As per PCIE Base Spec r4.0 Section 6.18
> 'Software must not enable LTR in an Endpoint unless the Root Complex
> and all intermediate Switches indicate support for LTR.'
> 
> This fixes the Unsupported Request error reported through AER during
> ASPM enablement.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216455
> 
> The error was unnoticed before and got visible because of the commit
> referenced below. This doesn't fix anything in the commit below, rather
> fixes the issue in amdgpu exposed by the commit. The reference is only
> to associate this commit with below one so that both go together.
> 
> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
> 
> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-

nbio_v4_3_program_ltr() checks pdev->ltr_path itself instead of doing
it in *_program_aspm().  It'd be nice to use the same approach for all
versions.

I really don't like the fact that amdgpu does all this ASPM fiddling
in the driver in the first place.  ASPM should be configured by the
PCI core, not by each individual driver.  ASPM has all sorts of
requirements that relate to upstream devices, which I think amdgpu
ignores, but the core pays attention to.

Do you know why the driver configures ASPM itself?  If the PCI core is
doing something wrong (and I'm sure it is, ASPM support is kind of a
mess), I'd much prefer to fix up the core where *all* drivers can
benefit from it.

> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> index b465baa26762..aa761ff3a5fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
>  		WREG32_PCIE(smnPCIE_LC_CNTL, data);
>  }
>  
> +#ifdef CONFIG_PCIEASPM
>  static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>  {
>  	uint32_t def, data;
> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>  	if (def != data)
>  		WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>  }
> +#endif
>  
>  static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_PCIEASPM
>  	uint32_t def, data;
>  
>  	def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  	if (def != data)
>  		WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>  
> -	nbio_v2_3_program_ltr(adev);
> +	/* Don't bother about LTR if LTR is not enabled
> +	 * in the path */
> +	if (adev->pdev->ltr_path)
> +		nbio_v2_3_program_ltr(adev);
>  
>  	def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
>  	data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  	data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>  	if (def != data)
>  		WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> +#endif
>  }