LinuxLists.cc - [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

2022-09-22 16:18:12

Subject: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

Software that has run before the USB4 CM in Linux runs may have disabled
hotplug events for a given lane adapter.

Other CMs such as that one distributed with Windows 11 will enable hotplug
events. Do the same thing in the Linux CM which fixes hotplug events on
"AMD Pink Sardine".

Cc: [email protected]
Signed-off-by: Mario Limonciello <[email protected]>
---
v1->v2:
* Only send second patch as first was merged already
* s/usb4_enable_hotplug/usb4_port_hotplug_enable/
* Clarify intended users in documentation comment
* Only call for lane adapters
* Add stable tag

drivers/thunderbolt/switch.c | 4 ++++
drivers/thunderbolt/tb.h | 1 +
drivers/thunderbolt/tb_regs.h | 1 +
drivers/thunderbolt/usb4.c | 20 ++++++++++++++++++++
4 files changed, 26 insertions(+)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 77d7f07ca075..3213239d12c8 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)

if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
port->ctl_credits = hop.initial_credits;
+
+ res = usb4_port_hotplug_enable(port);
+ if (res)
+ return res;
}
if (!port->ctl_credits)
port->ctl_credits = 2;
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 5db76de40cc1..332159f984fc 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw);
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_configure(struct tb_port *port);
void usb4_port_unconfigure(struct tb_port *port);
int usb4_port_configure_xdomain(struct tb_port *port);
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 166054110388..bbe38b2d9057 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -308,6 +308,7 @@ struct tb_regs_port_header {
#define ADP_CS_5 0x05
#define ADP_CS_5_LCA_MASK GENMASK(28, 22)
#define ADP_CS_5_LCA_SHIFT 22
+#define ADP_CS_5_DHP BIT(31)

/* TMU adapter registers */
#define TMU_ADP_CS_3 0x03
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index 3a2e7126db9d..f0b5a8f1ed3a 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port)
return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
}

+/**
+ * usb4_port_hotplug_enable() - Enables hotplug for a port
+ * @port: USB4 port to operate on
+ *
+ * Enables hot plug events on a given port. This is only intended
+ * to be used on lane, DP-IN, and DP-OUT adapters.
+ */
+int usb4_port_hotplug_enable(struct tb_port *port)
+{
+ int ret;
+ u32 val;
+
+ ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
+ if (ret)
+ return ret;
+
+ val &= ~ADP_CS_5_DHP;
+ return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
+}
+
static int usb4_port_set_configured(struct tb_port *port, bool configured)
{
int ret;
--
2.34.1

2022-09-22 16:31:13

by Mario Limonciello

[permalink] [raw]

Subject: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

2022-09-23 09:59:45

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

Hi Mario,

On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote:
> Software that has run before the USB4 CM in Linux runs may have disabled
> hotplug events for a given lane adapter.
>
> Other CMs such as that one distributed with Windows 11 will enable hotplug
> events. Do the same thing in the Linux CM which fixes hotplug events on
> "AMD Pink Sardine".
>
> Cc: [email protected]
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> v1->v2:
> * Only send second patch as first was merged already
> * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
> * Clarify intended users in documentation comment
> * Only call for lane adapters
> * Add stable tag
>
> drivers/thunderbolt/switch.c | 4 ++++
> drivers/thunderbolt/tb.h | 1 +
> drivers/thunderbolt/tb_regs.h | 1 +
> drivers/thunderbolt/usb4.c | 20 ++++++++++++++++++++
> 4 files changed, 26 insertions(+)
>
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 77d7f07ca075..3213239d12c8 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)
>
> if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
> port->ctl_credits = hop.initial_credits;
> +
> + res = usb4_port_hotplug_enable(port);
> + if (res)

I think this does not belong here in tb_init_port(). This is called from
both FW and SW CM paths and we don't want to confuse the FW CM more than
necessary ;-)

So instead I think this should be added to tb_plug_events_active().

Otherwise looks good to me.

