Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Jon Hunter]

Hi Rafael,

On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a stateless device link to a certain supplier with
> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> consumer driver's probe callback, the supplier's PM-runtime usage
> counter will be nonzero after that which effectively causes the
> supplier to remain "always on" going forward.
> 
> Namely, device_link_add() called to add the link invokes
> device_link_rpm_prepare() which notices that the consumer driver is
> probing, so it increments the supplier's PM-runtime usage counter
> with the assumption that the link will stay around until
> pm_runtime_put_suppliers() is called by driver_probe_device(),
> but if the link goes away before that point, the supplier's
> PM-runtime usage counter will remain nonzero.
> 
> To prevent that from happening, first rework pm_runtime_get_suppliers()
> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> links and make the latter only drop rpm_active and the supplier's
> PM-runtime usage counter for each link by one, unless rpm_active is
> one already for it.  Next, modify device_link_add() to bump up the
> new link's rpm_active refcount and the suppliers PM-runtime usage
> counter by two, to prevent pm_runtime_put_suppliers(), if it is
> called subsequently, from suspending the supplier prematurely (in
> case its PM-runtime usage counter goes down to 0 in there).
> 
> Due to the way rpm_put_suppliers() works, this change does not
> affect runtime suspend of the consumer ends of new device links (or,
> generally, device links for which DL_FLAG_PM_RUNTIME has just been
> set).
> 
> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Note that the issue had been there before commit e2f3cd831a28, but it was
> overlooked by that commit and this change is a fix on top of it, so make
> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> that the patch will not be applicable to).
I noticed that yesterday's and today's -next were no longer booting on
one of our Tegra boards (Tegra210 Jetson TX2) because networking is
failing. The ethernet chip is a USB device and looking at the bootlogs I
can see that the Tegra XHCI driver is failing ...

 tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
 tegra-xusb 70090000.usb: HC died; cleaning up

The Tegra XHCI driver uses multiple power-domains and uses
device_link_add() to attach them. So now I am wondering if there is
something that we have got wrong in our implementation. However, I don't
see the device being probed deferred on boot or anything like that.

The driver in question is drivers/usb/host/xhci-tegra.c and we add the
links in the function tegra_xusb_powerdomain_init() which is before RPM
is enabled. Let me know if you have any thoughts.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Rafael J. Wysocki]

On Fri, Feb 15, 2019 at 12:00 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Rafael,
>
> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If a stateless device link to a certain supplier with
> > DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> > consumer driver's probe callback, the supplier's PM-runtime usage
> > counter will be nonzero after that which effectively causes the
> > supplier to remain "always on" going forward.
> >
> > Namely, device_link_add() called to add the link invokes
> > device_link_rpm_prepare() which notices that the consumer driver is
> > probing, so it increments the supplier's PM-runtime usage counter
> > with the assumption that the link will stay around until
> > pm_runtime_put_suppliers() is called by driver_probe_device(),
> > but if the link goes away before that point, the supplier's
> > PM-runtime usage counter will remain nonzero.
> >
> > To prevent that from happening, first rework pm_runtime_get_suppliers()
> > and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> > links and make the latter only drop rpm_active and the supplier's
> > PM-runtime usage counter for each link by one, unless rpm_active is
> > one already for it.  Next, modify device_link_add() to bump up the
> > new link's rpm_active refcount and the suppliers PM-runtime usage
> > counter by two, to prevent pm_runtime_put_suppliers(), if it is
> > called subsequently, from suspending the supplier prematurely (in
> > case its PM-runtime usage counter goes down to 0 in there).
> >
> > Due to the way rpm_put_suppliers() works, this change does not
> > affect runtime suspend of the consumer ends of new device links (or,
> > generally, device links for which DL_FLAG_PM_RUNTIME has just been
> > set).
> >
> > Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Note that the issue had been there before commit e2f3cd831a28, but it was
> > overlooked by that commit and this change is a fix on top of it, so make
> > the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> > that the patch will not be applicable to).
>
> I noticed that yesterday's and today's -next were no longer booting on
> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> failing. The ethernet chip is a USB device and looking at the bootlogs I
> can see that the Tegra XHCI driver is failing ...

Is it failing because of this particular commit?  That is, does
reverting the entire commit help?

>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>  tegra-xusb 70090000.usb: HC died; cleaning up
>
> The Tegra XHCI driver uses multiple power-domains and uses
> device_link_add() to attach them. So now I am wondering if there is
> something that we have got wrong in our implementation. However, I don't
> see the device being probed deferred on boot or anything like that.

It won't be, because you use stateless links.

> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> links in the function tegra_xusb_powerdomain_init() which is before RPM
> is enabled. Let me know if you have any thoughts.

Well, if it breaks, then there is a bug somewhere.  I'm not seeing it
now, but let's dig into this.

Since you don't pass DL_FLAG_RPM_ACTIVE to device_link_add(), the
changes related to that don't matter.

The links are not there before your probe function runs.  It adds the
links and then pm_runtime_put_suppliers() sees them, but since
link->rpm_active is one for the new links, it won't do anything with
them.

Well, there is a difference, but if it matters, then something fishy
is going on IMO.  Before this change pm_runtime_put_suppliers() would
do pm_runtime_put() on the new links' suppliers and (because their
PM-runtime usage counters are both one at that point) it will actually
try to suspend the suppliers.  It should be easy enough to verify if
this really matters, stay tuned.

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Rafael J. Wysocki]

