Re: [PATCH 1/6] thunderbolt: Runtime PM activate both ends of the device link

From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: linux-usb@vger.kernel.org,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Yehezkel Bernat <YehezkelShB@gmail.com>,
	Michael Jamet <michael.jamet@intel.com>,
	Gil Fine <gil.fine@intel.com>, Lukas Wunner <lukas@wunner.de>,
	Andreas Noever <andreas.noever@gmail.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH 1/6] thunderbolt: Runtime PM activate both ends of the device link
Date: Fri, 26 Nov 2021 19:03:26 +0100	[thread overview]
Message-ID: <4690965.GXAFRqVoOG@kreacher> (raw)
In-Reply-To: <20211125073733.74902-2-mika.westerberg@linux.intel.com>

On Thursday, November 25, 2021 8:37:28 AM CET Mika Westerberg wrote:
> If protocol tunnels are already up when the driver is loaded, for
> instance if the boot firmware implements connection manager of its own,
> runtime PM reference count of the consumer devices behind the tunnel
> might have been increased already before the device link is created but
> the supplier device runtime PM reference count is not. This leads to a
> situation where the supplier (the Thunderbolt driver) can runtime
> suspend even if it should not because the corresponding protocol tunnel
> needs to be up causing the devices to be removed from the corresponding
> native bus.
> 
> Prevent this from happening by making both sides of the link runtime PM
> active briefly. The pm_runtime_put() for the consumer (PCIe
> root/downstream port, xHCI) then allows it to runtime suspend again but
> keeps the supplier runtime resumed the whole time it is runtime active.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

> ---
>  drivers/thunderbolt/acpi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
> index b67e72d5644b..7c9597a33929 100644
> --- a/drivers/thunderbolt/acpi.c
> +++ b/drivers/thunderbolt/acpi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "tb.h"
>  
> @@ -74,8 +75,18 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>  		 pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM))) {
>  		const struct device_link *link;
>  
> +		/*
> +		 * Make them both active first to make sure the NHI does
> +		 * not runtime suspend before the consumer. The
> +		 * pm_runtime_put() below then allows the consumer to
> +		 * runtime suspend again (which then allows NHI runtime
> +		 * suspend too now that the device link is established).
> +		 */
> +		pm_runtime_get_sync(&pdev->dev);
> +
>  		link = device_link_add(&pdev->dev, &nhi->pdev->dev,
>  				       DL_FLAG_AUTOREMOVE_SUPPLIER |
> +				       DL_FLAG_RPM_ACTIVE |
>  				       DL_FLAG_PM_RUNTIME);
>  		if (link) {
>  			dev_dbg(&nhi->pdev->dev, "created link from %s\n",
> @@ -84,6 +95,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>  			dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n",
>  				 dev_name(&pdev->dev));
>  		}
> +
> +		pm_runtime_put(&pdev->dev);
>  	}
>  
>  out_put:
> 