Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <747bd38a-898f-4556-a0df-a83352cf9e10@amd.com>
Date:   Tue, 12 Dec 2023 13:25:38 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [Patch v2 2/2] thunderbolt: Teardown tunnels and reset downstream
 ports created by boot firmware
Content-Language: en-US
From:   Mario Limonciello <mario.limonciello@amd.com>
To:     Sanath S <Sanath.S@amd.com>, andreas.noever@gmail.com,
        michael.jamet@intel.com, mika.westerberg@linux.intel.com,
        YehezkelShB@gmail.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231212191635.2022520-1-Sanath.S@amd.com>
 <20231212191635.2022520-3-Sanath.S@amd.com>
 <ef5c74ce-d794-4b80-8d37-4a88de5db3f2@amd.com>
In-Reply-To: <ef5c74ce-d794-4b80-8d37-4a88de5db3f2@amd.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?ZVdYSG9WRlppNitNUGxRbWJ2RXd3NWlxWGxSRnJLWmkxU3Y2Vmg5VGY4ajBt?=
 =?utf-8?B?cHBCeU5PcFN6TFBqMUZ3QnpNOWtTcnVGKzJMV2FqWWJweWJUU3VPaVA2aWNQ?=
 =?utf-8?B?a0NZMmhkeHMwUURIa2dISHEwYkxoNXZ4YUdWRDcvU05haElHZHdSbGJDbDh2?=
 =?utf-8?B?b1J4S0E2cE1KSS91eXZqdlpCbEVldm90MmVrcEpPRWVZSVRvRlE5aGJmNkZO?=
 =?utf-8?B?VHFxeHlsTHpqQ2RWWGFzTXYxaURVWXRvL1RRWm41aFJmb3cyQzI0ZnZ2NWJO?=
 =?utf-8?B?WmRadDM3dWxwL0J1eTQrMUFpUXNlSGNsdUg2UnJCMGRZSUNsMnI0c2g3R1FN?=
 =?utf-8?B?NUNCWithc1VxRjdNSHQ4M1VGKzAvdnVzek9tTnM0MkZFKzh0Y1l4c0JVa01W?=
 =?utf-8?B?SERIUFRyUEdQY1RPcXUvNXd3dml2RzVtUG01UHRQRDR5ZGFwSmgxY2Y0VWVB?=
 =?utf-8?B?Z1JqTFpOMzFJcWFXV1A3cG5hQVpmTU41R2hrUzFXYUZXMDloRmpiUVV0NFJw?=
 =?utf-8?B?dzlGOUdjTENMYTV3emhHVGFJYWsvY2lRS1I0Z3pYejNJZUtsbmY0VVVORDJ5?=
 =?utf-8?B?U2R6ZTRYa00vVHhxb0ErQit2MU5RR2JZdlVBU0RET3JQMytncTVoNHNrL093?=
 =?utf-8?B?OGp5c2VETGx5bmJrTk4vUEFLM1ZHbERnTGpkcTJyOFFTcFdES2ZTMkZSeStk?=
 =?utf-8?B?aVZuN3hUVlJYdVlMVkRKRmpFenVmVnpjbVpZN21DZDRnQ3oxQTc4R0J4T1ht?=
 =?utf-8?B?Y0VBN3d4UDRNTjlSNXZRMVQ4RzZDeFk3TkR3MzlkQU5nUUNWTnVmUEZjci9U?=
 =?utf-8?B?N3oyNkcxVm1nOHBMRG9Rdjc5WmQ3VktyVVA3dmN2aU9LTURydmNIdWhtNXhq?=
 =?utf-8?B?RWNaSjRjTmtnMVRtR21ua29LMlpUNEgrdURtOXVvMGtDMzNBaWQreEFTSGxI?=
 =?utf-8?B?SzBpUlZwczZ6cktzTXRaQm9HRDlBd0dSTTN6T0J1d1FUbFlhY2p6eTJaVTZ0?=
 =?utf-8?B?QTZkb2JocjZMYnlTUDBWNUxiZ0xrV1dTbE9keE9nMGpEU1hubjJ4djlZWnRa?=
 =?utf-8?B?Sjl5NE1nQk01a2l4SE8waklvNkJvS2djYmxJaitnTGdGNjIrV3JnYS9vZFFT?=
 =?utf-8?B?SlJGQi9jeUhVNGJGQWpzVXZCSGtINSt0REtPOXh1VTkxenp6bWd4Q3FGNXR4?=
 =?utf-8?B?dWlsK1NCS0c1OERlQTNtOThyaXVqaWFWRUZkQjNhdzQ4U0V0cjFaem1aTmNs?=
 =?utf-8?B?RnBpaDFEYXlFVldvQ3E5clROZGN5ZjhrWlNWNTA5d1N4elY3aEp3cFFPOThu?=
 =?utf-8?B?bllacDAzOE1xZFJ4MlA2aDFoVmMycGN5WUovRER2cDFMRWdITmVNa3ZGRTJr?=
 =?utf-8?B?S0R4Y3hPZ3lHbStwT0pRMVNjK0hYc1ZZTU5tUzBHcUNBbkZlRWlvVVd5MUhT?=
 =?utf-8?B?Q3k0bnZEMVdaZDR2clpxZG9vd1M0by9XU3lHVnBKWDluNGdQejgrVjdrUmlw?=
 =?utf-8?B?dTBjRjZkb2psdnFrKzVFcmJuby80RXB0KzhjanZlL1FzUTlVNGczZ3ArWFlo?=
 =?utf-8?B?QVh5OFVDcDhjU0haNEVFU1BoQWk5SUZiWk00V0x5QU10TWJtM0xwR2pGbktB?=
 =?utf-8?B?QVhjUTBVUC9CcWxaZjV5UHRlbTF0VGZTY3M1NHdCMkdtc1JoSERtQXBsYzhE?=
 =?utf-8?B?WVVEZFpLSS9FdTV3L1FKcGFNamdmN1gyTzRWVHFqQkgrM09MSG1KWlZGTm5S?=
 =?utf-8?B?V2pDb0RpL3BKZVNMVnNJeklCVXpJeEFZNWxZRjh6THFxZGdKQ3dJMjVpcks5?=
 =?utf-8?B?RlBHK1A5TnZHT0p6SU1ubW9kYytwaHJFWmsyMzBrTUp5TWNPbG4xdi9rRkNQ?=
 =?utf-8?B?WjVkWDlydU9pa1cvNzU5T1QwQTVQcWhpVE5iVnJmcjdHNzBmY1luTHVoU3hr?=
 =?utf-8?B?WGc2R01QNHlWSS80TmFKdzRFdmw1Z2VjQm5aQzFWdXFzZkJyT0M1WUZNdHFm?=
 =?utf-8?B?eU9MRWJ5ODZaRDNvY2FzN0FkY2xkTEg3MEZnSExvQ3MzQ3ZSWHNyYjlrRTA1?=
 =?utf-8?B?THBuM1I2MWRwU3luY2JqNmhqY0xaR3JYMDhDeWV2TjRGMGpRTDZLR1dRajE0?=
 =?utf-8?Q?yv7KEGZ1/QypGla8ie6EFwJEd?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f137473-bcc2-449a-246c-08dbfb481fab
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2023 19:25:39.2163
 (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: 5fAXAgymMDiVvEVyM8HUJU8dE+kCCjS8AR21u2yCMyNeIYHHA/v3doDWlZjJ7hM4Kbe6DxCaKp9NSYqPtfp7og==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8740
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
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 (howler.vger.email [0.0.0.0]); Tue, 12 Dec 2023 11:26:24 -0800 (PST)

On 12/12/2023 13:24, Mario Limonciello wrote:
> On 12/12/2023 13:16, Sanath S wrote:
>> Boot firmware might have created tunnels of its own. Since we cannot
>> be sure they are usable for us. Tear them down and reset the ports
>> to handle it as a new hotplug for USB3 routers.
> s/3/4/
> 
>>
>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Sanath S <Sanath.S@amd.com>
>> ---
>>   drivers/thunderbolt/tb.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>> index fd49f86e0353..febd0b6972e3 100644
>> --- a/drivers/thunderbolt/tb.c
>> +++ b/drivers/thunderbolt/tb.c
>> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb)
>>       tb_switch_tmu_enable(tb->root_switch);
>>       /* Full scan to discover devices added before the driver was 
>> loaded. */
>>       tb_scan_switch(tb->root_switch);
>> +    /*
>> +     * Boot firmware might have created tunnels of its own. Since we 
>> cannot
>> +     * be sure they are usable for us, Tear them down and reset the 
>> ports
>> +     * to handle it as new hotplug for USB4 routers.
>> +     */
>> +    if (tb_switch_is_usb4(tb->root_switch)) {
>> +        tb_switch_discover_tunnels(tb->root_switch,
>> +                       &tcm->tunnel_list, false);
>> +        tcm->hotplug_active = true;
>> +        return tb_switch_reset_ports(tb->root_switch);
>> +    }
>>       /* Find out tunnels created by the boot firmware */
>>       tb_discover_tunnels(tb);
>>       /* Add DP resources from the DP tunnels created by the boot 
>> firmware */
> 
> Doesn't this cause the following to not run and thus break hotplug?
> 
> tcm->hotplug_active = true;
> 
> 
> I think it would be better to do this like this flow:
> 
>      if (tb_switch_is_usb4(tb->root_switch)) {
>          tb_switch_discover_tunnels(tb->root_switch,
>                         &tcm->tunnel_list, false);
>          tcm->hotplug_active = true;
>          ret = tb_switch_reset_ports(tb->root_switch);
>          if (ret)
>              return ret;
>      } else {
>          /* keep existing tunnel flow */
>      }
> 
>      tcm->hotplug_active = true;
> 
>      return 0;
> 
> That makes it crystal clear that hotplug isn't enabled until it's done 
> being setup, which means either getting the existing tunnels or doing 
> the reset.

My apologies I completely missed replacement call even though I pasted it.

Besides the s/3/4 this looks fine to me.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>