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;
Message-ID: <60b2f5fb-8294-d104-16d8-0acfc70426c1@amd.com>
Date:   Fri, 14 Jul 2023 11:37:31 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [Intel-wired-lan] [PATCH] PCI/ASPM: Enable ASPM on external PCIe
 devices
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Bjorn Helgaas <helgaas@kernel.org>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        linux-pci@vger.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        linux-kernel@vger.kernel.org, Vidya Sagar <vidyas@nvidia.com>,
        Michael Bottini <michael.a.bottini@linux.intel.com>,
        intel-wired-lan@osuosl.org, bhelgaas@google.com,
        Mika Westerberg <mika.westerberg@linux.intel.com>
References: <20230705200617.GA72825@bhelgaas>
 <9d1095ab-23e5-3df3-58d6-b2974f87ee72@amd.com>
 <CAAd53p7L27dkzwb_Q9vhENhBye-JTcx2AuCG_YXAgb0F6MG-9w@mail.gmail.com>
Content-Language: en-US
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <CAAd53p7L27dkzwb_Q9vhENhBye-JTcx2AuCG_YXAgb0F6MG-9w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VnZwVjQ3V2JqaWxDUEYwMjhPd3o3YzRmNkNUMTFKeVBBdkl0OU1scXFFa29q?=
 =?utf-8?B?bXpJUXg2YjlUeEJGZUdWVDhoSy9CZlNWblhPSjVPa1ozSmJkZmFRbS9kVGd0?=
 =?utf-8?B?TThpQUtHMWU3MEtQM3BYbXdoZlFIcXQ5b1oyd2duZUFGenBacWc1Q3d0cnBT?=
 =?utf-8?B?enNJMlRkdXNwZ0NZdmhXdHlSTWFDQkY0cmJCSzA1QlRkNktwYldBeHZzVk93?=
 =?utf-8?B?dURweTUwSHpmTlZQNVB6OVJvT3NxbnhZUjRiV3l5SlhENllHQ1pRK3hJb3Ny?=
 =?utf-8?B?NzNGdGs5YWVSVFpBOHM4enRsNkdCMWxTK1MveUIra3hVYm8vZVpuVVVlbk1t?=
 =?utf-8?B?dUNlQzJWN3NpMjY3VXI2d3NMT05hTGFaREp6d2Z1N3Z5d1BDcUhOR0hLV0lB?=
 =?utf-8?B?VlZCMDFmamhCaXpYMzNEVWhlMktXQm5reVBqUTZYTVI5aEhscTRNNmRqVG5l?=
 =?utf-8?B?RmUxdi9NaDVQd0plRXlxWjg4eUVBM3U5UXhoRFhlYzduWGVJS2FKaVdoVklt?=
 =?utf-8?B?UjNTaDkvUDc3S1VFc2VaWG5Kbi9vN2NsYThoM1BYN3FYMC9Pdjd2NnN3VXE1?=
 =?utf-8?B?WXhpSEJXNWJyTll1NW85bVV5ZFUvODlTVjNYYnorMzlzaDF0L1hxYmdnSDhI?=
 =?utf-8?B?SUllc2FNV0VDSkR4akloSlFWQm5RSkdKQ3Faa0V2R2hLUjEwRFBXYW96cWdS?=
 =?utf-8?B?SzNUdmYzUEJ1ekNPajk2NWE1cW03cGE2dXd1L0NkaXh0aW5rMGZPVnRrSFJR?=
 =?utf-8?B?MGVrRnMyR0FSbUVPa1FCUi9jT1NqcGJsTjdXUGZ6cEU0aTJGTmNNTFl5UTFM?=
 =?utf-8?B?K1JFY0NiY3hjVFZpNjdicXlmZzlFQmFVdzV1c0daWDJ3M3JZL2hLN2VJUmts?=
 =?utf-8?B?Sm4vWFVLdDlycktFTEZKL1UzWXVmU2RVbXpPM0k1RnRkUFJkQzZ4eVZVUkww?=
 =?utf-8?B?RW5RdWVkSmpSc2k1VWlORkhkS1ZoWUxLU0RBQzhJNTdGTXovbUNVcEduS2Jw?=
 =?utf-8?B?b1I5Nmg3QXpEWWJhLy8xQVp4dFNyOEJTdUJjSmFic3BwenFiOUUyTjhER1hO?=
 =?utf-8?B?SWJjcUlzdnNhZ3d4VHFmcW0ydTZ2cUQxWVQwS2JGNUNXTER2Rmc0b3VncnhH?=
 =?utf-8?B?cUJBcXFGR0pDS3RmbWttY3VlTXZ0SVA4T0Q1M1J5eXFUakZTQ0JjcWd1VitT?=
 =?utf-8?B?eWU4TkZTK2RFTUdub1pGL1JWK2ZhblNKeENVeHdUOG9nZy9oeUFwZWQwSTY3?=
 =?utf-8?B?YXMweDhhQnFTdHpvY0p2OXdtY01SRXlyYjBkYVlQUUp6ZmsxRVdUMmI4bzE5?=
 =?utf-8?B?Ym1FZW5zRXJFVWdiUWd6bmU0U0h2UHg5RTV4Z1hwd1IxTEJJTytOUyt2NWhl?=
 =?utf-8?B?QTJYUzRLdkNxZmhtbzJ2UnFhNTViMXpzNDVBN1NCMUhJM1hudEgwdTRSd0py?=
 =?utf-8?B?ZWxUeU1oSXhhWFZ4Y2x1ZVpCOEt3NURrTTIwekFWUE82RUIrU1JRWW9IVGZp?=
 =?utf-8?B?dTlFN1JuUHByRmNtbE5zV0ZUQW1hM3hIbU1zdktwR3lWNWkyYTJ1YzVpU05H?=
 =?utf-8?B?NlpPQzNWS3dkMXpWek5DZ2l3akZqQnYvemMrQlg0TVZMTURTZDJNUkNQQitF?=
 =?utf-8?B?ZzZIeGRwNDdxajYvdHcyZ2lheGJ3a09yOStCZlVtZW4xbWlxVytuSUM4dXBm?=
 =?utf-8?B?enVLTmN1VGhZcWVMc0FSSnZkU1gzME00WHNTTjZTbGJ2aVpHTDd4TVMrNVht?=
 =?utf-8?B?eklPQzhVUG9BMU9STzNOdFdtTU4yK2dCU2tBVjF6Q1h5U2Jta0ZqOGtudE55?=
 =?utf-8?B?YWRjMUdKL2syZGNONktjR3ZoMFl6cXhtTzRVbk5rSVRzV2ppUjM1U0wyY0lK?=
 =?utf-8?B?Uk05dVp3MEdtLzBUV2lteWd4OGpKTk54UEZHSXF1WitGUDZwTXJvYndISDNI?=
 =?utf-8?B?WHhGZ0pIU0xSYTR5TWZJZkNUYUJNMkgrRjRvSzRQNk5BaU9SZ1VidXhCU1VE?=
 =?utf-8?B?N09ZdmlOS2prTHIvSmExcTZqZXdOYlh1VjcvVUZWTmZnbHg2RDhmV3MyWGtV?=
 =?utf-8?B?TFBQdEpLQ3lTVTN0REFYdVBmanZpZGZoWmxMZFV3d24zMVpZOGhOSTdid2ZB?=
 =?utf-8?B?dU10WTBLTFhKM0FSZ05GSEt2QUxDNUJtRjFHc3ZjZGFMbDRxMnF6VFp3ckhM?=
 =?utf-8?B?Y2c9PQ==?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64668a2d-687a-45db-b524-08db8488a029
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2023 16:37:34.2457
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bO0Pf53z73jBPBLAhn5FDc3rlSRGFsCj4eWPhP3FMGZtZzc7ZQYTNS5icqcfp+loh8CsC0v6xwLraM//jtIxfQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR12MB5816
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 7/14/23 03:17, Kai-Heng Feng wrote:
> On Thu, Jul 6, 2023 at 12:07 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 7/5/23 15:06, Bjorn Helgaas wrote:
>>> On Wed, Jun 28, 2023 at 01:09:49PM +0800, Kai-Heng Feng wrote:
>>>> On Wed, Jun 28, 2023 at 4:54 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Tue, Jun 27, 2023 at 04:35:25PM +0800, Kai-Heng Feng wrote:
>>>>>> On Fri, Jun 23, 2023 at 7:06 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>> On Tue, Jun 20, 2023 at 01:36:59PM -0500, Limonciello, Mario wrote:
>>>
>>>>> It's perfectly fine for the IP to support PCI features that are not
>>>>> and can not be enabled in a system design.  But I expect that
>>>>> strapping or firmware would disable those features so they are not
>>>>> advertised in config space.
>>>>>
>>>>> If BIOS leaves features disabled because they cannot work, but at the
>>>>> same time leaves them advertised in config space, I'd say that's a
>>>>> BIOS defect.  In that case, we should have a DMI quirk or something to
>>>>> work around the defect.
>>>>
>>>> That means most if not all BIOS are defected.
>>>> BIOS vendors and ODM never bothered (and probably will not) to change
>>>> the capabilities advertised by config space because "it already works
>>>> under Windows".
>>>
>>> This is what seems strange to me.  Are you saying that Windows never
>>> enables these power-saving features?  Or that Windows includes quirks
>>> for all these broken BIOSes?  Neither idea seems very convincing.
>>>
>>
>> I see your point.  I was looking through Microsoft documentation for
>> hints and came across this:
>>
>> https://learn.microsoft.com/en-us/windows-hardware/customize/power-settings/pci-express-settings-link-state-power-management
>>
>> They have a policy knob to globally set L0 or L1 for PCIe links.
>>
>> They don't explicitly say it, but surely it's based on what the devices
>> advertise in the capabilities registers.
> 
> So essentially it can be achieved via boot time kernel parameter
> and/or sysfs knob.
> 
> The main point is OS should stick to the BIOS default, which is the
> only ASPM setting tested before putting hardware to the market.

