Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 24 Sep 2022 09:04:11 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     "Limonciello, Mario" <mario.limonciello@amd.com>
Cc:     Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        Mehta Sanju <Sanju.Mehta@amd.com>, stable@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug
 events at startup
Message-ID: <Yy6d233SON9qonHa@black.fi.intel.com>
References: <20220922160730.898-1-mario.limonciello@amd.com>
 <Yy15gKzHyMcitY/N@black.fi.intel.com>
 <7c0623f8-5274-c7ee-71a7-3e0fab918f97@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7c0623f8-5274-c7ee-71a7-3e0fab918f97@amd.com>
Precedence: bulk

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@vger.kernel.org
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > 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.