On Friday, February 15, 2019 12:00:27 PM CET Jon Hunter wrote:
> Hi Rafael,
> 
> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If a stateless device link to a certain supplier with
> > DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> > consumer driver's probe callback, the supplier's PM-runtime usage
> > counter will be nonzero after that which effectively causes the
> > supplier to remain "always on" going forward.
> > 
> > Namely, device_link_add() called to add the link invokes
> > device_link_rpm_prepare() which notices that the consumer driver is
> > probing, so it increments the supplier's PM-runtime usage counter
> > with the assumption that the link will stay around until
> > pm_runtime_put_suppliers() is called by driver_probe_device(),
> > but if the link goes away before that point, the supplier's
> > PM-runtime usage counter will remain nonzero.
> > 
> > To prevent that from happening, first rework pm_runtime_get_suppliers()
> > and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> > links and make the latter only drop rpm_active and the supplier's
> > PM-runtime usage counter for each link by one, unless rpm_active is
> > one already for it.  Next, modify device_link_add() to bump up the
> > new link's rpm_active refcount and the suppliers PM-runtime usage
> > counter by two, to prevent pm_runtime_put_suppliers(), if it is
> > called subsequently, from suspending the supplier prematurely (in
> > case its PM-runtime usage counter goes down to 0 in there).
> > 
> > Due to the way rpm_put_suppliers() works, this change does not
> > affect runtime suspend of the consumer ends of new device links (or,
> > generally, device links for which DL_FLAG_PM_RUNTIME has just been
> > set).
> > 
> > Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org> 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > 
> > Note that the issue had been there before commit e2f3cd831a28, but it was
> > overlooked by that commit and this change is a fix on top of it, so make
> > the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> > that the patch will not be applicable to).
> I noticed that yesterday's and today's -next were no longer booting on
> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> failing. The ethernet chip is a USB device and looking at the bootlogs I
> can see that the Tegra XHCI driver is failing ...
> 
>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>  tegra-xusb 70090000.usb: HC died; cleaning up
> 
> The Tegra XHCI driver uses multiple power-domains and uses
> device_link_add() to attach them. So now I am wondering if there is
> something that we have got wrong in our implementation. However, I don't
> see the device being probed deferred on boot or anything like that.
> 
> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> links in the function tegra_xusb_powerdomain_init() which is before RPM
> is enabled. Let me know if you have any thoughts.

Please try the appended patch on top of the $subject one (provided that
reverting the $subject patch makes the problem go away).

---
 drivers/base/power/runtime.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1675,9 +1675,12 @@ void pm_runtime_put_suppliers(struct dev
 	idx = device_links_read_lock();
 
 	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
-		if (link->flags & DL_FLAG_PM_RUNTIME &&
-		    refcount_dec_not_one(&link->rpm_active))
-			pm_runtime_put(link->supplier);
+		if (link->flags & DL_FLAG_PM_RUNTIME) {
+			if (refcount_dec_not_one(&link->rpm_active))
+				pm_runtime_put(link->supplier);
+			else
+				pm_request_idle(link->supplier);
+		}
 
 	device_links_read_unlock(idx);
 }

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Jon Hunter]

On 15/02/2019 12:06, Rafael J. Wysocki wrote:
> On Friday, February 15, 2019 12:00:27 PM CET Jon Hunter wrote:
>> Hi Rafael,
>>
>> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> If a stateless device link to a certain supplier with
>>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
>>> consumer driver's probe callback, the supplier's PM-runtime usage
>>> counter will be nonzero after that which effectively causes the
>>> supplier to remain "always on" going forward.
>>>
>>> Namely, device_link_add() called to add the link invokes
>>> device_link_rpm_prepare() which notices that the consumer driver is
>>> probing, so it increments the supplier's PM-runtime usage counter
>>> with the assumption that the link will stay around until
>>> pm_runtime_put_suppliers() is called by driver_probe_device(),
>>> but if the link goes away before that point, the supplier's
>>> PM-runtime usage counter will remain nonzero.
>>>
>>> To prevent that from happening, first rework pm_runtime_get_suppliers()
>>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
>>> links and make the latter only drop rpm_active and the supplier's
>>> PM-runtime usage counter for each link by one, unless rpm_active is
>>> one already for it.  Next, modify device_link_add() to bump up the
>>> new link's rpm_active refcount and the suppliers PM-runtime usage
>>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
>>> called subsequently, from suspending the supplier prematurely (in
>>> case its PM-runtime usage counter goes down to 0 in there).
>>>
>>> Due to the way rpm_put_suppliers() works, this change does not
>>> affect runtime suspend of the consumer ends of new device links (or,
>>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
>>> set).
>>>
>>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
>>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org> 
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> Note that the issue had been there before commit e2f3cd831a28, but it was
>>> overlooked by that commit and this change is a fix on top of it, so make
>>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
>>> that the patch will not be applicable to).
>> I noticed that yesterday's and today's -next were no longer booting on
>> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
>> failing. The ethernet chip is a USB device and looking at the bootlogs I
>> can see that the Tegra XHCI driver is failing ...
>>
>>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>>  tegra-xusb 70090000.usb: HC died; cleaning up
>>
>> The Tegra XHCI driver uses multiple power-domains and uses
>> device_link_add() to attach them. So now I am wondering if there is
>> something that we have got wrong in our implementation. However, I don't
>> see the device being probed deferred on boot or anything like that.
>>
>> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
>> links in the function tegra_xusb_powerdomain_init() which is before RPM
>> is enabled. Let me know if you have any thoughts.
> 
> Please try the appended patch on top of the $subject one (provided that
> reverting the $subject patch makes the problem go away).

Thanks and yes to confirm, reverting the $subject patch on top of next
does make the issue go away.

> ---
>  drivers/base/power/runtime.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1675,9 +1675,12 @@ void pm_runtime_put_suppliers(struct dev
>  	idx = device_links_read_lock();
>  
>  	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> -		if (link->flags & DL_FLAG_PM_RUNTIME &&
> -		    refcount_dec_not_one(&link->rpm_active))
> -			pm_runtime_put(link->supplier);
> +		if (link->flags & DL_FLAG_PM_RUNTIME) {
> +			if (refcount_dec_not_one(&link->rpm_active))
> +				pm_runtime_put(link->supplier);
> +			else
> +				pm_request_idle(link->supplier);
> +		}
>  
>  	device_links_read_unlock(idx);
>  }