> + return res;
> }
> if (!port->ctl_credits)
> port->ctl_credits = 2;
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 5db76de40cc1..332159f984fc 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw);
> 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_configure(struct tb_port *port);
> void usb4_port_unconfigure(struct tb_port *port);
> int usb4_port_configure_xdomain(struct tb_port *port);
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index 166054110388..bbe38b2d9057 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -308,6 +308,7 @@ struct tb_regs_port_header {
> #define ADP_CS_5 0x05
> #define ADP_CS_5_LCA_MASK GENMASK(28, 22)
> #define ADP_CS_5_LCA_SHIFT 22
> +#define ADP_CS_5_DHP BIT(31)
>
> /* TMU adapter registers */
> #define TMU_ADP_CS_3 0x03
> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
> index 3a2e7126db9d..f0b5a8f1ed3a 100644
> --- a/drivers/thunderbolt/usb4.c
> +++ b/drivers/thunderbolt/usb4.c
> @@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port)
> return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
> }
>
> +/**
> + * usb4_port_hotplug_enable() - Enables hotplug for a port
> + * @port: USB4 port to operate on
> + *
> + * Enables hot plug events on a given port. This is only intended
> + * to be used on lane, DP-IN, and DP-OUT adapters.
> + */
> +int usb4_port_hotplug_enable(struct tb_port *port)
> +{
> + int ret;
> + u32 val;
> +
> + ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
> + if (ret)
> + return ret;
> +
> + val &= ~ADP_CS_5_DHP;
> + return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
> +}
> +
> static int usb4_port_set_configured(struct tb_port *port, bool configured)
> {
> int ret;
> --
> 2.34.1

2022-09-23 16:38:38

by Mario Limonciello

[permalink] [raw]

Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

On 9/23/2022 04:16, Mika Westerberg wrote:
> Hi Mario,
>
> On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote:
>> Software that has run before the USB4 CM in Linux runs may have disabled
>> hotplug events for a given lane adapter.
>>
>> Other CMs such as that one distributed with Windows 11 will enable hotplug
>> events. Do the same thing in the Linux CM which fixes hotplug events on
>> "AMD Pink Sardine".
>>
>> Cc: [email protected]
>> Signed-off-by: Mario Limonciello <[email protected]>
>> ---
>> v1->v2:
>> * Only send second patch as first was merged already
>> * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
>> * Clarify intended users in documentation comment
>> * Only call for lane adapters
>> * Add stable tag
>>
>> drivers/thunderbolt/switch.c | 4 ++++
>> drivers/thunderbolt/tb.h | 1 +
>> drivers/thunderbolt/tb_regs.h | 1 +
>> drivers/thunderbolt/usb4.c | 20 ++++++++++++++++++++
>> 4 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
>> index 77d7f07ca075..3213239d12c8 100644
>> --- a/drivers/thunderbolt/switch.c
>> +++ b/drivers/thunderbolt/switch.c
>> @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)
>>
>> if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
>> port->ctl_credits = hop.initial_credits;
>> +
>> + res = usb4_port_hotplug_enable(port);
>> + if (res)
>
> I think this does not belong here in tb_init_port(). This is called from
> both FW and SW CM paths and we don't want to confuse the FW CM more than
> necessary ;-)
>
> So instead I think this should be added to tb_plug_events_active().
>

The problem with that location is that tb_plug_events_active() is called
from tb_switch_configure() which is before tb_switch_add() is called.
tb_switch_add() calls tb_init_port() which reads port->config for the
first time.

So if this is only to be called in tb_switch_configure() it means
reading port->config "earlier" too.

So it definitely needs to be called in tb_init_port() or a later
function but before the device is announced to satisfy only running on
the appropriate port types.

tb_init_port() or tb_switch_add feels like the right place to me. How
about leaving it where it is but guarding with a "if
(!tb_switch_is_icm())" to avoid the risk to the FW CM case?

