Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <ef5c74ce-d794-4b80-8d37-4a88de5db3f2@amd.com>
Date:   Tue, 12 Dec 2023 13:24:22 -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
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>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <20231212191635.2022520-3-Sanath.S@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VG1NY1BHSnE0dUdhK0pudzFmYy9zVTZYbUdpelNvR1dLRS9OM0VMQ3FkT2Fk?=
 =?utf-8?B?aEFEYTcyVlFvd2hLeDNYcW9aSW14c0dvM3dxa3g2UUg2WEI0Q01EY0VwUDE5?=
 =?utf-8?B?VDgvbVBCaldROGpLb2ZCK2JPV3k2NlFveC9ZUEVtbjlmbWsrUUEvZG5GOW5a?=
 =?utf-8?B?cnNkKzc0RTY4MzVMN3Z0eW1pTmE3STJmQThvbC9tYXBFeTgrQWg5bjFJNk4w?=
 =?utf-8?B?T0k3SkFEYVF5NzhKY1BXRWlQNHJMeEVjWGt2ZkZGdkdWZXQ3c095UldlNVVW?=
 =?utf-8?B?M25IWTdsbkRJU1ZkWXdJdHZNbWFaZHNraUlaM1JqOVh5YVZCbDJjakNNZGls?=
 =?utf-8?B?cVZKOHZVWHJ6aHIyV1BpYVZ1ek41WDNHRnJBbnNQS2xCbzFwRjhvanAyNGJH?=
 =?utf-8?B?aVlnRUxMWlZvOGhrRmdhMit6cjBhdHkyYmVJT2dLclBzbHhPbDkrbjFSV0py?=
 =?utf-8?B?ZkhTZFdaemhUeWJDRmtQUWlLcmloOEtnSVJONTVRcjBrSklhU1VMbUtoZDk5?=
 =?utf-8?B?VXFuVGVmYkkvTm5QZElJamhvc0U5THdIa1BBOWNzNFpSOEE2R3BTS3BTbGxi?=
 =?utf-8?B?cjloME9FaVo5cXh3dW0vbEpUZHRTV1hzWTl3WkxYY3NudjhHbER0LzJDVDEv?=
 =?utf-8?B?bEl3U2l4Zi9rWThBQlRWVk54bWEzY0x3cU9FY3NTbTczMzNyak9rREk3cDVL?=
 =?utf-8?B?OWZKTzlHYTBlQ092MlRPNGtFc0Y3azlDbm4xSGFzclI5MTF1aHd6bU12Sk5w?=
 =?utf-8?B?c2xWbGIwWnJHN0N0NXN2K3FhQUVUOVEwNVk2Vnpyci9BY0ZoNC9tQzJMYWN3?=
 =?utf-8?B?andFaGwrc0hGcEpMYzNxRm51dGppRXU0QUNvQ2NOR0ovdU1LZ2xRamdrOHJn?=
 =?utf-8?B?SDQzamFBZ3R1V0VIR1cydmJkcGVEMWkyeE91ZDYvVlFlbEtLa0RCdWFWVXBD?=
 =?utf-8?B?VVBaOG9EbzZMVytPUDRtMVZHVDV5NXFNMlVHbko2aDRVejdySlBjSmd2S2pj?=
 =?utf-8?B?YW5NakpKMUNoZmtWTGcrb3ZCUnUxSUltNGtPbnl6d3FCZElDWXU0N0NVVGND?=
 =?utf-8?B?Um5wYjhRZzZjaCtFNDN6RkhZYkM2YW0rRU1xQ2dUUW9xamVSZUVFTVpwdjI5?=
 =?utf-8?B?N09vQnYrY1B0eXdRRnlyUTNJRjJNMFF0RFIzbXJ5R04rMGdBaW9UdHBhMEI5?=
 =?utf-8?B?cW9HanNCR083VmNLeVorQXB2bTFBdUJUYml0U095U2pOOXZoK0xwTjdndTd5?=
 =?utf-8?B?Z2tYcGNqbmR1SmRoOEZkTmg1Z2N1M25weFlTR1lzYmhFaGtsc0FJblFDaU84?=
 =?utf-8?B?NXlJVzVQODJFQUxOZUwzZzYwbWgvY2srUDhYakNtREJhNnlVYVpOSm5CbDEz?=
 =?utf-8?B?RHhNVElHazZOZG1VVDNMWkNWd0hQc3MyQVFaMkRFcldHeWRzRFFqaWxNNC9S?=
 =?utf-8?B?WWNxbC9HZ0h5Q0xSVlB5Z3dyYXAwWHlTMG5seXBEZ0JUSVBrcC9wZGN4R3Qr?=
 =?utf-8?B?ditwckZRcjZnWHRReCsvcTV6OWdBeTFmeGtKVjRuQlI4a2RsaDQ5RXNlNVo5?=
 =?utf-8?B?VWdwbUxPanVLaFFySlVkaHBnakxETkdLbEJZUU9oeTJNY3NEZ2NGT2o0N2NT?=
 =?utf-8?B?OTBaK1BjOXBYV1RIMUFmQko5UkVRRDZyUUZFS01KWlJwSGZ3V0FNekxIZFY0?=
 =?utf-8?B?eElIVHh3Zi9RanVoTVh4dnhBNitSbjBIWHMvY1Fhazg4Q3V0T1ZQU1dJc2tn?=
 =?utf-8?B?NG5kNDNJdGVROXlidXJzVGZRdVpZOXdMK1l1WEpuOWl0NGR6U0NhS1RxWE5V?=
 =?utf-8?B?NzR5RmFYRmZaeFM1Tmc4a0FESytJbjhqS1NOOTZIQ1FXMmdRdzkyaCswTnBr?=
 =?utf-8?B?cWNuWk1scytzdXRid3lSdktBQW9ha29YbXNRVkMrUnRXRWI1THFMemNNV2hs?=
 =?utf-8?B?OGtHV0lrdVR2RDl2SVk4QXVTa2M4WWoxQldxWnlucmZrbDY5b014a2RsQmVi?=
 =?utf-8?B?S3NiNC9GZHBhNy9id2xRVG5QVXV0aUVPQzV5Tlk4c3dQSStnUmYzbGRnTGE0?=
 =?utf-8?B?WW5ZZEFOR3JNTkFHcWVTeko3YzYwcmdnNDRtVzJTc0FoandHU09PMHBma1Fz?=
 =?utf-8?Q?8cHKk5um3SfWs5TP0c6C72+hh?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9398a680-588a-4a81-1263-08dbfb47f38d
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:24:25.2101
 (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: NCKKMcyJDawFSNB8HkarX7kRw5CVcacmrznFVqWrVsxiGURZS8zE/2NyJNHgaIUqziLvRSe4HQzFppv4gTSS6g==
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 pete.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 (pete.vger.email [0.0.0.0]); Tue, 12 Dec 2023 11:24:38 -0800 (PST)

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.