Date:   Thu, 14 Dec 2023 21:00:29 +0530
From:   Sanath S <sanaths2@....com>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Sanath S <Sanath.S@....com>, mario.limonciello@....com,
        andreas.noever@...il.com, michael.jamet@...el.com,
        YehezkelShB@...il.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Patch v2 2/2] thunderbolt: Teardown tunnels and reset downstream
 ports created by boot firmware


On 12/14/2023 1:02 PM, Mika Westerberg wrote:
> On Thu, Dec 14, 2023 at 12:50:21PM +0530, Sanath S wrote:
>> On 12/14/2023 12:37 PM, Mika Westerberg wrote:
>>> On Thu, Dec 14, 2023 at 12:08:34PM +0530, Sanath S wrote:
>>>> On 12/13/2023 5:22 PM, Mika Westerberg wrote:
>>>>> On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote:
>>>>>> On 12/13/2023 11:53 AM, Mika Westerberg wrote:
>>>>>>> On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote:
>>>>>>>> On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote:
>>>>>>>>> On Wed, Dec 13, 2023 at 12:46:35AM +0530, 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.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: Mario Limonciello <mario.limonciello@....com>
>>>>>>>>>> Signed-off-by: Sanath S <Sanath.S@....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);
>>>>>>>>> Why this is needed?
>>>>>>>>>
>>>>>>>>> It should be enough, to do simply something like this:
>>>>>>>>>
>>>>>>>>> 	if (tb_switch_is_usb4(tb->root_switch))
>>>>>>>>> 		tb_switch_reset(tb->root_switch);
>>>>>> If we don't tear down of tunnels before performing the DPR, the PCIe
>>>>>> enumeration is failing.
>>>>>>
>>>>>> PCIe link is not coming up after DPR. Below log is missing without
>>>>>> performing path
>>>>>> deactivation before performing DPR and hence PCIe enumeration is not
>>>>>> initiated.
>>>>>>
>>>>>> [  746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present
>>>>>> [  746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up
>>>>>>
>>>>>> I think when we do a DPR, it internally does some handling with PCI Path
>>>>>> Enable bit(PE).
>>>>>> So, deactivation of PCIe path is necessary for DPR to work.
>>>>> Rigth, it should be enough to reset the protocol adapter config and path
>>>>> config spaces. I guess using discovery at this point is fine too but I
>>>>> would at least check how complex doing the minimal "reset" turns out.
>>>>>
>>>>> I mean in tb_switch_reset() for USB4 v1 routers it can go over all the
>>>>> adapters and perform "cleanup" or so.
>>>> I gave it a thought yesterday and we can do something like this:
>>>>
>>>> We are already doing tb_discovery(tb) in tb_start. This would
>>>> discover the path configuration done by Boot firmware.
>>>>
>>>> Now, we can place the tb_switch_reset() right below that api with
>>>> conditions suggested by you.
>>>>
>>>> And tb_switch_reset() would internally DPR for all down steam ports.
>>>>
>>>> It can look something like below:
>>>>
>>>>       /* Find out tunnels created by the boot firmware */
>>>>           tb_discover_tunnels(tb);
>>>>       /*
>>>>        * Reset USB4 v1 host router to get rid of possible tunnels the
>>>>        * boot firmware created. This makes sure all the tunnels are
>>>>        * created by us and thus have known configuration.
>>>>        *
>>>>        * For USB4 v2 and beyond we do this in nhi_reset() using the
>>>>        * host router reset interface.
>>>>        */
>>>>       if (host_reset && usb4_switch_version(tb->root_switch) == 1)
>>>>           tb_switch_reset(tb->root_switch);
>>>>
>>>> With this, we are making sure while we get a unplug event after doing a DPR,
>>>> We are clearing all the paths established by Boot firmware. This wouldn't be
>>>> possible
>>>> if we had not discovered the paths before we perform DPR.
>>>>
>>>> It would create inconsistency for a new hot plug if we have not cleared the
>>>> path configurations
>>>> of previous hot unplug events.
>>> Right. I would still check if doing protocol adapter "reset" + path
>>> config space clear in tb_switch_reset() is enough and how complex that
>>> ends up to be. I think that's all what is needed.
>>>
>>> If it turns out too complex, yes I guess something like this:
>>>
>>> 	/* Find out tunnels created by the boot firmware */
>>> 	tb_discover_tunnels(tb);
>>> 	/* Add DP resources from the DP tunnels created by the boot firmware */
>>> 	tb_discover_dp_resources(tb);
>>>
>>> 	if (host_reset && usb4_switch_version(tb->root_switch) == 1) {
>>> 		struct tb_tunnel *n, *tunnel;
>>>
>>> 		list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list)
>>> 			tb_deactivate_and_free_tunnel(tunnel);
>>>
>>> 		tb_switch_reset(tb->root_switch);
>>> 	}
>>>
>>> With proper comments would work, no?
>> Yes, this works. Tested it too and works fine.
> Cool.
>
>> Probably we can move tb_deactivate_and_free_tunnel() inside
>> tb_switch_reset() to make it
>> look better.
> Unfortunately that's not possible because tb_switch_reset() lives in
> switch.s (and should live there) and tb_deactivate_and_free_tunnel() is
> part of tb.c (as should be). This is actually why I would like to try
> the "reset" protocol adapters + their path config spaces in
> tb_switch_reset() as then that would work with any router and does not
> need to have any knowledge about tunnels or tb.c internals.
Ok.
To perform a protocol adapter reset we would require upstream and downstream
adapter ports. So that we can use tb_port_write() set the 
ADP_PCIE_CS_0.Path Enable bits.
Challenge here is to get the upstream port without discovering. I'll 
think more on this line
if its possible to reset the protocol adapters and its path.

If you have any reference here or any idea already in mind, let me know, 
I can give it a try :)

Powered by blists - more mailing lists