Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Jul 2022 13:28:52 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Manyi Li <limanyi@uniontech.com>
Cc:     bhelgaas@google.com, refactormyself@gmail.com, kw@linux.com,
        rajatja@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Kai-Heng Feng <kai.heng.feng@canonical.com>,
        Vidya Sagar <vidyas@nvidia.com>
Subject: Re: [PATCH] PCI/ASPM: Should not report ASPM support to BIOS if FADT
 indicates ASPM is unsupported
Message-ID: <20220713182852.GA841582@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220713112612.6935-1-limanyi@uniontech.com>
Precedence: bulk

[+cc Kai-Heng, Vidya, who also have ASPM patches in flight]

On Wed, Jul 13, 2022 at 07:26:12PM +0800, Manyi Li wrote:
> Startup log of ASUSTeK X456UJ Notebook show:
> [    0.130563] ACPI FADT declares the system doesn't support PCIe ASPM, so disable it
> [   48.092472] pcieport 0000:00:1c.5: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> [   48.092479] pcieport 0000:00:1c.5:   device [8086:9d15] error status/mask=00000001/00002000
> [   48.092481] pcieport 0000:00:1c.5:    [ 0] RxErr
> [   48.092490] pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> [   48.092504] pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> [   48.092506] pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5

Can you elaborate on the connection between the FADT ASPM bit and the
AER logs above?

What problem are we solving here?  A single corrected error being
logged?  An infinite stream of errors?  A device that doesn't work at
all?

We don't need the dmesg timestamps unless they contribute to
understanding the problem.  I don't think they do in this case.

> Signed-off-by: Manyi Li <limanyi@uniontech.com>
> ---
>  drivers/pci/pcie/aspm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc..b173d3c75ae7 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1359,6 +1359,7 @@ void pcie_no_aspm(void)
>  	if (!aspm_force) {
>  		aspm_policy = POLICY_DEFAULT;
>  		aspm_disabled = 1;
> +		aspm_support_enabled = false;

This makes pcie_no_aspm() work the same as booting with
"pcie_aspm=off".  That might be reasonable.

I do wonder why we need both "aspm_disabled" and
"aspm_support_enabled".  And I wonder why we need to set "aspm_policy"
when we're disabling ASPM.  But those aren't really connected to your
change here.

>  	}
>  }
>  
> -- 
> 2.20.1
> 
> 
>