I will try this now and report back in a bit.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Jon Hunter]

On 15/02/2019 13:21, Jon Hunter wrote:
> 
> On 15/02/2019 12:06, Rafael J. Wysocki wrote:
>> On Friday, February 15, 2019 12:00:27 PM CET Jon Hunter wrote:
>>> Hi Rafael,
>>>
>>> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> If a stateless device link to a certain supplier with
>>>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
>>>> consumer driver's probe callback, the supplier's PM-runtime usage
>>>> counter will be nonzero after that which effectively causes the
>>>> supplier to remain "always on" going forward.
>>>>
>>>> Namely, device_link_add() called to add the link invokes
>>>> device_link_rpm_prepare() which notices that the consumer driver is
>>>> probing, so it increments the supplier's PM-runtime usage counter
>>>> with the assumption that the link will stay around until
>>>> pm_runtime_put_suppliers() is called by driver_probe_device(),
>>>> but if the link goes away before that point, the supplier's
>>>> PM-runtime usage counter will remain nonzero.
>>>>
>>>> To prevent that from happening, first rework pm_runtime_get_suppliers()
>>>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
>>>> links and make the latter only drop rpm_active and the supplier's
>>>> PM-runtime usage counter for each link by one, unless rpm_active is
>>>> one already for it.  Next, modify device_link_add() to bump up the
>>>> new link's rpm_active refcount and the suppliers PM-runtime usage
>>>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
>>>> called subsequently, from suspending the supplier prematurely (in
>>>> case its PM-runtime usage counter goes down to 0 in there).
>>>>
>>>> Due to the way rpm_put_suppliers() works, this change does not
>>>> affect runtime suspend of the consumer ends of new device links (or,
>>>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
>>>> set).
>>>>
>>>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
>>>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org> 
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>
>>>> Note that the issue had been there before commit e2f3cd831a28, but it was
>>>> overlooked by that commit and this change is a fix on top of it, so make
>>>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
>>>> that the patch will not be applicable to).
>>> I noticed that yesterday's and today's -next were no longer booting on
>>> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
>>> failing. The ethernet chip is a USB device and looking at the bootlogs I
>>> can see that the Tegra XHCI driver is failing ...
>>>
>>>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>>>  tegra-xusb 70090000.usb: HC died; cleaning up
>>>
>>> The Tegra XHCI driver uses multiple power-domains and uses
>>> device_link_add() to attach them. So now I am wondering if there is
>>> something that we have got wrong in our implementation. However, I don't
>>> see the device being probed deferred on boot or anything like that.
>>>
>>> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
>>> links in the function tegra_xusb_powerdomain_init() which is before RPM
>>> is enabled. Let me know if you have any thoughts.
>>
>> Please try the appended patch on top of the $subject one (provided that
>> reverting the $subject patch makes the problem go away).
> 
> Thanks and yes to confirm, reverting the $subject patch on top of next
> does make the issue go away.
> 
>> ---
>>  drivers/base/power/runtime.c |    9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/runtime.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/runtime.c
>> +++ linux-pm/drivers/base/power/runtime.c
>> @@ -1675,9 +1675,12 @@ void pm_runtime_put_suppliers(struct dev
>>  	idx = device_links_read_lock();
>>  
>>  	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
>> -		if (link->flags & DL_FLAG_PM_RUNTIME &&
>> -		    refcount_dec_not_one(&link->rpm_active))
>> -			pm_runtime_put(link->supplier);
>> +		if (link->flags & DL_FLAG_PM_RUNTIME) {
>> +			if (refcount_dec_not_one(&link->rpm_active))
>> +				pm_runtime_put(link->supplier);
>> +			else
>> +				pm_request_idle(link->supplier);
>> +		}
>>  
>>  	device_links_read_unlock(idx);
>>  }
> 
> I will try this now and report back in a bit.

I tried this on top of next, but unfortunately the same issue still
persists and so this did not fix it. Let me know if there is any debug I
can add/enable.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Ulf Hansson]

On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Rafael,
>
> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If a stateless device link to a certain supplier with
> > DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> > consumer driver's probe callback, the supplier's PM-runtime usage
> > counter will be nonzero after that which effectively causes the
> > supplier to remain "always on" going forward.
> >
> > Namely, device_link_add() called to add the link invokes
> > device_link_rpm_prepare() which notices that the consumer driver is
> > probing, so it increments the supplier's PM-runtime usage counter
> > with the assumption that the link will stay around until
> > pm_runtime_put_suppliers() is called by driver_probe_device(),
> > but if the link goes away before that point, the supplier's
> > PM-runtime usage counter will remain nonzero.
> >
> > To prevent that from happening, first rework pm_runtime_get_suppliers()
> > and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> > links and make the latter only drop rpm_active and the supplier's
> > PM-runtime usage counter for each link by one, unless rpm_active is
> > one already for it.  Next, modify device_link_add() to bump up the
> > new link's rpm_active refcount and the suppliers PM-runtime usage
> > counter by two, to prevent pm_runtime_put_suppliers(), if it is
> > called subsequently, from suspending the supplier prematurely (in
> > case its PM-runtime usage counter goes down to 0 in there).
> >
> > Due to the way rpm_put_suppliers() works, this change does not
> > affect runtime suspend of the consumer ends of new device links (or,
> > generally, device links for which DL_FLAG_PM_RUNTIME has just been
> > set).
> >
> > Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Note that the issue had been there before commit e2f3cd831a28, but it was
> > overlooked by that commit and this change is a fix on top of it, so make
> > the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> > that the patch will not be applicable to).
> I noticed that yesterday's and today's -next were no longer booting on
> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> failing. The ethernet chip is a USB device and looking at the bootlogs I
> can see that the Tegra XHCI driver is failing ...
>
>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>  tegra-xusb 70090000.usb: HC died; cleaning up
>
> The Tegra XHCI driver uses multiple power-domains and uses
> device_link_add() to attach them. So now I am wondering if there is
> something that we have got wrong in our implementation. However, I don't
> see the device being probed deferred on boot or anything like that.
>
> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> links in the function tegra_xusb_powerdomain_init() which is before RPM
> is enabled. Let me know if you have any thoughts.

