| 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: <efb152bc-fd17-a374-4303-20aa9bde698d@amd.com>
Date: Thu, 23 Nov 2023 10:46:27 +0530
From: Sanath S <sanaths2@....com>
To: Mario Limonciello <mario.limonciello@....com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Sanath S <Sanath.S@....com>
Cc: andreas.noever@...il.com, michael.jamet@...el.com,
YehezkelShB@...il.com, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [Patch v2] thunderbolt: Add quirk to reset downstream port
On 11/22/2023 9:13 PM, Mario Limonciello wrote:
> On 11/22/2023 00:03, Mika Westerberg wrote:
>> Hi,
>>
>> On Wed, Nov 22, 2023 at 10:36:39AM +0530, Sanath S wrote:
>>> Boot firmware on AMD's Yellow Carp and Pink Sardine allocates
>>> very minimal buses for PCIe downstream ports. This results in
>>> failure to extend the daisy chain.
>>>
>>> Add quirk to reset the downstream port to help reset the topology
>>> created by boot firmware.
>>
>> But this resets the USB4 side of ports, how does this help with the PCIe
>> side? Or this also resets the PCIe side? Please add this information to
>> the changelog too.
>
Sure, I'll add the PCIe side reset in changelog.
> IIUC the PCIe side will be implicitly reset as well.
>
>>
>> I suppose it is not possible to fix the boot firmware?
>
> It's a really difficult case to make with firmware team. Windows and
> Linux have a different behavior here. The Windows CM doesn't take the
> existing tunnels from firmware and instead always resets them.
> So Windows "isn't affected" by this problem.
>
> Furthermore there are already lots of systems out "in the wild" as
> these are already both production silicon with shipping OEM products.
>
>>
>>> Suggested-by: Mario Limonciello <mario.limonciello@....com>
>>> Signed-off-by: Sanath S <Sanath.S@....com>
>>> Fixes: e390909ac763 ("thunderbolt: Add vendor specific NHI quirk for
>>> auto-clearing interrupt status")
>>> Cc: <stable@...r.kernel.org>
>>> ---
>>> Changes since v1:
>>> - Initialize ret variable to avoid compiler warning.
>>> - Add Fixes tag with commit id.
>>> ---
>>>
>>> drivers/thunderbolt/quirks.c | 14 ++++++++++++++
>>> drivers/thunderbolt/switch.c | 28 ++++++++++++++++++++++++++++
>>> drivers/thunderbolt/tb.h | 2 ++
>>> drivers/thunderbolt/tb_regs.h | 1 +
>>> 4 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/thunderbolt/quirks.c
>>> b/drivers/thunderbolt/quirks.c
>>> index e6bfa63b40ae..45e9d6c43e4a 100644
>>> --- a/drivers/thunderbolt/quirks.c
>>> +++ b/drivers/thunderbolt/quirks.c
>>> @@ -27,6 +27,12 @@ static void quirk_clx_disable(struct tb_switch *sw)
>>> tb_sw_dbg(sw, "disabling CL states\n");
>>> }
>>> +static void quirk_amd_downstream_port_reset(struct tb_switch *sw)
>>> +{
>>> + sw->quirks |= QUIRK_DPR;
>>> + tb_sw_dbg(sw, "Resetting Down Stream Port\n");
>>
>> That's "resetting downstream ports\n"
>>
Ack, Will take care in v3.
>>> +}
>>> +
>>> static void quirk_usb3_maximum_bandwidth(struct tb_switch *sw)
>>> {
>>> struct tb_port *port;
>>> @@ -93,6 +99,14 @@ static const struct tb_quirk tb_quirks[] = {
>>> { 0x0438, 0x0209, 0x0000, 0x0000, quirk_clx_disable },
>>> { 0x0438, 0x020a, 0x0000, 0x0000, quirk_clx_disable },
>>> { 0x0438, 0x020b, 0x0000, 0x0000, quirk_clx_disable },
>>> + /*
>>> + * Reset Down Stream Ports on AMD USB4 Yellow Carp and
>>> + * Pink Sardine platforms.
>>> + */
>>> + { 0x0438, 0x0208, 0x0000, 0x0000,
>>> quirk_amd_downstream_port_reset },
>>> + { 0x0438, 0x0209, 0x0000, 0x0000,
>>> quirk_amd_downstream_port_reset },
>>> + { 0x0438, 0x020a, 0x0000, 0x0000,
>>> quirk_amd_downstream_port_reset },
>>> + { 0x0438, 0x020b, 0x0000, 0x0000,
>>> quirk_amd_downstream_port_reset },
>>> };
>>> /**
>>> diff --git a/drivers/thunderbolt/switch.c
>>> b/drivers/thunderbolt/switch.c
>>> index 1e15ffa79295..1c4b1dd5f472 100644
>>> --- a/drivers/thunderbolt/switch.c
>>> +++ b/drivers/thunderbolt/switch.c
>>> @@ -1547,6 +1547,23 @@ static void tb_dump_switch(const struct tb
>>> *tb, const struct tb_switch *sw)
>>> regs->__unknown1, regs->__unknown4);
>>> }
>>> +static int tb_switch_reset_downstream_port(struct tb_switch *sw)
>>> +{
>>> + struct tb_port *port;
>>> + uint32_t val = 0;
>>
>> u32
>>
Ack.
>>> + int ret = -1;
>>
>> What is -1? Please use proper error codes.
>>
Ack, It'll be ret = -ENODEV;
>>> +
>>> + tb_switch_for_each_port(sw, port) {
>>> + if (port->config.type == TB_TYPE_PORT) {
>>
>> You mean
>>
>> tb_port_is_null()
>>
>> also please make it a separate function, tb_port_reset() following the
>> similar tb_port_unlock() and friends. With the matching kernel-doc and
>> everything.
>>
Sure, I'll handle this and send a v3.
Will also take case of 10ms delay as per spec.
>>> + val = val | PORT_CS_19_DPR;
>>> + ret = tb_port_write(port, &val, TB_CFG_PORT,
>>> + port->cap_usb4 + PORT_CS_19, 1);
>>
>> Since it is using cap_usb4 you probably need to make usb4_port_reset()
>> as well that gets called from tb_port_reset() (try to make it as simple
>> as possible though).
>>
>>> + break;
>>
>> It is OK just to reset one port?
>>
As per spec, setting the DPR bit of downstream port would help us
reconfigure
the USB4 link, So had a condition check only for downstream port.
>>> + }
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> /**
>>> * tb_switch_reset() - reconfigure route, enable and send
>>> TB_CFG_PKG_RESET
>>> * @sw: Switch to reset
>>> @@ -3201,6 +3218,17 @@ int tb_switch_add(struct tb_switch *sw)
>>> return ret;
>>> }
>>> + /*
>>> + * PCIe resource allocated by boot firmware is not utilizing
>>> all the
>>> + * available buses, So perform reset of topology to avoid
>>> failure in
>>> + * extending daisy chain.
>>> + */
>>
>> This comment should be inside the quirk, not here.
>>
Sure, I'll remove this comment as it is already mentioned in tb_quirks[]
>>> + if (sw->quirks & QUIRK_DPR) {
>>> + ret = tb_switch_reset_downstream_port(sw);
>>
>> And the name of the function should be tb_switch_reset_ports() or so.
>>
Ack.
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> ret = tb_switch_port_hotplug_enable(sw);
>>> if (ret)
>>> return ret;
>>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>>> index e299e53473ae..7a9ff53be67a 100644
>>> --- a/drivers/thunderbolt/tb.h
>>> +++ b/drivers/thunderbolt/tb.h
>>> @@ -23,6 +23,8 @@
>>> #define QUIRK_FORCE_POWER_LINK_CONTROLLER BIT(0)
>>> /* Disable CLx if not supported */
>>> #define QUIRK_NO_CLX BIT(1)
>>> +/* Reset Down Stream Port */
>>> +#define QUIRK_DPR BIT(2)
>>> /**
>>> * struct tb_nvm - Structure holding NVM information
>>> diff --git a/drivers/thunderbolt/tb_regs.h
>>> b/drivers/thunderbolt/tb_regs.h
>>> index 87e4795275fe..d49530bc0d53 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)
>>> --
>>> 2.34.1
>
Powered by blists - more mailing lists