| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c433f29b-597c-b6d6-aa48-2b84a26dc623@amd.com>
Date: Mon, 18 Dec 2023 16:49:13 +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/18/2023 4:12 PM, Mika Westerberg wrote:
> On Mon, Dec 18, 2023 at 03:50:26PM +0530, Sanath S wrote:
>> On 12/15/2023 7:32 PM, Mika Westerberg wrote:
>>> On Fri, Dec 15, 2023 at 07:24:07PM +0530, Sanath S wrote:
>>>> On 12/15/2023 5:25 PM, Mika Westerberg wrote:
>>>>> On Thu, Dec 14, 2023 at 09:00:29PM +0530, Sanath S wrote:
>>>>>>> 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 :)
>>>>> Here is something I had in mind. Only build tested now, and not split
>>>>> into proper patches. This would make it possible to reset any router if
>>>>> we ever need that (not sure if we want to add the TBT2/3 bits now). Let
>>>>> me know if you see problems with this.
>>>>>
>>>>> Feel free to use this code as you like (or ignore completely).
>>>> Thanks Mika for your suggestion.
>>>> It looks good to me and I'll be checking this on Monday.
>>> Okay.
>> Unfortunately this is not working :(
>>>> I have one doubt on protocol adapter reset part which I have mentioned
>>>> below.
>>>>> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
>>>>> index ec7b5f65804e..31f3da4e6a08 100644
>>>>> --- a/drivers/thunderbolt/domain.c
>>>>> +++ b/drivers/thunderbolt/domain.c
>>>>> @@ -423,6 +423,7 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize
>>>>> /**
>>>>> * tb_domain_add() - Add domain to the system
>>>>> * @tb: Domain to add
>>>>> + * @reset: Issue reset to the host router
>>>>> *
>>>>> * Starts the domain and adds it to the system. Hotplugging devices will
>>>>> * work after this has been returned successfully. In order to remove
>>>>> @@ -431,7 +432,7 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize
>>>>> *
>>>>> * Return: %0 in case of success and negative errno in case of error
>>>>> */
>>>>> -int tb_domain_add(struct tb *tb)
>>>>> +int tb_domain_add(struct tb *tb, bool reset)
>>>>> {
>>>>> int ret;
>>>>> @@ -460,7 +461,7 @@ int tb_domain_add(struct tb *tb)
>>>>> /* Start the domain */
>>>>> if (tb->cm_ops->start) {
>>>>> - ret = tb->cm_ops->start(tb);
>>>>> + ret = tb->cm_ops->start(tb, reset);
>>>>> if (ret)
>>>>> goto err_domain_del;
>>>>> }
>>>>> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
>>>>> index d8b9c734abd3..623aa81a8833 100644
>>>>> --- a/drivers/thunderbolt/icm.c
>>>>> +++ b/drivers/thunderbolt/icm.c
>>>>> @@ -2144,7 +2144,7 @@ static int icm_runtime_resume(struct tb *tb)
>>>>> return 0;
>>>>> }
>>>>> -static int icm_start(struct tb *tb)
>>>>> +static int icm_start(struct tb *tb, bool not_used)
>>>>> {
>>>>> struct icm *icm = tb_priv(tb);
>>>>> int ret;
>>>>> diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
>>>>> index 633970fbe9b0..63cb4b6afb71 100644
>>>>> --- a/drivers/thunderbolt/lc.c
>>>>> +++ b/drivers/thunderbolt/lc.c
>>>>> @@ -6,6 +6,8 @@
>>>>> * Author: Mika Westerberg <mika.westerberg@...ux.intel.com>
>>>>> */
>>>>> +#include <linux/delay.h>
>>>>> +
>>>>> #include "tb.h"
>>>>> /**
>>>>> @@ -45,6 +47,49 @@ static int find_port_lc_cap(struct tb_port *port)
>>>>> return sw->cap_lc + start + phys * size;
>>>>> }
>>>>> +/**
>>>>> + * tb_lc_reset_port() - Trigger downstream port reset through LC
>>>>> + * @port: Port that is reset
>>>>> + *
>>>>> + * Triggers downstream port reset through link controller registers.
>>>>> + * Returns %0 in case of success negative errno otherwise. Only supports
>>>>> + * non-USB4 routers with link controller (that's Thunderbolt 2 and
>>>>> + * Thunderbolt 3).
>>>>> + */
>>>>> +int tb_lc_reset_port(struct tb_port *port)
>>>>> +{
>>>>> + struct tb_switch *sw = port->sw;
>>>>> + int cap, ret;
>>>>> + u32 mode;
>>>>> +
>>>>> + if (sw->generation < 2)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + cap = find_port_lc_cap(port);
>>>>> + if (cap < 0)
>>>>> + return cap;
>>>>> +
>>>>> + ret = tb_sw_read(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + mode |= TB_LC_PORT_MODE_DPR;
>>>>> +
>>>>> + ret = tb_sw_write(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + fsleep(10000);
>>>>> +
>>>>> + ret = tb_sw_read(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + mode &= ~TB_LC_PORT_MODE_DPR;
>>>>> +
>>>>> + return tb_sw_write(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1);
>>>>> +}
>>>>> +
>>>>> static int tb_lc_set_port_configured(struct tb_port *port, bool configured)
>>>>> {
>>>>> bool upstream = tb_is_upstream_port(port);
>>>>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>>>>> index fb4f46e51753..b22023fae60d 100644
>>>>> --- a/drivers/thunderbolt/nhi.c
>>>>> +++ b/drivers/thunderbolt/nhi.c
>>>>> @@ -1221,7 +1221,7 @@ static void nhi_check_iommu(struct tb_nhi *nhi)
>>>>> str_enabled_disabled(port_ok));
>>>>> }
>>>>> -static void nhi_reset(struct tb_nhi *nhi)
>>>>> +static bool nhi_reset(struct tb_nhi *nhi)
>>>>> {
>>>>> ktime_t timeout;
>>>>> u32 val;
>>>>> @@ -1229,11 +1229,11 @@ static void nhi_reset(struct tb_nhi *nhi)
>>>>> val = ioread32(nhi->iobase + REG_CAPS);
>>>>> /* Reset only v2 and later routers */
>>>>> if (FIELD_GET(REG_CAPS_VERSION_MASK, val) < REG_CAPS_VERSION_2)
>>>>> - return;
>>>>> + return false;
>>>>> if (!host_reset) {
>>>>> dev_dbg(&nhi->pdev->dev, "skipping host router reset\n");
>>>>> - return;
>>>>> + return false;
>>>>> }
>>>>> iowrite32(REG_RESET_HRR, nhi->iobase + REG_RESET);
>>>>> @@ -1244,12 +1244,14 @@ static void nhi_reset(struct tb_nhi *nhi)
>>>>> val = ioread32(nhi->iobase + REG_RESET);
>>>>> if (!(val & REG_RESET_HRR)) {
>>>>> dev_warn(&nhi->pdev->dev, "host router reset successful\n");
>>>>> - return;
>>>>> + return true;
>>>>> }
>>>>> usleep_range(10, 20);
>>>>> } while (ktime_before(ktime_get(), timeout));
>>>>> dev_warn(&nhi->pdev->dev, "timeout resetting host router\n");
>>>>> +
>>>>> + return false;
>>>>> }
>>>>> static int nhi_init_msi(struct tb_nhi *nhi)
>>>>> @@ -1331,6 +1333,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>>> struct device *dev = &pdev->dev;
>>>>> struct tb_nhi *nhi;
>>>>> struct tb *tb;
>>>>> + bool reset;
>>>>> int res;
>>>>> if (!nhi_imr_valid(pdev))
>>>>> @@ -1365,7 +1368,11 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>>> nhi_check_quirks(nhi);
>>>>> nhi_check_iommu(nhi);
>>>>> - nhi_reset(nhi);
>>>>> + /*
>>>>> + * Only USB4 v2 hosts support host reset so if we already did
>>>>> + * that then don't do it again when the domain is initialized.
>>>>> + */
>>>>> + reset = nhi_reset(nhi) ? false : host_reset;
>>>>> res = nhi_init_msi(nhi);
>>>>> if (res)
>>>>> @@ -1392,7 +1399,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>>> dev_dbg(dev, "NHI initialized, starting thunderbolt\n");
>>>>> - res = tb_domain_add(tb);
>>>>> + res = tb_domain_add(tb, reset);
>>>>> if (res) {
>>>>> /*
>>>>> * At this point the RX/TX rings might already have been
>>>>> diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c
>>>>> index 091a81bbdbdc..f760e54cd9bd 100644
>>>>> --- a/drivers/thunderbolt/path.c
>>>>> +++ b/drivers/thunderbolt/path.c
>>>>> @@ -446,6 +446,19 @@ static int __tb_path_deactivate_hop(struct tb_port *port, int hop_index,
>>>>> return -ETIMEDOUT;
>>>>> }
>>>>> +/**
>>>>> + * tb_path_deactivate_hop() - Deactivate one path in path config space
>>>>> + * @port: Lane or protocol adapter
>>>>> + * @hop_index: HopID of the path to be cleared
>>>>> + *
>>>>> + * This deactivates or clears a single path config space entry at
>>>>> + * @hop_index. Returns %0 in success and negative errno otherwise.
>>>>> + */
>>>>> +int tb_path_deactivate_hop(struct tb_port *port, int hop_index)
>>>>> +{
>>>>> + return __tb_path_deactivate_hop(port, hop_index, true);
>>>>> +}
>>>>> +
>>>>> static void __tb_path_deactivate_hops(struct tb_path *path, int first_hop)
>>>>> {
>>>>> int i, res;
>>>>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
>>>>> index 900114ba4371..c4f486629a2b 100644
>>>>> --- a/drivers/thunderbolt/switch.c
>>>>> +++ b/drivers/thunderbolt/switch.c
>>>>> @@ -676,6 +676,13 @@ int tb_port_disable(struct tb_port *port)
>>>>> return __tb_port_enable(port, false);
>>>>> }
>>>>> +static int tb_port_reset(struct tb_port *port)
>>>>> +{
>>>>> + if (tb_switch_is_usb4(port->sw))
>>>>> + return usb4_port_reset(port);
>>>>> + return tb_lc_reset_port(port);
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * tb_init_port() - initialize a port
>>>>> *
>>>>> @@ -1532,20 +1539,64 @@ static void tb_dump_switch(const struct tb *tb, const struct tb_switch *sw)
>>>>> }
>>>>> /**
>>>>> - * tb_switch_reset() - reconfigure route, enable and send TB_CFG_PKG_RESET
>>>>> - * @sw: Switch to reset
>>>>> + * tb_switch_reset() - Perform reset to the router
>>>>> + * @sw: Router to reset
>>>>> *
>>>>> - * Return: Returns 0 on success or an error code on failure.
>>>>> + * Issues reset to the router. Can be used for any router. Returns %0
>>>>> + * on success or an error code on failure.
>>>>> */
>>>>> int tb_switch_reset(struct tb_switch *sw)
>>>>> {
>>>>> struct tb_cfg_result res;
>>>>> - if (sw->generation > 1)
>>>>> - return 0;
>>>>> + tb_sw_dbg(sw, "resetting router\n");
>>>>> +
>>>>> + if (sw->generation > 1) {
>>>>> + struct tb_port *port;
>>>>> +
>>>>> + tb_switch_for_each_port(sw, port) {
>>>>> + int i, ret;
>>>>> +
>>>>> + /*
>>>>> + * For lane adapters we issue downstream port
>>>>> + * reset. That clears up also the path config
>>>>> + * spaces.
>>>>> + *
>>>>> + * For protocol adapters we disable the path and
>>>>> + * clear path config space one by one (from 8 to
>>>>> + * Max Input HopID of the adapter).
>>>>> + */
>>>>> + if (tb_port_is_null(port) && !tb_is_upstream_port(port)) {
>>>>> + ret = tb_port_reset(port);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + continue;
>>>> I had thought in similar lines when you told about reset protocol adapters.
>>>>
>>>> But, here we are traversing through all the ports, what if we get to perform
>>>> the DPR first ?
>>>> and then the PCIe, USB and DP reset. This may cause unplug event before we
>>>> tear down
>>>> protocol adapters and its path configuration. (I may be wrong)
>>> Yeah, it could be that it is better first disable protocol adapters and
>>> then do the DPR or so.
>> I gave it a try doing a DPR later and before disabling protocol adapters.
>> Both the methods didn't work.
>>
>> However, when I do it like below, it works.
>>
>> tb_switch_discover_tunnels(tb->root_switch,
>> &tcm->tunnel_list, false);
>> ret = tb_switch_reset(tb->root_switch);
>>
>> We are missing something here.
> 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
>>>> I'll check the behavior on Monday and update.
>>>>
>>>> Assuming this works, I can incorporate the suggestion and send out v3 with
>>>> appropriate tags ? It can be split into 3 patches.
>>> Sure.
>>>
>>> Bonus points if you can drop some more lines from that :)
>>>
>>>>> + } 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);
>> Here, as per spec it would be better if we first teardown it for DOWN path
>> and then the UP
>> path.
> Right makes sense.
We never get up_port of protocol adapters here for reset. It's always
down_port.
So probably when we discover the path, we do path deactivation for both
down and up ports.
>>>>> + } 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;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> + }
>>>>> + /* Thunderbolt 1 uses the "reset" config space packet */
>>>>> res.err = tb_sw_write(sw, ((u32 *) &sw->config) + 2,
>>>>> TB_CFG_SWITCH, 2, 2);
>>>>> if (res.err)
>>>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>>>>> index 8bc3985df749..5e5e3aebe018 100644
>>>>> --- a/drivers/thunderbolt/tb.c
>>>>> +++ b/drivers/thunderbolt/tb.c
>>>>> @@ -2590,7 +2590,7 @@ static int tb_scan_finalize_switch(struct device *dev, void *data)
>>>>> return 0;
>>>>> }
>>>>> -static int tb_start(struct tb *tb)
>>>>> +static int tb_start(struct tb *tb, bool reset)
>>>>> {
>>>>> struct tb_cm *tcm = tb_priv(tb);
>>>>> int ret;
>>>>> @@ -2631,12 +2631,22 @@ static int tb_start(struct tb *tb)
>>>>> tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_MODE_LOWRES);
>>>>> /* Enable TMU if it is off */
>>>>> tb_switch_tmu_enable(tb->root_switch);
>>>>> - /* Full scan to discover devices added before the driver was loaded. */
>>>>> - tb_scan_switch(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 */
>>>>> - tb_discover_dp_resources(tb);
>>>>> +
>>>>> + if (reset && usb4_switch_version(tb->root_switch) == 1) {
>>>>> + ret = tb_switch_reset(tb->root_switch);
>>>>> + if (ret) {
>>>>> + tb_sw_warn(tb->root_switch, "failed to reset\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>> Ok. So idea is to drop reset for <= TBT3 currently.
>>> Yes, there are some older Apple systems that "benefit" from the
>>> discovery so I would keep it there for them.
>>>
>>>>> } else {
>>>>> + /* Full scan to discover devices added before the driver was loaded. */
>>>>> + tb_scan_switch(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 */
>>>>> + tb_discover_dp_resources(tb);
>>>>> + }
>>>>> +
>>>>> /*
>>>>> * If the boot firmware did not create USB 3.x tunnels create them
>>>>> * now for the whole topology.
>>>>> @@ -2702,7 +2712,7 @@ static int tb_resume_noirq(struct tb *tb)
>>>>> {
>>>>> struct tb_cm *tcm = tb_priv(tb);
>>>>> struct tb_tunnel *tunnel, *n;
>>>>> - unsigned int usb3_delay = 0;
>>>>> + unsigned int usb3_delay;
>>>>> LIST_HEAD(tunnels);
>>>>> tb_dbg(tb, "resuming...\n");
>>>>> @@ -2715,19 +2725,7 @@ static int tb_resume_noirq(struct tb *tb)
>>>>> tb_free_unplugged_children(tb->root_switch);
>>>>> tb_restore_children(tb->root_switch);
>>>>> - /*
>>>>> - * If we get here from suspend to disk the boot firmware or the
>>>>> - * restore kernel might have created tunnels of its own. Since
>>>>> - * we cannot be sure they are usable for us we find and tear
>>>>> - * them down.
>>>>> - */
>>>>> - tb_switch_discover_tunnels(tb->root_switch, &tunnels, false);
>>>>> - list_for_each_entry_safe_reverse(tunnel, n, &tunnels, list) {
>>>>> - if (tb_tunnel_is_usb3(tunnel))
>>>>> - usb3_delay = 500;
>>>>> - tb_tunnel_deactivate(tunnel);
>>>>> - tb_tunnel_free(tunnel);
>>>>> - }
>>>>> + usb3_delay = tb_switch_is_usb4(tb->root_switch) ? 500 : 0;
>>>>> /* Re-create our tunnels now */
>>>>> list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) {
>>>>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>>>>> index 7ad55f5966f3..2bc8eca965ed 100644
>>>>> --- a/drivers/thunderbolt/tb.h
>>>>> +++ b/drivers/thunderbolt/tb.h
>>>>> @@ -489,7 +489,7 @@ struct tb_path {
>>>>> */
>>>>> struct tb_cm_ops {
>>>>> int (*driver_ready)(struct tb *tb);
>>>>> - int (*start)(struct tb *tb);
>>>>> + int (*start)(struct tb *tb, bool reset);
>>>>> void (*stop)(struct tb *tb);
>>>>> int (*suspend_noirq)(struct tb *tb);
>>>>> int (*resume_noirq)(struct tb *tb);
>>>>> @@ -752,7 +752,7 @@ int tb_xdomain_init(void);
>>>>> void tb_xdomain_exit(void);
>>>>> struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize);
>>>>> -int tb_domain_add(struct tb *tb);
>>>>> +int tb_domain_add(struct tb *tb, bool reset);
>>>>> void tb_domain_remove(struct tb *tb);
>>>>> int tb_domain_suspend_noirq(struct tb *tb);
>>>>> int tb_domain_resume_noirq(struct tb *tb);
>>>>> @@ -1156,6 +1156,7 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
>>>>> void tb_path_free(struct tb_path *path);
>>>>> int tb_path_activate(struct tb_path *path);
>>>>> void tb_path_deactivate(struct tb_path *path);
>>>>> +int tb_path_deactivate_hop(struct tb_port *port, int hop_index);
>>>>> bool tb_path_is_invalid(struct tb_path *path);
>>>>> bool tb_path_port_on_path(const struct tb_path *path,
>>>>> const struct tb_port *port);
>>>>> @@ -1175,6 +1176,7 @@ int tb_drom_read(struct tb_switch *sw);
>>>>> int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid);
>>>>> int tb_lc_read_uuid(struct tb_switch *sw, u32 *uuid);
>>>>> +int tb_lc_reset_port(struct tb_port *port);
>>>>> int tb_lc_configure_port(struct tb_port *port);
>>>>> void tb_lc_unconfigure_port(struct tb_port *port);
>>>>> int tb_lc_configure_xdomain(struct tb_port *port);
>>>>> @@ -1307,6 +1309,7 @@ void usb4_switch_remove_ports(struct tb_switch *sw);
>>>>> int usb4_port_unlock(struct tb_port *port);
>>>>> int usb4_port_hotplug_enable(struct tb_port *port);
>>>>> +int usb4_port_reset(struct tb_port *port);
>>>>> int usb4_port_configure(struct tb_port *port);
>>>>> void usb4_port_unconfigure(struct tb_port *port);
>>>>> int usb4_port_configure_xdomain(struct tb_port *port, struct tb_xdomain *xd);
>>>>> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
>>>>> index 87e4795275fe..efcae298b370 100644
>>>>> --- a/drivers/thunderbolt/tb_regs.h
>>>>> +++ b/drivers/thunderbolt/tb_regs.h
>>>>> @@ -389,6 +389,7 @@ struct tb_regs_port_header {
>>>>> #define PORT_CS_18_CSA BIT(22)
>>>>> #define PORT_CS_18_TIP BIT(24)
>>>>> #define PORT_CS_19 0x13
>>>>> +#define PORT_CS_19_DPR BIT(0)
>>>>> #define PORT_CS_19_PC BIT(3)
>>>>> #define PORT_CS_19_PID BIT(4)
>>>>> #define PORT_CS_19_WOC BIT(16)
>>>>> @@ -584,6 +585,9 @@ struct tb_regs_hop {
>>>>> #define TB_LC_POWER 0x740
>>>>> /* Link controller registers */
>>>>> +#define TB_LC_PORT_MODE 0x26
>>>>> +#define TB_LC_PORT_MODE_DPR BIT(0)
>>>>> +
>>>>> #define TB_LC_CS_42 0x2a
>>>>> #define TB_LC_CS_42_USB_PLUGGED BIT(31)
>>>>> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
>>>>> index 675d1ed62372..9e002bf73d2e 100644
>>>>> --- a/drivers/thunderbolt/usb4.c
>>>>> +++ b/drivers/thunderbolt/usb4.c
>>>>> @@ -1107,6 +1107,45 @@ int usb4_port_hotplug_enable(struct tb_port *port)
>>>>> return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
>>>>> }
>>>>> +/**
>>>>> + * usb4_port_reset() - Issue downstream port reset
>>>>> + * @port: USB4 port to reset
>>>>> + *
>>>>> + * Issues downstream port reset to @port.
>>>>> + */
>>>>> +int usb4_port_reset(struct tb_port *port)
>>>>> +{
>>>>> + int ret;
>>>>> + u32 val;
>>>>> +
>>>>> + if (!port->cap_usb4)
>>>>> + return -EINVAL;
>>>>> +
>> We've to drop this check here and check it before calling for that port.
>> Otherwise, I it may throw -EINVAL and results in probe failure.
> Agree.
>
>>>>> + ret = tb_port_read(port, &val, TB_CFG_PORT,
>>>>> + port->cap_usb4 + PORT_CS_19, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + val |= PORT_CS_19_DPR;
>>>>> +
>>>>> + ret = tb_port_write(port, &val, TB_CFG_PORT,
>>>>> + port->cap_usb4 + PORT_CS_19, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + fsleep(10000);
>>>>> +
>>>>> + ret = tb_port_read(port, &val, TB_CFG_PORT,
>>>>> + port->cap_usb4 + PORT_CS_19, 1);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + val &= ~PORT_CS_19_DPR;
>>>>> +
>>>>> + return tb_port_write(port, &val, TB_CFG_PORT,
>>>>> + port->cap_usb4 + PORT_CS_19, 1);
>>>>> +}
>>>>> +
>>>>> static int usb4_port_set_configured(struct tb_port *port, bool configured)
>>>>> {
>>>>> int ret;
>>>>>
>>>>>
Powered by blists - more mailing lists