Unfortunately; I don't think you can jump to this conclusion.

A big difference in the Windows world to Linux world is that OEMs ship 
with a factory Windows image that may set policies like this.  OEM 
"platform" drivers can set registry keys too.

I think the next ASPM issue that comes up what we (collectively) need to 
do is compare ASPM policy and PCI registers for:
1) A "clean" Windows install from Microsoft media before all the OEM 
drivers are installed.
2) A Windows install that the drivers have been installed.
3) A up to date mainline Linux kernel.

Actually as this thread started for determining policy for external PCIe 
devices, maybe that would be good to check with those.

> 
> Kai-Heng
> 
>>
>>>>>> So the logic is to ignore the capability and trust the default set
>>>>>> by BIOS.
>>>>>
>>>>> I think limiting ASPM support to whatever BIOS configured at boot-time
>>>>> is problematic.  I don't think we can assume that all platforms have
>>>>> firmware that configures ASPM as aggressively as possible, and
>>>>> obviously firmware won't configure hot-added devices at all (in
>>>>> general; I know ACPI _HPX can do some of that).
>>>>
>>>> Totally agree. I was not suggesting to limiting the setting at all.
>>>> A boot-time parameter to flip ASPM setting is very useful. If none has
>>>> been set, default to BIOS setting.
>>>
>>> A boot-time parameter for debugging and workarounds is fine.  IMO,
>>> needing a boot-time parameter in the course of normal operation is
>>> not OK.
>>>
>>> Bjorn
>>