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:47:03 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mario Limonciello <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 v3] thunderbolt: Explicitly enable lane adapter hotplug
 events at startup
Message-ID: <Yy6n56H09ct76GTJ@black.fi.intel.com>
References: <20220923183944.10746-1-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220923183944.10746-1-mario.limonciello@amd.com>
Precedence: bulk

Hi Mario,

On Fri, Sep 23, 2022 at 01:39:44PM -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>
> ---
> v2->v3:
>  * Guard with tb_switch_is_icm to avoid risk to Intel FW CM case
> v1->v2:
>  * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
>  * Clarify intended users in documentation comment
>  * Only call for lane adapters
>  * Add stable tag
> 
> Note: v2 it was suggested to move this to tb_switch_configure, but
> port->config is not yet read at that time.  This should be the right
> time to call it, so this version of the patch just guards against the
> code running on Intel's controllers that have a FW CM.

Can we put it in a separate function that is called from
tb_switch_add()? I explained in the previous reply (to v2) that the
tb_init_port() is pretty much just reading stuff and therefore I would
not like to add there anything that does writing.

While at it, the USB4 v2 spec (not yet public but can be found from the
working group site) adds a CM requirement that forbids writing lane 1
adapter configuration registers so in addition to that please check
port->cap_usb4 there so we know we deal with the lane 0 adapter only (as
->cap_usb4 is only set for the lane 0 adapters).

So something like this:

static void tb_switch_port_hotplug_enable(struct tb_switch *sw)
{
	if (tb_switch_is_icm(sw))
		return;

	tb_switch_for_each_port(sw, port) {
		if (!port->cap_usb4)
			continue;

		// here enable the hotplug
	}
}

int tb_switch_add(struct tb_switch *sw)
{
	... // init ports and stuff happens here

	tb_switch_default_link_ports(sw);
	tb_switch_port_hotplug_enable(sw);
}

(we should probably create a new macro tb_switch_for_each_usb4_port()
that just enumerates all USB4 ports).