If you are willing to help debugging then I am offering my assistance.

I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
some more information about the runtime PM state of the device, like
the usage count for example.
I would also add a couple of prints in
tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
callbacks for the corresponding genpds, to see when those gets called.

While I was testing $subject patch I also used a local debug patch,
which adds a sysfs node that can be used to get the state of linked
suppliers for a consumer device. Feel free to use it, attached below.

Of course, the interesting part is the comparison of what happens with
and without $subject patch.

From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Mon, 11 Feb 2019 15:37:44 +0100
Subject: [PATCH] PM / Runtime: Add sysfs for runtime counting of suppliers

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/sysfs.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738ce796..ce5c188cdf54 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -537,6 +537,25 @@ static ssize_t runtime_enabled_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(runtime_enabled);

+static ssize_t runtime_suppliers_show(struct device *dev,
+                                 struct device_attribute *attr, char *buf)
+{
+       struct device_link *link;
+       int chars = 0;
+
+       list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+
+               if (!(link->flags & DL_FLAG_PM_RUNTIME))
+                       continue;
+
+               chars += sprintf(buf + chars, "%s %d\n",
+                               dev_name(link->supplier),
+                               refcount_read(&link->rpm_active));
+       }
+       return chars;
+}
+static DEVICE_ATTR_RO(runtime_suppliers);
+
 #ifdef CONFIG_PM_SLEEP
 static ssize_t async_show(struct device *dev, struct device_attribute *attr,
                          char *buf)
@@ -572,6 +591,7 @@ static struct attribute *power_attrs[] = {
        &dev_attr_runtime_usage.attr,
        &dev_attr_runtime_active_kids.attr,
        &dev_attr_runtime_enabled.attr,
+       &dev_attr_runtime_suppliers.attr,
 #endif /* CONFIG_PM_ADVANCED_DEBUG */
        NULL,
 };
-- 
2.17.1

Kind regards
Uffe

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Jon Hunter]

On 15/02/2019 14:37, Ulf Hansson wrote:
> On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Rafael,
>>
>> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> If a stateless device link to a certain supplier with
>>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
>>> consumer driver's probe callback, the supplier's PM-runtime usage
>>> counter will be nonzero after that which effectively causes the
>>> supplier to remain "always on" going forward.
>>>
>>> Namely, device_link_add() called to add the link invokes
>>> device_link_rpm_prepare() which notices that the consumer driver is
>>> probing, so it increments the supplier's PM-runtime usage counter
>>> with the assumption that the link will stay around until
>>> pm_runtime_put_suppliers() is called by driver_probe_device(),
>>> but if the link goes away before that point, the supplier's
>>> PM-runtime usage counter will remain nonzero.
>>>
>>> To prevent that from happening, first rework pm_runtime_get_suppliers()
>>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
>>> links and make the latter only drop rpm_active and the supplier's
>>> PM-runtime usage counter for each link by one, unless rpm_active is
>>> one already for it.  Next, modify device_link_add() to bump up the
>>> new link's rpm_active refcount and the suppliers PM-runtime usage
>>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
>>> called subsequently, from suspending the supplier prematurely (in
>>> case its PM-runtime usage counter goes down to 0 in there).
>>>
>>> Due to the way rpm_put_suppliers() works, this change does not
>>> affect runtime suspend of the consumer ends of new device links (or,
>>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
>>> set).
>>>
>>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
>>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> Note that the issue had been there before commit e2f3cd831a28, but it was
>>> overlooked by that commit and this change is a fix on top of it, so make
>>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
>>> that the patch will not be applicable to).
>> I noticed that yesterday's and today's -next were no longer booting on
>> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
>> failing. The ethernet chip is a USB device and looking at the bootlogs I
>> can see that the Tegra XHCI driver is failing ...
>>
>>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>>  tegra-xusb 70090000.usb: HC died; cleaning up
>>
>> The Tegra XHCI driver uses multiple power-domains and uses
>> device_link_add() to attach them. So now I am wondering if there is
>> something that we have got wrong in our implementation. However, I don't
>> see the device being probed deferred on boot or anything like that.
>>
>> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
>> links in the function tegra_xusb_powerdomain_init() which is before RPM
>> is enabled. Let me know if you have any thoughts.
> 
> If you are willing to help debugging then I am offering my assistance.
> 
> I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
> some more information about the runtime PM state of the device, like
> the usage count for example.
> I would also add a couple of prints in
> tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
> callbacks for the corresponding genpds, to see when those gets called.

From the bootlog I see ...

[    4.445827] tegra_xusb_runtime_resume-788
[    4.508799] tegra-xusb 70090000.usb: Firmware timestamp: 2015-08-10 09:47:54 UTC
[    4.516223] tegra-xusb 70090000.usb: xHCI Host Controller
[    4.521622] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 1
[    4.530087] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci version 0x100 quirks 0x0000000000010010
[    4.539398] tegra-xusb 70090000.usb: irq 69, io mem 0x70090000
[    4.553671] tegra-xusb 70090000.usb: xHCI Host Controller
[    4.559064] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 2
[    4.566622] tegra-xusb 70090000.usb: Host supports USB 3.0  SuperSpeed
[    4.595393] tegra-pmc: tegra_genpd_power_off-673: xusbc
[    4.600672] tegra-pmc: tegra_genpd_power_off-673: xusba
[    4.657346] tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
[    4.665157] tegra-xusb 70090000.usb: HC died; cleaning up

