Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <->
 bridge case
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <20200505133812.GA353121@bjorn-Precision-5520>
Date:   Tue, 5 May 2020 22:00:44 +0800
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Krzysztof Wilczynski <kw@linux.com>,
        "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <B6977248-C345-466D-AE8B-600088B73FA8@canonical.com>
References: <20200505133812.GA353121@bjorn-Precision-5520>
To:     Bjorn Helgaas <helgaas@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On May 5, 2020, at 21:38, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Tue, May 05, 2020 at 08:27:59PM +0800, Kai-Heng Feng wrote:
>> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
>> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
>> power. On Windows ASPM L1 is enabled on the device and its upstream
>> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
>> power.
> 
> The above is a benefit, but leading off with it suggests that this
> change is specifically for that config, which it isn't.

Yes, it applies all devices that meet the condition.

> 
>> Currently, ASPM is disabled if downstream has bridge function. It was
>> introduced by commit 7d715a6c1ae5 ("PCI: add PCI Express ASPM support").
>> The commit introduced PCIe ASPM support, but didn't explain why ASPM
>> needs to be in that case.
> 
> s/needs to be in that case/needs to be disabled in that case/ ?

Yes indeed I missed that word...

> 
>> So relax the condition a bit to let bridge which connects to root
>> complex enables ASPM, instead of removing it completely, to avoid
>> regression.
> 
> If this is a regression, that means it used to work correctly.  So are
> you saying 7d715a6c1ae5^ works correctly?  That seems doubtful since
> 7d715a6c1ae5 appeared in v2.6.26 and added ASPM support in the first
> place.

Clearly I didn't express my intention well enough.
What I meant was, we can either remove the "disable ASPM on bridge" case completely, or do what this patch does.

> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pcie/aspm.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 2378ed692534..af5e22d78101 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -629,13 +629,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>> 	/* Setup initial capable state. Will be updated later */
>> 	link->aspm_capable = link->aspm_support;
>> 	/*
>> -	 * If the downstream component has pci bridge function, don't
>> -	 * do ASPM for now.
> 
> I agree, that comment is missing the essential information about *why*
> we don't do ASPM.

Or missing a part to re-enable ASPM in later time.

> 
>> +	 * If upstream bridge isn't connected to root complex and the
>> +	 * downstream component has pci bridge function, don't do ASPM for now.
> 
> But this comment just perpetuates it and makes the special case even
> more special.  I think we should either remove that special case
> completely or figure out what the real issue is.

I do prefer remote it completely, but I was afraid of introducing any regression so I just made the case more "special".

> 
> I know we weren't always very good about computing the acceptable
> latencies (and we still don't handle LTR correctly, though that's an
> L1 Substates issue that wouldn't have applied in the 7d715a6c1ae5
> timeframe).

Seems like Windows doesn't disable ASPM on bridge to bridge case, can we take the risk and remove the special case completely?

Kai-Heng

> 
>> 	 */
>> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
>> -		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
>> -			link->aspm_disable = ASPM_STATE_ALL;
>> -			break;
>> +	if (parent->bus->parent) {
>> +		list_for_each_entry(child, &linkbus->devices, bus_list) {
>> +			if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
>> +				link->aspm_disable = ASPM_STATE_ALL;
>> +				break;
>> +			}
>> 		}
>> 	}
>> 
>> -- 
>> 2.17.1