linux-kernel - Re: PM-runtime: supplier looses track of consumer during probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 1 Dec 2022 15:09:58 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Nitin Rawat <quic_nitirawa@...cinc.com>,
        Tushar Nimkar <quic_tnimkar@...cinc.com>,
        linux-pm@...r.kernel.org, linux-scsi@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     linux-arm-msm@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        bjorn.andersson@...nel.org, quic_mkshah@...cinc.com,
        quic_lsrao@...cinc.com, bvanassche@....org,
        Peter Wang <peter.wang@...iatek.com>
Subject: Re: PM-runtime: supplier looses track of consumer during probe

On 29/11/22 18:56, Nitin Rawat wrote:
> Hi Adrian,
> 
> On 11/21/2022 11:38 AM, Tushar Nimkar wrote:
>> Hi Adrian,
>>
>> On 11/18/2022 8:25 PM, Adrian Hunter wrote:
>>> On 4/11/22 11:19, Tushar Nimkar wrote:
>>>> Hi linux-pm/linux-scsi,
>>
>>>>> Process -1
>>>>> ufshcd_async_scan context (process 1)
>>>>> scsi_autopm_put_device() //0:0:0:0
>>>
>>> I am having trouble following your description.  What function is calling
>>> scsi_autopm_put_device() here?
>>>
>> Below is flow which calls scsi_autopm_put_device()
>> Process -1
>> ufshcd_async_scan()
>>      scsi_probe_and_add_lun()
>>          scsi_add_lun()
>>              slave_configure()
>>                  scsi_sysfs_add_sdev()
>>                      scsi_autopm_get_device()
>>                          device_add()     <- invoked [Process 2] sd_probe()
>>                              scsi_autopm_put_device()
>>
>>>>> pm_runtime_put_sync()
>>>>> __pm_runtime_idle()
>>>>> rpm_idle() -- RPM_GET_PUT(4)
>>>>>       __rpm_callback
>>>>>           scsi_runtime_idle()
>>>>>               pm_runtime_mark_last_busy()
>>>>>               pm_runtime_autosuspend()  --[A]
>>>>>                   rpm_suspend() -- RPM_AUTO(8)
>>>>>                       pm_runtime_autosuspend_expiration() use_autosuspend    is false return 0   --- [B]
>>>>>                           __update_runtime_status to RPM_SUSPENDING
>>>>>                       __rpm_callback()
>>>>>                           __rpm_put_suppliers(dev, false)
>>>>>                       __update_runtime_status to RPM_SUSPENDED
>>>>>                   rpm_suspend_suppliers()
>>>>>                       rpm_idle() for supplier -- RPM_ASYNC(1) return (-EAGAIN) [ Other consumer active for supplier]
>>>>>                   rpm_suspend() – END with return=0
>>>>>           scsi_runtime_idle() END return (-EBUSY) always.
>>>
>>> Not following here either.  Which device is EBUSY and why?
>>
>> scsi_runtime_idle() return -EBUSY always [3]
>> Storage/scsi team can better explain -EBUSY implementation.
> 
> EBUSY is returned from below code for consumer dev 0:0:0:0.
> scsi_runtime_idle is called from scsi_autopm_put_device which inturn is called from ufshcd_async_scan (Process 1 as per above call stack)
> static int scsi_runtime_idle(struct device *dev)
> {
>     :
> 
>     if (scsi_is_sdev_device(dev)) {
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_autosuspend(dev);
>         return -EBUSY; ---> EBUSY returned from here.
>     }
> 
>     
> }
> 
>>
>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/scsi/scsi_pm.c?h=next-20221118#n210
>>
>>
>>>>>
>>>>> [1]: https://lore.kernel.org/lkml/4748074.GXAFRqVoOG@kreacher/T/
>>>>> [2]: https://lkml.org/lkml/2022/10/12/259

It looks to me like __rpm_callback() makes assumptions about
dev->power.runtime_status that are not necessarily true because
dev->power.lock is dropped.  AFAICT the intention of the code
would be fulfilled by instead using the status as it was before
the lock was dropped.

Consequently, perhaps you could try this:

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b52049098d4e..3cf9abc3b2c2 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -365,6 +365,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 {
 	int retval = 0, idx;
 	bool use_links = dev->power.links_count > 0;
+	enum rpm_status runtime_status = dev->power.runtime_status;
 
 	if (dev->power.irq_safe) {
 		spin_unlock(&dev->power.lock);
@@ -378,7 +379,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 		 * routine returns, so it is safe to read the status outside of
 		 * the lock.
 		 */
-		if (use_links && dev->power.runtime_status == RPM_RESUMING) {
+		if (use_links && runtime_status == RPM_RESUMING) {
 			idx = device_links_read_lock();
 
 			retval = rpm_get_suppliers(dev);
@@ -405,8 +406,8 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 		 * Do that if resume fails too.
 		 */
 		if (use_links
-		    && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
-		    || (dev->power.runtime_status == RPM_RESUMING && retval))) {
+		    && ((runtime_status == RPM_SUSPENDING && !retval)
+		    || (runtime_status == RPM_RESUMING && retval))) {
 			idx = device_links_read_lock();
 
 			__rpm_put_suppliers(dev, false);