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

[Limonciello, Mario]

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: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > 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