This shows the xusb controller is runtime resumed during probe but
then after probe the pm-domains, xusba and xusbc, are turned off
without it being runtime suspended. We never suspend it until it
is removed currently. 

Following boot the xusb device appears to still be active ... 

$ cat /sys/devices/platform/70090000.usb/power/runtime_status 
active

... but the status reported by the pm_genpd summary disagrees ...

$ sudo cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
vic                             off-0           
    /devices/platform/50000000.host1x/54340000.vic      suspended
xusbc                           off-0           
    /devices/genpd:0:70090000.usb                       suspended
xusbb                           off-0           
xusba                           off-0           
    /devices/genpd:1:70090000.usb                       suspended
sor                             on              
    /devices/platform/700e3000.mipi                     unsupported
    /devices/platform/50000000.host1x/54300000.dsi      active
    /devices/platform/50000000.host1x/54040000.dpaux    active
    /devices/platform/50000000.host1x/54580000.sor      suspended
aud                             off-0           

... and ...

$ cat /sys/devices/platform/70090000.usb/power/runtime_suppliers 
genpd:0:70090000.usb 1
genpd:1:70090000.usb 1

Let me know if you have any thoughts.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Rafael J. Wysocki]

On Fri, Feb 15, 2019 at 5:44 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 15/02/2019 14:37, Ulf Hansson wrote:
> > On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> If a stateless device link to a certain supplier with
> >>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> >>> consumer driver's probe callback, the supplier's PM-runtime usage
> >>> counter will be nonzero after that which effectively causes the
> >>> supplier to remain "always on" going forward.
> >>>
> >>> Namely, device_link_add() called to add the link invokes
> >>> device_link_rpm_prepare() which notices that the consumer driver is
> >>> probing, so it increments the supplier's PM-runtime usage counter
> >>> with the assumption that the link will stay around until
> >>> pm_runtime_put_suppliers() is called by driver_probe_device(),
> >>> but if the link goes away before that point, the supplier's
> >>> PM-runtime usage counter will remain nonzero.
> >>>
> >>> To prevent that from happening, first rework pm_runtime_get_suppliers()
> >>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> >>> links and make the latter only drop rpm_active and the supplier's
> >>> PM-runtime usage counter for each link by one, unless rpm_active is
> >>> one already for it.  Next, modify device_link_add() to bump up the
> >>> new link's rpm_active refcount and the suppliers PM-runtime usage
> >>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
> >>> called subsequently, from suspending the supplier prematurely (in
> >>> case its PM-runtime usage counter goes down to 0 in there).
> >>>
> >>> Due to the way rpm_put_suppliers() works, this change does not
> >>> affect runtime suspend of the consumer ends of new device links (or,
> >>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
> >>> set).
> >>>
> >>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> >>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>
> >>> Note that the issue had been there before commit e2f3cd831a28, but it was
> >>> overlooked by that commit and this change is a fix on top of it, so make
> >>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> >>> that the patch will not be applicable to).
> >> I noticed that yesterday's and today's -next were no longer booting on
> >> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> >> failing. The ethernet chip is a USB device and looking at the bootlogs I
> >> can see that the Tegra XHCI driver is failing ...
> >>
> >>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
> >>  tegra-xusb 70090000.usb: HC died; cleaning up
> >>
> >> The Tegra XHCI driver uses multiple power-domains and uses
> >> device_link_add() to attach them. So now I am wondering if there is
> >> something that we have got wrong in our implementation. However, I don't
> >> see the device being probed deferred on boot or anything like that.
> >>
> >> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> >> links in the function tegra_xusb_powerdomain_init() which is before RPM
> >> is enabled. Let me know if you have any thoughts.
> >
> > If you are willing to help debugging then I am offering my assistance.
> >
> > I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
> > some more information about the runtime PM state of the device, like
> > the usage count for example.
> > I would also add a couple of prints in
> > tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
> > callbacks for the corresponding genpds, to see when those gets called.
>
> From the bootlog I see ...
>
> [    4.445827] tegra_xusb_runtime_resume-788
> [    4.508799] tegra-xusb 70090000.usb: Firmware timestamp: 2015-08-10 09:47:54 UTC
> [    4.516223] tegra-xusb 70090000.usb: xHCI Host Controller
> [    4.521622] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 1
> [    4.530087] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci version 0x100 quirks 0x0000000000010010
> [    4.539398] tegra-xusb 70090000.usb: irq 69, io mem 0x70090000
> [    4.553671] tegra-xusb 70090000.usb: xHCI Host Controller
> [    4.559064] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 2
> [    4.566622] tegra-xusb 70090000.usb: Host supports USB 3.0  SuperSpeed
> [    4.595393] tegra-pmc: tegra_genpd_power_off-673: xusbc
> [    4.600672] tegra-pmc: tegra_genpd_power_off-673: xusba
> [    4.657346] tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
> [    4.665157] tegra-xusb 70090000.usb: HC died; cleaning up
>
> This shows the xusb controller is runtime resumed during probe but
> then after probe the pm-domains, xusba and xusbc, are turned off
> without it being runtime suspended. We never suspend it until it
> is removed currently.

As I said offline on Sat, I suspected that the suppliers (that would
be xusba and xusbc if my understanding of the system is correct) were
suspended prematurely during or after the consumer (xusb) probe, which
previously had been prevented from occurring by the
pm_runtime_get_noresume(supplier) removed by the $subject patch.

What you said above seems to be in agreement with that theory, so it
looks like you simply need to pass DL_FLAG_RPM_ACTICE to
device_link_add() in both cases, as that would prevent the suppliers
from being suspended until the consumer suspends (or its PM-runtime
status is changed to "suspended").  And if the consumer never
suspends, the suppliers will never suspend too then.

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Rafael J. Wysocki]

 On Fri, Feb 15, 2019 at 5:44 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 15/02/2019 14:37, Ulf Hansson wrote:
