Received-SPF: pass (google.com: domain of linux-kernel+bounces-16967-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <558fb39a-470d-16e3-0043-510e6b4b3f8e@amd.com>
Date: Thu, 4 Jan 2024 22:19:20 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [Patch v2 2/2] thunderbolt: Teardown tunnels and reset downstream
 ports created by boot firmware
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Sanath S <Sanath.S@amd.com>, mario.limonciello@amd.com,
 andreas.noever@gmail.com, michael.jamet@intel.com, YehezkelShB@gmail.com,
 linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <c7d174d3-028f-9ce4-7ef5-3e033c195159@amd.com>
 <20231215140224.GX1074920@black.fi.intel.com>
 <866cb714-b9a8-a7d4-4c59-6ba771ef325f@amd.com>
 <20231218104234.GB1074920@black.fi.intel.com>
 <c433f29b-597c-b6d6-aa48-2b84a26dc623@amd.com>
 <20231218113151.GC1074920@black.fi.intel.com>
 <20231218122312.GE1074920@black.fi.intel.com>
 <997f2a94-66d9-fb95-8f75-46d61937f7e8@amd.com>
 <20231218131840.GH1074920@black.fi.intel.com>
 <0fd5c09f-1cf2-8813-a8f9-1bd856e3a298@amd.com>
 <20231219122634.GJ1074920@black.fi.intel.com>
Content-Language: en-US
From: Sanath S <sanaths2@amd.com>
In-Reply-To: <20231219122634.GJ1074920@black.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?d29abmlWMnBaNzkzWUUyZnBQK1JkVVlnT0FEeEhVeWk4Mm04UDZmMFJybjNm?=
 =?utf-8?B?NUFZRFFhVUNMRFNWdUhMY2pJZ2h1Q3U5djZVZkFzQ2ZNeFBDTy8yVnU1UjBz?=
 =?utf-8?B?LzBKTkl5Zys1dzc0ZXFlZUU4Vlg1dy9mTDc4NE4zdlJnUHRRdDRHTnoxWHN2?=
 =?utf-8?B?YlQ5eFhDcHkwVWdqa0hwaloyZXZYbEhySzlBV25qZytGTFhHMm1INjRvb2tI?=
 =?utf-8?B?TTRCaHZ5dHJld1RyZzQ5cmJUay9KK0N4S3BFV0tqYThGMmdQS004bTdaeUcz?=
 =?utf-8?B?QWJlV3lISXFBbTVZczRuS0NFWHY0VDc5NVN4SkFYS1dPd08xR1ZEWHllUFBt?=
 =?utf-8?B?eDlTTy9PTDh2ZEhsMDRWR0Y5WERqR2FiYkNtdCtJczF4Q01BRURUYzE2MGNi?=
 =?utf-8?B?TSt1dU1ITmF0bWFva0pPTU43dnJPQjV4VkdhbFJTZnFTRDNHc0pKQUNvMUpn?=
 =?utf-8?B?M2E4WkhmZEF3bnFWWGIvNjlON3F6dTJ0OUMyVHBXSldWTE5ZSVRhZko5anZt?=
 =?utf-8?B?VFRtTnFDUEM4UFpEdWRrR2Zkd3hpTFVPdGVUT1FFRkhrYjA5SXFCRFByT0t4?=
 =?utf-8?B?OVY2YWpKa3ZjZ012ZTVGdHBTeXhZMUdiMDRHK1pWRi9ZMWordVpsLzBGVFhB?=
 =?utf-8?B?TEFuenV6Mkk1T3VZTXd1YkRTRnpZelM3LzBQV2VlK3FlKzdHeU53YlRsSjlT?=
 =?utf-8?B?dExydEl2QzIra3lSU0hjcUhRMWNyUCs2VHJLTGR3ZEdPR0NOblo2M1hjTEVp?=
 =?utf-8?B?QTlPT2NabmdQOFAxUzVwN1l1cW13Mm16d2FRbXZmWGFYcFRkRjFhNjExSzFG?=
 =?utf-8?B?UzJXaXRTRUFUdDNGLzdtcXpoQTZBU1NOUExGMG5pUTkwMDhLZm1taUtOMVA0?=
 =?utf-8?B?SkxnS1RxRXgrNXJWcERSbyt6RTRzNUJHTHRkKzFjc1kxUHFrUlpNV2tPUllG?=
 =?utf-8?B?Sy9WYVBvaEhienVlQUUxUTNlazh1bUhLL2dmVUtpY3Q1VEpZV3V5dEhhMVZS?=
 =?utf-8?B?TnNHUVdvdXNEelpJUGcxRVRKWWNpRG9kbkVlTEJ3S1Y3NHVsV3pHa0JNbXg5?=
 =?utf-8?B?bkliUkh3RWJFTERpU3JycmdIUWlpdFEvWVlaSFJGVGExM1RpYW5HZVQrcEdG?=
 =?utf-8?B?cVpTcFk4elViTnkwUW1oZFNHMnlBZlZMQ3dyV1pwdVJFQkt4ZnBYQ0gxZ0NQ?=
 =?utf-8?B?M0VMREJ2SFQ4TjM5dVVaZWpsTE1LQmYwendMNisvYUp3Q2Vkenk0UEkvM2tP?=
 =?utf-8?B?MWhTbXVjTkN6RUQ2WGptYVRHeWNIcmdDNU9ZTG9VL21hRFI0YkduWTdxeVo3?=
 =?utf-8?B?N1grTURkSEtlbUFqWDFLQzNCQXB3cXN4bTdLSmh4ZG9WNGx5UDVZYU13OUMz?=
 =?utf-8?B?VkRhVkRkRG9CQUZTTkhkdjJGUGlRSTdQNWk5Z3pETnNYU1J3anBTMi9jWTk0?=
 =?utf-8?B?K1lESzBQd3duckphOEt6VVR2SCtWM0dab2N2UUR2U2djTzY2cmI4eXpBaUtq?=
 =?utf-8?B?UERQUmtoQU9HTXQrVmcwdkh4Z3RFL1hsbHJCRmVXakRLclJTVUVmbDJpcFVk?=
 =?utf-8?B?bitHQnZHVi9jSnJJemxQRVpEWXlhOXVieEFCZ3BycCtPRjdSVXplQStLSjVh?=
 =?utf-8?B?Q2RRdzliVDVidFBQWDl4N245Uml3THFUQ2NLY2VROHV5SkcyUXRCYkNhUnFr?=
 =?utf-8?B?RjVJRXNaT0NoeDdDc1hmc3Q2dFhuS25PQTRZdDZqbUtaRjRiK1RrTzNzYlND?=
 =?utf-8?B?elJPU2IvMkkzYzUybW5zQUJ2QVNCMmNyejNIbWxzWExmUUl3Z0xtWjVHVDhF?=
 =?utf-8?B?eTdvbkdYZmNJYWI3UlpkeWlqSWtQWFRvUXV0Z1FvODBhZHZTWGlDM2k4aTQr?=
 =?utf-8?B?RWFkY3JpWitmbzUyOXBIODFITDcvVE1lVWtjS2l0S2VYTDVVMUg1Yng0SEoy?=
 =?utf-8?B?emRBcENQMWcycTgrb2tDTUZYYktKcDNZKzFVbTZ3KzJNZ3ZBeFFqUTF0Y1hB?=
 =?utf-8?B?aGVteEpJVVRVTUYvamtMWXZTQW5OTytkdzFmU3pIY2d3QzBlK3FGMUlCT0xm?=
 =?utf-8?B?cVZlT3FMbmdhZElLOTRWK2hmSHp1amJFb0NuMUxsMjdMQjNXQm9DYlBGZk44?=
 =?utf-8?Q?OvG6VIjWmyLJMlFxetnwBujUc?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f897333-2602-4dfd-7b4b-08dc0d451e1b
X-MS-Exchange-CrossTenant-AuthSource: MW4PR12MB7016.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 16:49:29.2574
 (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: cLoQHPEMnSs54V0ZNYCydRiR30G1YDW+8CcxGYzELRT6RxyRjwpS67M+Y8rZyut3SXXo4vcghwCEx3qFEhu1pQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5359


On 12/19/2023 5:56 PM, Mika Westerberg wrote:
> On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote:
>> On 12/18/2023 6:48 PM, Mika Westerberg wrote:
>>> On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote:
>>>> On 12/18/2023 5:53 PM, Mika Westerberg wrote:
>>>>> On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote:
>>>>>> On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote:
>>>>>>>> The discover part should not do anything (like write the hardware) so
>>>>>>>> perhaps it is just some timing thing (but that's weird too).
>>>>>>>>
>>>>>>>> I think we should do something like this:
>>>>>>>>
>>>>>>>> 1. Disable all enabled protocol adapters (reset them to defaults).
>>>>>>>> 2. Clear all protocol adapter paths.
>>>>>>>> 3. Issue DPR over all enabled USB4 ports.
>>>>>>>>
>>>>>>>> BTW, what you mean "didn't work"?
>>>>>>> Path activation would go fine after DPR like below:
>>>>>>>
>>>>>>> [   15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating
>>>>>>> [   15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5
>>>>>>> to 2:9
>>>>>>> [   15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to
>>>>>>> 0:5
>>>>>>>
>>>>>>> But, PCIE enumeration doesn't happen (pcie link up will not happen, will not
>>>>>>> see below logs)
>>>>>>> [   15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present
>>>>>>> [   15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up
>>>>>> Okay, what if you like reset the PCIe adapter config spaces back to the
>>>>>> defaults? Just as an experiment.
>>>>> If this turns out to be really complex then I guess it is better to do
>>>>> it like you did originally using discovery but at least it would be nice
>>>>> to see what the end result of this experiment looks like :)
>> I feel it's better to go with discover and then reset for now (as v3).
>> I'll keep this experiment as "to do" and will send out when I crack it down.
I got what we were missing. It's not required to do a discover_tunnel 
before we tear down.

We were resetting the downstream port and do a "continue;"
So, we were not cleaning up its path. Actually we have to cleanup its 
path after DPR.

After changing it, It works without any tunnel_discover() api's.

+            if (tb_port_is_null(port) && !tb_is_upstream_port(port)) {
+                ret = tb_port_reset(port);
+                if (ret)
+                    return ret;
+                continue; >>>>>>>>>>>>>>>>>> Need to be removed
+            } else if (tb_port_is_usb3_down(port) ||
+                   tb_port_is_usb3_up(port)) {
+                tb_usb3_port_enable(port, false);
+            } else if (tb_port_is_dpin(port) ||
+                   tb_port_is_dpout(port)) {
+                tb_dp_port_enable(port, false);
+            } else if (tb_port_is_pcie_down(port) ||
+                   tb_port_is_pcie_up(port)) {
+                tb_pci_port_enable(port, false);
+            } else {
+                continue;
+            }

-    tb_sw_dbg(sw, "resetting switch\n");
+            /* Cleanup path config space of protocol adapter */
+            for (i = TB_PATH_MIN_HOPID;
+                 i <= port->config.max_in_hop_id; i++) {
+                ret = tb_path_deactivate_hop(port, i);
+                if (ret)
+                    return ret;
+            }
+        }
> Fair enough.
>
>>>> Yes, I'll give a try.
>>>> As an experiment, I tried to compare the path deactivation that occurs at
>>>> two place.
>>>> 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i).
>>>> 2. While we get a unplug event after doing DPR.
>>>>
>>>> I observed both have different hop_index and port numbers.
>>>> So, are we calling tb_path_deactivate_hop with wrong hop ids ?
>>> Wrong adapters possibly.
>>>
>>>>   From deactivate tunnel (called while unplug) :
>>>> [    3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from
>>>> 2:9 to 0:5
>>>> [    3.408282] deactivate hop port = 9 hop_index=8
>>>> [    3.408328] deactivate hop port = 2 hop_index=10
>>> Definitely should be port = 5 (that's PCIe down in your log) and
>>> hop_index = 8 (that's the one used with PCIe).
>>>
>>>> Deactivate from tb_switch_reset() :
>>>> deactivate hop port = 5 hop_index=8
>>> Can you add some more logging and provide me the dmesg or
>>> alternativively investigate it yourself. You can use tb_port_dbg() to
>>> get the port numbers to the log.
>> I've sent you complete dmesg.
> Got it, thanks!
>
>> Here is the log w.r.t port numbers and path clean up.
>>
>> [    3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR
>> [    3.389065] Calling usb4_port_reset
>> [    3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN
>> [    3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path,
>> port->port = 4 hopid = 8
>> [    3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4
>> hop_index=8
>> [    3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down
>> [    3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path,
>> port->port = 5 hopid = 8
>> [    3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5
>> hop_index=8
>> [    3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN
>> [    3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path,
>> port->port = 6 hopid = 8
>> [    3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6
>> hop_index=8
>> [    3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path,
>> port->port = 6 hopid = 9
>> [    3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6
>> hop_index=9
>> [    3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN
>> [    3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path,
>> port->port = 7 hopid = 8
>> [    3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7
>> hop_index=8
>> [    3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path,
>> port->port = 7 hopid = 9
>> [    3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7
>> hop_index=9
>> [    3.391342] Reset success
>> [    3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged
>> [    3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating
>> [    3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from
>> 0:4 to 2:16
>> [    3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4
>> hop_index=8
>> [    3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1
>> hop_index=9
>> [    3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16
>> to 0:4
>> [    3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16
>> hop_index=8
>> [    3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2
>> hop_index=9
>> [    3.391701] thunderbolt 0000:c4:00.6: total paths: 3
>> [    3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled
>> [    3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating
>> [    3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from
>> 2:9 to 0:5
>> [    3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9
>> hop_index=8
>> [    3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2
>> hop_index=10
>> [    3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5
>> to 2:9
>> [    3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5
>> hop_index=8
>> [    3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1
>> hop_index=10
>> [    3.392799] thunderbolt 0-2: device disconnected
>>
>> But it seems like we are not cleaning up all the paths ?
> To me this looks correct and even your dmesg the PCIe tunnel that gets
> established after the "reset" seems to be working just fine. I also see
> that in your log you are doing the discovery before reset even though
> the original idea was to avoid it.
>
> In any case this was a good experiment. I will see if I can get this
> working on my side if I have some spare time during holidays.
>
> I guess we can to with the discovery but taking into account the
> "host_reset".
>
> One additional question though, say we have PCIe tunnel established by
> the BIOS CM and we do the "reset", that means there will be hot-remove
> on the PCIe side and then hotplug again, does this slow down the boot
> considerably? We have some delays there in the PCIe code that might hit
> us here although I agree that we definitely prefer working system rather
> than fast-booting non-working system but perhaps the delays are not
> noticeable by the end-user?