Messages in this thread | | | Date | Thu, 14 Dec 2023 12:08:34 +0530 | Subject | Re: [Patch v2 2/2] thunderbolt: Teardown tunnels and reset downstream ports created by boot firmware | From | Sanath S <> |
| |
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@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); >>>>> 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. >>>> Actually this needs to be done only for USB4 v1 routers since we already >>>> reset USB4 v2 hosts so something like: >>>> >>>> /* >>>> * 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 (usb4_switch_version(tb->root_switch) == 1) >>>> tb_switch_reset(tb->root_switch); >>>> >>>> (possibly add similar comment to the nhi_reset() to refer this one). >>> Oh, and would it be possible to tie this with the "host_reset" parameter >>> too somehow? I guess it could be moved to "tb.c" and "tb.h" and then >>> check it from nhi.c as already done and then here so this would become: >>> >>> if (host_reset && usb4_switch_version(tb->root_switch) == 1) >>> tb_switch_reset(tb->root_switch); >> Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in >> this case. >> If its needed, then we have to modify to enable host_reset in nhi.c as well. > Well you are effectively doing that here, no? You "reset" the host > router therefore tying this to the same command line parameter makes > sense and allows user an "escape hatch" if this turns out breaking > things.
| |