> > On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> If a stateless device link to a certain supplier with
> >>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> >>> consumer driver's probe callback, the supplier's PM-runtime usage
> >>> counter will be nonzero after that which effectively causes the
> >>> supplier to remain "always on" going forward.
> >>>
> >>> Namely, device_link_add() called to add the link invokes
> >>> device_link_rpm_prepare() which notices that the consumer driver is
> >>> probing, so it increments the supplier's PM-runtime usage counter
> >>> with the assumption that the link will stay around until
> >>> pm_runtime_put_suppliers() is called by driver_probe_device(),
> >>> but if the link goes away before that point, the supplier's
> >>> PM-runtime usage counter will remain nonzero.
> >>>
> >>> To prevent that from happening, first rework pm_runtime_get_suppliers()
> >>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> >>> links and make the latter only drop rpm_active and the supplier's
> >>> PM-runtime usage counter for each link by one, unless rpm_active is
> >>> one already for it.  Next, modify device_link_add() to bump up the
> >>> new link's rpm_active refcount and the suppliers PM-runtime usage
> >>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
> >>> called subsequently, from suspending the supplier prematurely (in
> >>> case its PM-runtime usage counter goes down to 0 in there).
> >>>
> >>> Due to the way rpm_put_suppliers() works, this change does not
> >>> affect runtime suspend of the consumer ends of new device links (or,
> >>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
> >>> set).
> >>>
> >>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> >>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>
> >>> Note that the issue had been there before commit e2f3cd831a28, but it was
> >>> overlooked by that commit and this change is a fix on top of it, so make
> >>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> >>> that the patch will not be applicable to).
> >> I noticed that yesterday's and today's -next were no longer booting on
> >> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> >> failing. The ethernet chip is a USB device and looking at the bootlogs I
> >> can see that the Tegra XHCI driver is failing ...
> >>
> >>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
> >>  tegra-xusb 70090000.usb: HC died; cleaning up
> >>
> >> The Tegra XHCI driver uses multiple power-domains and uses
> >> device_link_add() to attach them. So now I am wondering if there is
> >> something that we have got wrong in our implementation. However, I don't
> >> see the device being probed deferred on boot or anything like that.
> >>
> >> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> >> links in the function tegra_xusb_powerdomain_init() which is before RPM
> >> is enabled. Let me know if you have any thoughts.
> >
> > If you are willing to help debugging then I am offering my assistance.
> >
> > I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
> > some more information about the runtime PM state of the device, like
> > the usage count for example.
> > I would also add a couple of prints in
> > tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
> > callbacks for the corresponding genpds, to see when those gets called.
>
> From the bootlog I see ...
>
> [    4.445827] tegra_xusb_runtime_resume-788
> [    4.508799] tegra-xusb 70090000.usb: Firmware timestamp: 2015-08-10 09:47:54 UTC

This message comes from tegra_xusb_load_firmware() in
tegra_xusb_probe() which is after the pm_runtime_get_sync().

If the device was PM-runtime-suspended before, the
pm_runtime_get_sync() will runtime-resume and reference-count the
suppliers in addition to resuming the device.  In that case
pm_runtime_put_suppliers() will suspend the suppliers, so there is a
bug in there.

What happens is that the links are new when pm_runtime_get_sync() runs
and so their rpm_active refcounts are one.  After the
pm_runtime_get_sync() they are two and pm_runtime_put_suppliers() will
drop them by one and drop the PM-runtime usage counter of each of them
by one, so they will become zero and the suppliers will suspend.

Passing DL_FLAG_RPM_ACTIVE to device_link_add() should help, but IMO
things should also work without that.

> [    4.516223] tegra-xusb 70090000.usb: xHCI Host Controller
> [    4.521622] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 1

This comes from usb_add_hcd()

> [    4.530087] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci version 0x100 quirks 0x0000000000010010
> [    4.539398] tegra-xusb 70090000.usb: irq 69, io mem 0x70090000
> [    4.553671] tegra-xusb 70090000.usb: xHCI Host Controller
> [    4.559064] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 2

Like this.

> [    4.566622] tegra-xusb 70090000.usb: Host supports USB 3.0  SuperSpeed

And this if from xhci_gen_setup(), so probe returns around this point.

> [    4.595393] tegra-pmc: tegra_genpd_power_off-673: xusbc
> [    4.600672] tegra-pmc: tegra_genpd_power_off-673: xusba

And this appears to be done by pm_runtime_put_suppliers().

Hmm, I need to think how to fix this.  Maybe we'll need to revert
$subject patch and do something else, we'll see (later today).

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Jon Hunter]

