Re: [PATCH v2] thunderbolt: Fix wake on connect at runtime

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/19/2025 10:24 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> commit 1a760d10ded37 ("thunderbolt: Fix a logic error in wake on connect")
> fixated on the USB4 port sysfs wakeup file not working properly to control
> policy, but it had an unintended side effect that the sysfs file controls
> policy both at runtime and at suspend time. The sysfs file is supposed to
> only control behavior while system is suspended.
> 
> Pass whether programming a port for runtime into usb4_switch_set_wake()
> and if runtime then ignore the value in the sysfs file.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Alexander Kovacs <Alexander.Kovacs@amd.com>
> Tested-by: Alexander Kovacs <Alexander.Kovacs@amd.com>
> Fixes: 1a760d10ded37 ("thunderbolt: Fix a logic error in wake on connect")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>   * Fix kdoc issue reported by lkp robot
> ---
>   drivers/thunderbolt/switch.c |  8 ++++----
>   drivers/thunderbolt/tb.h     |  2 +-
>   drivers/thunderbolt/usb4.c   | 10 +++++-----
>   3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 28febb95f8fa1..e9809fb57c354 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -3437,7 +3437,7 @@ void tb_sw_set_unplugged(struct tb_switch *sw)
>   	}
>   }
>   
> -static int tb_switch_set_wake(struct tb_switch *sw, unsigned int flags)
> +static int tb_switch_set_wake(struct tb_switch *sw, unsigned int flags, bool runtime)
>   {
>   	if (flags)
>   		tb_sw_dbg(sw, "enabling wakeup: %#x\n", flags);
> @@ -3445,7 +3445,7 @@ static int tb_switch_set_wake(struct tb_switch *sw, unsigned int flags)
>   		tb_sw_dbg(sw, "disabling wakeup\n");
>   
>   	if (tb_switch_is_usb4(sw))
> -		return usb4_switch_set_wake(sw, flags);
> +		return usb4_switch_set_wake(sw, flags, runtime);
>   	return tb_lc_set_wake(sw, flags);
>   }
>   
> @@ -3521,7 +3521,7 @@ int tb_switch_resume(struct tb_switch *sw, bool runtime)
>   		tb_switch_check_wakes(sw);
>   
>   	/* Disable wakes */
> -	tb_switch_set_wake(sw, 0);
> +	tb_switch_set_wake(sw, 0, true);
>   
>   	err = tb_switch_tmu_init(sw);
>   	if (err)
> @@ -3603,7 +3603,7 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
>   		flags |= TB_WAKE_ON_USB4 | TB_WAKE_ON_USB3 | TB_WAKE_ON_PCIE;
>   	}
>   
> -	tb_switch_set_wake(sw, flags);
> +	tb_switch_set_wake(sw, flags, runtime);
>   
>   	if (tb_switch_is_usb4(sw))
>   		usb4_switch_set_sleep(sw);
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 87afd5a7c504b..f503bad864130 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1317,7 +1317,7 @@ int usb4_switch_read_uid(struct tb_switch *sw, u64 *uid);
>   int usb4_switch_drom_read(struct tb_switch *sw, unsigned int address, void *buf,
>   			  size_t size);
>   bool usb4_switch_lane_bonding_possible(struct tb_switch *sw);
> -int usb4_switch_set_wake(struct tb_switch *sw, unsigned int flags);
> +int usb4_switch_set_wake(struct tb_switch *sw, unsigned int flags, bool runtime);
>   int usb4_switch_set_sleep(struct tb_switch *sw);
>   int usb4_switch_nvm_sector_size(struct tb_switch *sw);
>   int usb4_switch_nvm_read(struct tb_switch *sw, unsigned int address, void *buf,
> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
> index fce3c0f2354a7..d46d9434933c4 100644
> --- a/drivers/thunderbolt/usb4.c
> +++ b/drivers/thunderbolt/usb4.c
> @@ -403,10 +403,11 @@ bool usb4_switch_lane_bonding_possible(struct tb_switch *sw)
>    * usb4_switch_set_wake() - Enabled/disable wake
>    * @sw: USB4 router
>    * @flags: Wakeup flags (%0 to disable)
> + * @runtime: Wake is being programmed during system runtime
>    *
>    * Enables/disables router to wake up from sleep.
>    */
> -int usb4_switch_set_wake(struct tb_switch *sw, unsigned int flags)
> +int usb4_switch_set_wake(struct tb_switch *sw, unsigned int flags, bool runtime)
>   {
>   	struct usb4_port *usb4;
>   	struct tb_port *port;
> @@ -438,13 +439,12 @@ int usb4_switch_set_wake(struct tb_switch *sw, unsigned int flags)
>   			val |= PORT_CS_19_WOU4;
>   		} else {
>   			bool configured = val & PORT_CS_19_PC;
> +			bool wakeup = runtime || device_may_wakeup(&usb4->dev);
>   			usb4 = port->usb4;

Mistake here; at suspend this is causing a problem since the pointer 
isn't assigned 🤦.

I'll follow up with a v3, sorry for the noise.

>   
> -			if (((flags & TB_WAKE_ON_CONNECT) &&
> -			      device_may_wakeup(&usb4->dev)) && !configured)
> +			if ((flags & TB_WAKE_ON_CONNECT) && wakeup && !configured)
>   				val |= PORT_CS_19_WOC;
> -			if (((flags & TB_WAKE_ON_DISCONNECT) &&
> -			      device_may_wakeup(&usb4->dev)) && configured)
> +			if ((flags & TB_WAKE_ON_DISCONNECT) && wakeup && configured)
>   				val |= PORT_CS_19_WOD;
>   			if ((flags & TB_WAKE_ON_USB4) && configured)
>   				val |= PORT_CS_19_WOU4;