Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <8ff0355a-7ed3-4f15-9ad7-350e6094f00f@amd.com>
Date:   Mon, 6 Nov 2023 10:51:01 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 8/9] PCI: Exclude PCIe ports used for tunneling in
 pcie_bandwidth_available()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Karol Herbst <kherbst@redhat.com>, Lyude Paul <lyude@redhat.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Lukas Wunner <lukas@wunner.de>,
        Danilo Krummrich <dakr@redhat.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Xinhui Pan <Xinhui.Pan@amd.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Mark Gross <markgross@kernel.org>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>,
        =?UTF-8?Q?Marek_Beh=C3=BAn?= <kabel@kernel.org>,
        "Maciej W . Rozycki" <macro@orcam.me.uk>,
        Manivannan Sadhasivam <mani@kernel.org>,
        "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" 
        <dri-devel@lists.freedesktop.org>,
        "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" 
        <nouveau@lists.freedesktop.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:RADEON and AMDGPU DRM DRIVERS" 
        <amd-gfx@lists.freedesktop.org>,
        "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
        "open list:ACPI" <linux-acpi@vger.kernel.org>,
        "open list:X86 PLATFORM DRIVERS" 
        <platform-driver-x86@vger.kernel.org>,
        "open list:THUNDERBOLT DRIVER" <linux-usb@vger.kernel.org>