On 18/02/2019 12:12, Rafael J. Wysocki wrote:
>  On Fri, Feb 15, 2019 at 5:44 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 15/02/2019 14:37, Ulf Hansson wrote:
>>> On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> If a stateless device link to a certain supplier with
>>>>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
>>>>> consumer driver's probe callback, the supplier's PM-runtime usage
>>>>> counter will be nonzero after that which effectively causes the
>>>>> supplier to remain "always on" going forward.
>>>>>
>>>>> Namely, device_link_add() called to add the link invokes
>>>>> device_link_rpm_prepare() which notices that the consumer driver is
>>>>> probing, so it increments the supplier's PM-runtime usage counter
>>>>> with the assumption that the link will stay around until
>>>>> pm_runtime_put_suppliers() is called by driver_probe_device(),
>>>>> but if the link goes away before that point, the supplier's
>>>>> PM-runtime usage counter will remain nonzero.
>>>>>
>>>>> To prevent that from happening, first rework pm_runtime_get_suppliers()
>>>>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
>>>>> links and make the latter only drop rpm_active and the supplier's
>>>>> PM-runtime usage counter for each link by one, unless rpm_active is
>>>>> one already for it.  Next, modify device_link_add() to bump up the
>>>>> new link's rpm_active refcount and the suppliers PM-runtime usage
>>>>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
>>>>> called subsequently, from suspending the supplier prematurely (in
>>>>> case its PM-runtime usage counter goes down to 0 in there).
>>>>>
>>>>> Due to the way rpm_put_suppliers() works, this change does not
>>>>> affect runtime suspend of the consumer ends of new device links (or,
>>>>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
>>>>> set).
>>>>>
>>>>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
>>>>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>> ---
>>>>>
>>>>> Note that the issue had been there before commit e2f3cd831a28, but it was
>>>>> overlooked by that commit and this change is a fix on top of it, so make
>>>>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
>>>>> that the patch will not be applicable to).
>>>> I noticed that yesterday's and today's -next were no longer booting on
>>>> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
>>>> failing. The ethernet chip is a USB device and looking at the bootlogs I
>>>> can see that the Tegra XHCI driver is failing ...
>>>>
>>>>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
>>>>  tegra-xusb 70090000.usb: HC died; cleaning up
>>>>
>>>> The Tegra XHCI driver uses multiple power-domains and uses
>>>> device_link_add() to attach them. So now I am wondering if there is
>>>> something that we have got wrong in our implementation. However, I don't
>>>> see the device being probed deferred on boot or anything like that.
>>>>
>>>> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
>>>> links in the function tegra_xusb_powerdomain_init() which is before RPM
>>>> is enabled. Let me know if you have any thoughts.
>>>
>>> If you are willing to help debugging then I am offering my assistance.
>>>
>>> I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
>>> some more information about the runtime PM state of the device, like
>>> the usage count for example.
>>> I would also add a couple of prints in
>>> tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
>>> callbacks for the corresponding genpds, to see when those gets called.
>>
>> From the bootlog I see ...
>>
>> [    4.445827] tegra_xusb_runtime_resume-788
>> [    4.508799] tegra-xusb 70090000.usb: Firmware timestamp: 2015-08-10 09:47:54 UTC
> 
> This message comes from tegra_xusb_load_firmware() in
> tegra_xusb_probe() which is after the pm_runtime_get_sync().
> 
> If the device was PM-runtime-suspended before, the
> pm_runtime_get_sync() will runtime-resume and reference-count the
> suppliers in addition to resuming the device.  In that case
> pm_runtime_put_suppliers() will suspend the suppliers, so there is a
> bug in there.
> 
> What happens is that the links are new when pm_runtime_get_sync() runs
> and so their rpm_active refcounts are one.  After the
> pm_runtime_get_sync() they are two and pm_runtime_put_suppliers() will
> drop them by one and drop the PM-runtime usage counter of each of them
> by one, so they will become zero and the suppliers will suspend.
> 
> Passing DL_FLAG_RPM_ACTIVE to device_link_add() should help, but IMO
> things should also work without that.

I can confirm that DL_FLAG_RPM_ACTIVE does indeed work. I assume though
this would prevent the suppliers from ever being suspended, which maybe
we will want to do eventually.

>> [    4.516223] tegra-xusb 70090000.usb: xHCI Host Controller
>> [    4.521622] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 1
> 
> This comes from usb_add_hcd()
> 
>> [    4.530087] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci version 0x100 quirks 0x0000000000010010
>> [    4.539398] tegra-xusb 70090000.usb: irq 69, io mem 0x70090000
>> [    4.553671] tegra-xusb 70090000.usb: xHCI Host Controller
>> [    4.559064] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 2
> 
> Like this.
> 
>> [    4.566622] tegra-xusb 70090000.usb: Host supports USB 3.0  SuperSpeed
> 
> And this if from xhci_gen_setup(), so probe returns around this point.
> 
>> [    4.595393] tegra-pmc: tegra_genpd_power_off-673: xusbc
>> [    4.600672] tegra-pmc: tegra_genpd_power_off-673: xusba
> 
> And this appears to be done by pm_runtime_put_suppliers().
> 
> Hmm, I need to think how to fix this.  Maybe we'll need to revert
> $subject patch and do something else, we'll see (later today).

OK, thanks. Let me know if there is anything else I can test.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/2] driver core: Fix possible supplier PM-usage counter imbalance

[Rafael J. Wysocki]