> Otherwise looks good to me.
>
>> + return res;
>> }
>> if (!port->ctl_credits)
>> port->ctl_credits = 2;
>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>> index 5db76de40cc1..332159f984fc 100644
>> --- a/drivers/thunderbolt/tb.h
>> +++ b/drivers/thunderbolt/tb.h
>> @@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw);
>> 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_configure(struct tb_port *port);
>> void usb4_port_unconfigure(struct tb_port *port);
>> int usb4_port_configure_xdomain(struct tb_port *port);
>> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
>> index 166054110388..bbe38b2d9057 100644
>> --- a/drivers/thunderbolt/tb_regs.h
>> +++ b/drivers/thunderbolt/tb_regs.h
>> @@ -308,6 +308,7 @@ struct tb_regs_port_header {
>> #define ADP_CS_5 0x05
>> #define ADP_CS_5_LCA_MASK GENMASK(28, 22)
>> #define ADP_CS_5_LCA_SHIFT 22
>> +#define ADP_CS_5_DHP BIT(31)
>>
>> /* TMU adapter registers */
>> #define TMU_ADP_CS_3 0x03
>> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
>> index 3a2e7126db9d..f0b5a8f1ed3a 100644
>> --- a/drivers/thunderbolt/usb4.c
>> +++ b/drivers/thunderbolt/usb4.c
>> @@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port)
>> return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
>> }
>>
>> +/**
>> + * usb4_port_hotplug_enable() - Enables hotplug for a port
>> + * @port: USB4 port to operate on
>> + *
>> + * Enables hot plug events on a given port. This is only intended
>> + * to be used on lane, DP-IN, and DP-OUT adapters.
>> + */
>> +int usb4_port_hotplug_enable(struct tb_port *port)
>> +{
>> + int ret;
>> + u32 val;
>> +
>> + ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
>> + if (ret)
>> + return ret;
>> +
>> + val &= ~ADP_CS_5_DHP;
>> + return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
>> +}
>> +
>> static int usb4_port_set_configured(struct tb_port *port, bool configured)
>> {
>> int ret;
>> --
>> 2.34.1

2022-09-24 06:06:55

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup

Hi,

On Fri, Sep 23, 2022 at 11:08:30AM -0500, Limonciello, Mario wrote:
> On 9/23/2022 04:16, Mika Westerberg wrote:
> > Hi Mario,
> >
> > On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote:
> > > Software that has run before the USB4 CM in Linux runs may have disabled
> > > hotplug events for a given lane adapter.
> > >
> > > Other CMs such as that one distributed with Windows 11 will enable hotplug
> > > events. Do the same thing in the Linux CM which fixes hotplug events on
> > > "AMD Pink Sardine".
> > >
> > > Cc: [email protected]
> > > Signed-off-by: Mario Limonciello <[email protected]>
> > > ---
> > > v1->v2:
> > > * Only send second patch as first was merged already
> > > * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
> > > * Clarify intended users in documentation comment
> > > * Only call for lane adapters
> > > * Add stable tag
> > >
> > > drivers/thunderbolt/switch.c | 4 ++++
> > > drivers/thunderbolt/tb.h | 1 +
> > > drivers/thunderbolt/tb_regs.h | 1 +
> > > drivers/thunderbolt/usb4.c | 20 ++++++++++++++++++++
> > > 4 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> > > index 77d7f07ca075..3213239d12c8 100644
> > > --- a/drivers/thunderbolt/switch.c
> > > +++ b/drivers/thunderbolt/switch.c
> > > @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)
> > > if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
> > > port->ctl_credits = hop.initial_credits;
> > > +
> > > + res = usb4_port_hotplug_enable(port);
> > > + if (res)
> >
> > I think this does not belong here in tb_init_port(). This is called from
> > both FW and SW CM paths and we don't want to confuse the FW CM more than
> > necessary ;-)
> >
> > So instead I think this should be added to tb_plug_events_active().
> >
>
> The problem with that location is that tb_plug_events_active() is called
> from tb_switch_configure() which is before tb_switch_add() is called.
> tb_switch_add() calls tb_init_port() which reads port->config for the first
> time.

Ah indeed, I missed that.

> So if this is only to be called in tb_switch_configure() it means reading
> port->config "earlier" too.
>
> So it definitely needs to be called in tb_init_port() or a later function
> but before the device is announced to satisfy only running on the
> appropriate port types.
>
> tb_init_port() or tb_switch_add feels like the right place to me. How about
> leaving it where it is but guarding with a "if (!tb_switch_is_icm())" to
> avoid the risk to the FW CM case?

What about adding a new function that does this and it called from
tb_switch_add() before announcing devices to the world? tb_init_port()
is pretty much reading stuff from adapters so I would not like to add
there anything that does writing if possible.