Date: Thu, 10 Apr 2025 07:27:23 +0300
From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Mario Limonciello <superm1@kernel.org>
Cc: Raul Rangel <rrangel@chromium.org>,
	Opal Voravootivat <puthik@google.com>,
	"open list:USB XHCI DRIVER" <linux-usb@vger.kernel.org>,
	"Gong, Richard" <Richard.Gong@amd.com>,
	Utkarsh Patel <utkarsh.h.patel@intel.com>
Subject: Re: Wake on connect / wake on disconnect
Message-ID: <20250410042723.GU3152277@black.fi.intel.com>
References: <20250404060203.GM3152277@black.fi.intel.com>
 <3b283147-cd32-410e-a163-9e603fd40bc1@gmail.com>
 <20250404115346.GN3152277@black.fi.intel.com>
 <98539971-3b43-4d64-a105-6790198c46d1@amd.com>
 <20250404161008.GO3152277@black.fi.intel.com>
 <f9806a24-875f-4c44-9694-96b62a6aefd9@gmail.com>
 <5f9004aa-9b51-474d-9cb0-c8c4e23b19f4@kernel.org>
 <CAHQZ30C=7d1DkHYTixrgmArCwKHzhxBn602P=YY5rP6OcAXuVw@mail.gmail.com>
 <20250407055520.GP3152277@black.fi.intel.com>
 <dcf41124-d693-4b0f-a1d1-7ad7cd914449@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <dcf41124-d693-4b0f-a1d1-7ad7cd914449@kernel.org>

On Wed, Apr 09, 2025 at 12:03:52PM -0500, Mario Limonciello wrote:
> On 4/7/2025 12:55 AM, Mika Westerberg wrote:
> > On Fri, Apr 04, 2025 at 10:55:35AM -0600, Raul Rangel wrote:
> > > On Fri, Apr 4, 2025 at 10:20 AM Mario Limonciello <superm1@kernel.org> wrote:
> > > > 
> > > > 
> > > > 
> > > > On 4/4/25 11:16, Mario Limonciello wrote:
> > > > > 
> > > > > 
> > > > > On 4/4/25 11:10, Mika Westerberg wrote:
> > > > > > On Fri, Apr 04, 2025 at 10:03:18AM -0500, Mario Limonciello wrote:
> > > > > > > On 4/4/2025 6:53 AM, Mika Westerberg wrote:
> > > > > > > > On Fri, Apr 04, 2025 at 06:47:31AM -0500, Mario Limonciello wrote:
> > > > > > > > > On 4/4/25 01:02, Mika Westerberg wrote:
> > > > > > > > > > Hi Mario,
> > > > > > > > > > 
> > > > > > > > > > On Thu, Apr 03, 2025 at 01:12:07PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > Mika,
> > > > > > > > > > > 
> > > > > > > > > > > Recently there are some conversations about wake-up from connect/
> > > > > > > > > > > disconnect
> > > > > > > > > > > happening and I wanted to get some background from you about the
> > > > > > > > > > > current
> > > > > > > > > > > policy set in tb_switch_suspend().
> > > > > > > > > > > 
> > > > > > > > > > > Wake on connect and disconnect are only used for runtime, not for
> > > > > > > > > > > system
> > > > > > > > > > > suspend.  Would you be open to adding wake on connect as well for
> > > > > > > > > > > system
> > > > > > > > > > > suspend?  This should help enable use cases like plugging in a
> > > > > > > > > > > closed laptop
> > > > > > > > > > > to a dock (which works on Windows).
> > > > > > > > > > 
> > > > > > > > > > Don't we already have a similar for all usb4_portX devices? That
> > > > > > > > > > does not
> > > > > > > > > > work for you?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I think that will functionally work - but I'll double check.
> > > > > > > > > 
> > > > > > > > > However usb_portX power/wakeup defaults are 'disabled' so this
> > > > > > > > > would need a
> > > > > > > > > udev rule.  Could we set the kernel default for those to 'enabled'
> > > > > > > > > instead?
> > > > > > > > 
> > > > > > > > No because that would trigger wakeup each time you unplug/plug and
> > > > > > > > this is
> > > > > > > > certainly not good if you close the lid, unplug from dock and throw the
> > > > > > > > laptop to your backpack. Chrome uses this with "dark resume" so if
> > > > > > > > this is
> > > > > > > > supported by the userspace then it can also enable these.
> > > > > > > 
> > > > > > > Ahhh.  I was thinking specifically with wake on connect that's not a
> > > > > > > problem, but the sysfs knob for the port changes both wake on connect
> > > > > > > and
> > > > > > > wake on disconnect.
> > > > > > > 
> > > > > > > Is there actually a use case for chrome with wake on disconnect?  IE
> > > > > > > if we
> > > > > > > didn't turn on wake on disconnect but defaulted to wake on connect would
> > > > > > > things be OK?  Or made the sysfs knob control only wake on disconnect?
> > > > > > 
> > > > > > Good guestion - I don't know ;-) The Chrome folks wanted this so I
> > > > > > suppose
> > > > > > their usecase is specifically that dark resume and I think that's when
> > > > > > you
> > > > > > unplug a device so disconnect. Not so sure about wake on connect.
> > > > > 
> > > > > I found the original patch from Rajat [1].
> > > > > 
> > > > > Rajat, any comments?  Could you loop in the right people from the Chrome
> > > > > side to ask?    I think my "preference" would be that we make "wake on
> > > > > connect" always enabled and then let the sysfs knob control "wake on
> > > > > disconnect".
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-usb/20221101115042.248187-1-
> > > > > rajat.khandelwal@intel.com/
> > > > 
> > > > Oh Rajat's email bounced.  The only person I know that I've worked on
> > > > wakeup related stuff is Raul.  I'll add him.
> > > > 
> > > > Mika, Raul,
> > > > 
> > > > Could you pull in current Chrome people from Intel and Google that could
> > > > comment here?
> > > 
> > > + Opal who should be able to answer the question regarding wake on
> > > connect/disconnect.
> > 
> > Added Utkarsh from Intel side.
> 
> I had another look at usb4_port.c the flows used.  This is the call path:
> 
> tb_switch_suspend()
> ->tb_switch_set_wake()
> ->->usb4_switch_set_wake()
> 
> The flags for wakeup policy are set in tb_switch_suspend() and then passed
> as arguments down to usb4_switch_set_wake().  This enumerates whether wake
> is set for any usb4_port device and applies the flags to that device.

Indeed. So this actually never worked.

> So AFAICT that means that even on ChromeOS there won't be a wake on connect
> or wake on disconnect event for suspend/resume no matter what the sysfs
> files for each port are set to.
> 
> In summary; my ask is whether we can do this:
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 6a2116cbb06f9..f2f6a085a742b 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -3599,6 +3599,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool
> runtime)
>                 flags |= TB_WAKE_ON_USB4;
>                 flags |= TB_WAKE_ON_USB3 | TB_WAKE_ON_PCIE | TB_WAKE_ON_DP;
>         } else if (device_may_wakeup(&sw->dev)) {
> +               flags |= TB_WAKE_ON_CONNECT;
>                 flags |= TB_WAKE_ON_USB4 | TB_WAKE_ON_USB3 |
> TB_WAKE_ON_PCIE;
>         }
> 
> That would allow the use case of "plug in a closed laptop to a dock" and it
> wakes up.

I think we should set both here. Someone will want the unplug to behave the
same and at that point it is hard to change anymore to avoid breaking
things. Then it is up to the userspace to enable per USB4 port, and deal
with (e.g dark resume).