On Monday, February 18, 2019 2:02:50 PM CET Jon Hunter wrote:
> 
> On 18/02/2019 12:12, Rafael J. Wysocki wrote:
> >  On Fri, Feb 15, 2019 at 5:44 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >>
> >> On 15/02/2019 14:37, Ulf Hansson wrote:
> >>> On Fri, 15 Feb 2019 at 12:00, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>>>
> >>>> Hi Rafael,
> >>>>
> >>>> On 12/02/2019 12:08, Rafael J. Wysocki wrote:
> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>>
> >>>>> If a stateless device link to a certain supplier with
> >>>>> DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the
> >>>>> consumer driver's probe callback, the supplier's PM-runtime usage
> >>>>> counter will be nonzero after that which effectively causes the
> >>>>> supplier to remain "always on" going forward.
> >>>>>
> >>>>> Namely, device_link_add() called to add the link invokes
> >>>>> device_link_rpm_prepare() which notices that the consumer driver is
> >>>>> probing, so it increments the supplier's PM-runtime usage counter
> >>>>> with the assumption that the link will stay around until
> >>>>> pm_runtime_put_suppliers() is called by driver_probe_device(),
> >>>>> but if the link goes away before that point, the supplier's
> >>>>> PM-runtime usage counter will remain nonzero.
> >>>>>
> >>>>> To prevent that from happening, first rework pm_runtime_get_suppliers()
> >>>>> and pm_runtime_put_suppliers() to use the rpm_active refounts of device
> >>>>> links and make the latter only drop rpm_active and the supplier's
> >>>>> PM-runtime usage counter for each link by one, unless rpm_active is
> >>>>> one already for it.  Next, modify device_link_add() to bump up the
> >>>>> new link's rpm_active refcount and the suppliers PM-runtime usage
> >>>>> counter by two, to prevent pm_runtime_put_suppliers(), if it is
> >>>>> called subsequently, from suspending the supplier prematurely (in
> >>>>> case its PM-runtime usage counter goes down to 0 in there).
> >>>>>
> >>>>> Due to the way rpm_put_suppliers() works, this change does not
> >>>>> affect runtime suspend of the consumer ends of new device links (or,
> >>>>> generally, device links for which DL_FLAG_PM_RUNTIME has just been
> >>>>> set).
> >>>>>
> >>>>> Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()")
> >>>>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>> ---
> >>>>>
> >>>>> Note that the issue had been there before commit e2f3cd831a28, but it was
> >>>>> overlooked by that commit and this change is a fix on top of it, so make
> >>>>> the Fixes: tag point to commit e2f3cd831a28 (instead of an earlier one
> >>>>> that the patch will not be applicable to).
> >>>> I noticed that yesterday's and today's -next were no longer booting on
> >>>> one of our Tegra boards (Tegra210 Jetson TX2) because networking is
> >>>> failing. The ethernet chip is a USB device and looking at the bootlogs I
> >>>> can see that the Tegra XHCI driver is failing ...
> >>>>
> >>>>  tegra-xusb 70090000.usb: xHCI host controller not responding, assume dead
> >>>>  tegra-xusb 70090000.usb: HC died; cleaning up
> >>>>
> >>>> The Tegra XHCI driver uses multiple power-domains and uses
> >>>> device_link_add() to attach them. So now I am wondering if there is
> >>>> something that we have got wrong in our implementation. However, I don't
> >>>> see the device being probed deferred on boot or anything like that.
> >>>>
> >>>> The driver in question is drivers/usb/host/xhci-tegra.c and we add the
> >>>> links in the function tegra_xusb_powerdomain_init() which is before RPM
> >>>> is enabled. Let me know if you have any thoughts.
> >>>
> >>> If you are willing to help debugging then I am offering my assistance.
> >>>
> >>> I would start by enabling CONFIG_PM_ADVANCED_DEBUG, which gives you
> >>> some more information about the runtime PM state of the device, like
> >>> the usage count for example.
> >>> I would also add a couple of prints in
> >>> tegra_xusb_runtime_suspend|resume() and in the ->power_on|off()
> >>> callbacks for the corresponding genpds, to see when those gets called.
> >>
> >> From the bootlog I see ...
> >>
> >> [    4.445827] tegra_xusb_runtime_resume-788
> >> [    4.508799] tegra-xusb 70090000.usb: Firmware timestamp: 2015-08-10 09:47:54 UTC
> > 
> > This message comes from tegra_xusb_load_firmware() in
> > tegra_xusb_probe() which is after the pm_runtime_get_sync().
> > 
> > If the device was PM-runtime-suspended before, the
> > pm_runtime_get_sync() will runtime-resume and reference-count the
> > suppliers in addition to resuming the device.  In that case
> > pm_runtime_put_suppliers() will suspend the suppliers, so there is a
> > bug in there.
> > 
> > What happens is that the links are new when pm_runtime_get_sync() runs
> > and so their rpm_active refcounts are one.  After the
> > pm_runtime_get_sync() they are two and pm_runtime_put_suppliers() will
> > drop them by one and drop the PM-runtime usage counter of each of them
> > by one, so they will become zero and the suppliers will suspend.
> > 
> > Passing DL_FLAG_RPM_ACTIVE to device_link_add() should help, but IMO
> > things should also work without that.
> 
> I can confirm that DL_FLAG_RPM_ACTIVE does indeed work. I assume though
> this would prevent the suppliers from ever being suspended,

No, it wouldn't.  It only prevents the supplier from suspending as long as
the consumer remains active.

In your case it shouldn't matter (it only matters because of the bug in
the $subject patch), because the consumer is suspended to start with.

Had it been active initially, though, passing DL_FLAG_RPM_ACTIVE to
device_link_add() would have been the only way to to force the supplier to
remain active accross the return from consumer probe until the consumer is
suspended.

> which maybe
> we will want to do eventually.
> 
> >> [    4.516223] tegra-xusb 70090000.usb: xHCI Host Controller
> >> [    4.521622] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 1
> > 
> > This comes from usb_add_hcd()
> > 
> >> [    4.530087] tegra-xusb 70090000.usb: hcc params 0x0184f525 hci version 0x100 quirks 0x0000000000010010
> >> [    4.539398] tegra-xusb 70090000.usb: irq 69, io mem 0x70090000
> >> [    4.553671] tegra-xusb 70090000.usb: xHCI Host Controller
> >> [    4.559064] tegra-xusb 70090000.usb: new USB bus registered, assigned bus number 2
> > 
> > Like this.
> > 
> >> [    4.566622] tegra-xusb 70090000.usb: Host supports USB 3.0  SuperSpeed
> > 
> > And this if from xhci_gen_setup(), so probe returns around this point.
> > 
> >> [    4.595393] tegra-pmc: tegra_genpd_power_off-673: xusbc
> >> [    4.600672] tegra-pmc: tegra_genpd_power_off-673: xusba
> > 
> > And this appears to be done by pm_runtime_put_suppliers().
> > 
> > Hmm, I need to think how to fix this.  Maybe we'll need to revert
> > $subject patch and do something else, we'll see (later today).
> 
> OK, thanks. Let me know if there is anything else I can test.

Please test https://patchwork.kernel.org/patch/10818923/

Cheers,
Rafael