References: <20231103190758.82911-1-mario.limonciello@amd.com>
 <20231103190758.82911-9-mario.limonciello@amd.com>
 <bdae1a8-d62-6af6-316d-1e3a5ac15bc@linux.intel.com>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <bdae1a8-d62-6af6-316d-1e3a5ac15bc@linux.intel.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?M2NaL2p1ek03c1hGSUNGZWU0cjg0eURBM3p5bFFlNXhTUnpkQ3BKalpiMWlX?=
 =?utf-8?B?bFdGZXl2MllpKzNJcmFueUczRUFkU2ZhdUU5RTdVbnJ5RWFZcS85NkYwL1dU?=
 =?utf-8?B?TWFJQ3Q0aktzbVRTSjBZRld2eUNHS0FVbDFDN254UjJNRnRsY1BFcjBxTzFX?=
 =?utf-8?B?VnM4K1JlUnhXbEltdWJSUm93bFA2VHBLK0VoUzQ4SWJyU3RjWWxQYUlqQ1Z5?=
 =?utf-8?B?TGNkUmd0Rkg2YlZlbXpBM2owUHF4YWtsWjBWYmxvZFNaVCt6NGJjYkdwajVD?=
 =?utf-8?B?em9Fa1pCaExZOVI2Q1BUQi9xMWtSQ2lxdWFKSm9FN3NPU2hFRktxUDJ2NkY3?=
 =?utf-8?B?WUlHdVBLWUc3QkM1YXN3Y1ZiK2plMkVxRzNrRDRkV1VqRXRvUXd1enpKaU1t?=
 =?utf-8?B?MWUrVzNSbUd5VGVMdzJaN3NlY2M5ZDFxN2VxZGZxemVJUmlQbDR0aUY1RkFV?=
 =?utf-8?B?RzluZkh2N29xVFlVR1hSU005L0VrdmZKS2RZU01INkZ1b0lyR1JYem1nTmdh?=
 =?utf-8?B?RVFSYzg0LzhvSE9ZUGV5N3lQZmcxVzJiY2VTSjNVSkFvcVhvdG5sZlJFZEMw?=
 =?utf-8?B?am9LQUorSmZTajRENE9zeUxvTE9rY2pncTd5NTVqMDBhakRJU3NSZEJDVFA2?=
 =?utf-8?B?NTBVZWRMVzB5Q080Z25GVHFwQjkzbW5HTFdnTGhhOGpBTFNSM256UW5LcFFX?=
 =?utf-8?B?bkV2NGhEa0NkZmRjN0RFQ2xrN2l1V2swK09zUHNWcEVHRERYUjNMaUdrZmpz?=
 =?utf-8?B?MGIxc3B3UUhPMFVPRGdJdXpzdDB6eXJsQWhjN0hKRU5qejIvbU1ncHBQZmF3?=
 =?utf-8?B?VEcvRDFXYUR4MGdJb041NVpzN0s4OGRsVDRhbmdaQnVXb2d0d3VBT2pkU1gy?=
 =?utf-8?B?Q0V0bzhSME4xVExUK3AvVGgzZHFyajB3dVlZVURVMHNiN1VEbjVmRkdPaVRt?=
 =?utf-8?B?QWpxckNvSTFRZnUvWWpFTWVqUGlBbmlwUDZhSjA4NVIyYnY1d0llbGdLQ0Jq?=
 =?utf-8?B?ZWI3K3N3TmVqOTNLd21xVFNiMWJ0UXZNZkZRZGdpVEhpc3FYS1ZhMm1GbGJm?=
 =?utf-8?B?NnhsRTJuaFRBUFVxQW5obTQ0SXFNMWNlcndlYmhKbWpUamdLTGdXbFdzRzVY?=
 =?utf-8?B?dzJpblpIeUJrVHhkdGlQandQTE9ZOEZMMXhKTUtmK09NN3YvZkJhaDlTbFNU?=
 =?utf-8?B?eG5ScUtoc1QwMHpPQVFrQWgwZ0NSMzI5UHdLelRpbUNWTnhBYmtEbC9wZDdq?=
 =?utf-8?B?d3IyN2NkOCtyaEFSMjVUUVBtWTZ3ZllUby9UOHcwcllzY3RHdFhWYVNhem5L?=
 =?utf-8?B?T29XakRYTXZHek1jSmNsTFNqcmlFTG5TUDNobzd0YUl2SDQvZGFiSkpxd0RB?=
 =?utf-8?B?WkgvRExwRm1nWVQ1SkhZNVNhajgxdEVnSEN3L1lxY0l5SlBYSE1HUjNpbXNI?=
 =?utf-8?B?NVc5NG1XMktFampwcGRleC96RXh2TGNnTjVZdnc3d3RJei8vQ2dXTXJzcVdz?=
 =?utf-8?B?N0tGVEo5R3RIaDZNVHV1eXRZQ1BJajM3bUg0NHBtRVVKdXlMaDRmTVdVWVV6?=
 =?utf-8?B?TWEvRDU4emVkV1duNUlaWkl2SWppTFlaV2FUcFNRU3UwbmJ6NldqQnVTSzRy?=
 =?utf-8?B?RGxHblY4cFB0TnZ2SGJ5UllTV2tuNUlwVzZNdmpZMGlWbVN4UzArajQ3bmgz?=
 =?utf-8?B?VU94U2QvU0Iwc2xCVlF3OUhXUDZNUUxGNjBOTVk4VXJUR0Z2RkhsdEdiM2xq?=
 =?utf-8?B?TGVHQlZGQ2VXaTVYSjBnTnhnOXAvYkxMaFdiaDFZZEtSQnJFYnpNVlFLMEtL?=
 =?utf-8?B?NThrcU9NTmxjdDVyV0dkYjZiT01kU3JOZnFzUyt3OXUrdHk3WFlFOTJNSEY3?=
 =?utf-8?B?aUlocWRRVVJSNEtBRFBSRGdOcHJ0NjhCWkFla1FZZGQ5YUZCQ0pJT1FiVVNM?=
 =?utf-8?B?dExxNkxIcWJCTm1SZHh3TlFMbTJEQ3Z5SUFpK2NIZUFMbWhOY0JCaCs1aEFk?=
 =?utf-8?B?Ums4T0FjZDNSSE8yQTcyL1pmdmRjdmtpZndxUFAzQldwN0tCTC9KSFd5U0th?=
 =?utf-8?B?dWxhQVZwTFRob1lTVWNPcTU4UEEvNUowalB1MjhmOXpQSEFkcHhya1lDczF0?=
 =?utf-8?Q?0zH3+kokVdB+LOu2Vt65e5Pqc?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c8948ce1-9e7f-4c42-ec3c-08dbdee8901d
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2023 16:51:03.6565
 (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: FwV6QNIwJZ4KgOgaLK8nDw78hJkxtgjej0WzJiyjY5jnvTrnQ86ZYOVTFhcOP/qwLHdji7YgfYkJKTlNMibhmQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB9097
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE 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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Mon, 06 Nov 2023 08:51:24 -0800 (PST)

On 11/6/2023 06:52, Ilpo Järvinen wrote:
> On Fri, 3 Nov 2023, Mario Limonciello wrote:
> 
>> The USB4 spec specifies that PCIe ports that are used for tunneling
>> PCIe traffic over USB4 fabric will be hardcoded to advertise 2.5GT/s and
>> behave as a PCIe Gen1 device. The actual performance of these ports is
>> controlled by the fabric implementation.
>>
>> Downstream drivers such as amdgpu which utilize pcie_bandwidth_available()
>> to program the device will always find the PCIe ports used for
>> tunneling as a limiting factor potentially leading to incorrect
>> performance decisions.
>>
>> To prevent problems in downstream drivers check explicitly for ports
>> being used for PCIe tunneling and skip them when looking for bandwidth
>> limitations of the hierarchy. If the only device connected is a root port
>> used for tunneling then report that device.
>>
>> Downstream drivers could make this change on their own but then they
>> wouldn't be able to detect other potential speed bottlenecks from the
>> hierarchy without duplicating pcie_bandwidth_available() logic.
>>
>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2925#note_2145860
>> Link: https://www.usb.org/document-library/usb4r-specification-v20
>>        USB4 V2 with Errata and ECN through June 2023
>>        Section 11.2.1
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/pci.c | 74 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 49 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d9aa5a39f585..15e37164ce56 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6223,6 +6223,35 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
>>   }
>>   EXPORT_SYMBOL(pcie_set_mps);
>>   
>> +static u32 pcie_calc_bw_limits(struct pci_dev *dev, u32 bw,
>> +			       struct pci_dev **limiting_dev,
>> +			       enum pci_bus_speed *speed,
>> +			       enum pcie_link_width *width)
>> +{
>> +	enum pcie_link_width next_width;
>> +	enum pci_bus_speed next_speed;
>> +	u32 next_bw;
>> +	u16 lnksta;
>> +
>> +	pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
>> +	next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
>> +	next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT;
>> +	next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
>> +
>> +	/* Check if current device limits the total bandwidth */
>> +	if (!bw || next_bw <= bw) {
>> +		bw = next_bw;
>> +		if (limiting_dev)
>> +			*limiting_dev = dev;
>> +		if (speed)
>> +			*speed = next_speed;
>> +		if (width)
>> +			*width = next_width;
>> +	}
>> +
>> +	return bw;
>> +}
>> +
>>   /**
>>    * pcie_bandwidth_available - determine minimum link settings of a PCIe
>>    *			      device and its bandwidth limitation
>> @@ -6236,47 +6265,42 @@ EXPORT_SYMBOL(pcie_set_mps);
>>    * limiting_dev, speed, and width pointers are supplied) information about
>>    * that point.  The bandwidth returned is in Mb/s, i.e., megabits/second of
>>    * raw bandwidth.
>> + *
>> + * This excludes the bandwidth calculation that has been returned from a
>> + * PCIe device used for transmitting tunneled PCIe traffic over a Thunderbolt
>> + * or USB4 link that is part of larger hierarchy. The calculation is excluded
>> + * because the USB4 specification specifies that the max speed returned from
>> + * PCIe configuration registers for the tunneling link is always PCI 1x 2.5 GT/s.
>> + * When only tunneled devices are present, the bandwidth returned is the
>> + * bandwidth available from the first tunneled device.
>>    */
>>   u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
>>   			     enum pci_bus_speed *speed,
>>   			     enum pcie_link_width *width)
>>   {
>> -	u16 lnksta;
>> -	enum pci_bus_speed next_speed;
>> -	enum pcie_link_width next_width;
>> -	u32 bw, next_bw;
>> +	struct pci_dev *tdev = NULL;
>> +	u32 bw = 0;
>>   
>>   	if (speed)
>>   		*speed = PCI_SPEED_UNKNOWN;
>>   	if (width)
>>   		*width = PCIE_LNK_WIDTH_UNKNOWN;
>>   
>> -	bw = 0;
>> -
>>   	while (dev) {
>> -		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
>> -
>> -		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
>> -		next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
>> -			PCI_EXP_LNKSTA_NLW_SHIFT;
>> -
>> -		next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
>> -
>> -		/* Check if current device limits the total bandwidth */
>> -		if (!bw || next_bw <= bw) {
>> -			bw = next_bw;
>> -
>> -			if (limiting_dev)
>> -				*limiting_dev = dev;
>> -			if (speed)
>> -				*speed = next_speed;
>> -			if (width)
>> -				*width = next_width;
>> +		if (dev->is_tunneled) {
>> +			if (!tdev)
>> +				tdev = dev;
>> +			goto skip;
>>   		}
>> -
>> +		bw = pcie_calc_bw_limits(dev, bw, limiting_dev, speed, width);
>> +skip:
>>   		dev = pci_upstream_bridge(dev);
>>   	}
>>   
>> +	/* If nothing "faster" found on link, limit to first tunneled device */
>> +	if (tdev && !bw)
>> +		bw = pcie_calc_bw_limits(tdev, bw, limiting_dev, speed, width);
>> +
>>   	return bw;
>>   }
>>   EXPORT_SYMBOL(pcie_bandwidth_available);
>>
> 
> This patch should be split into two, where one just moves the code to the
> new function.

Good idea, thanks.

> 
> Also note that this will conflict with the FIELD_GET() changes (try to
> not reintroduce non-FIELD_GET() code when you rebase this on top of
> v6.7-rc1 :-)).
> 

Sure, will adjust for that when it's rebased to 6.7-rc1.