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

[Mika Westerberg]

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: 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.

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.