Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: cang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 23 Nov 2020 09:23:53 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     asutoshd@codeaurora.org, nguyenb@codeaurora.org,
        hongwus@codeaurora.org, ziqichen@codeaurora.org,
        rnayak@codeaurora.org, linux-scsi@vger.kernel.org,
        kernel-team@android.com, saravanak@google.com, salyzyn@google.com,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bart Van Assche <bvanassche@acm.org>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during
 system resume/thaw/restore
In-Reply-To: <20201120163524.GB619708@rowland.harvard.edu>
References: <1605861443-11459-1-git-send-email-cang@codeaurora.org>
 <20201120163524.GB619708@rowland.harvard.edu>
Message-ID: <ff2975f88cc452d134b8bf24c55bec09@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

Hi Alan,

On 2020-11-21 00:35, Alan Stern wrote:
> On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
>> Runtime resume is handled by runtime PM framework, no need to forcibly
>> set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.
> 
> Sorry, I don't understand this explanation at all.
> 
> Sure, runtime resume is handled by the runtime PM framework.  But this
> patch changes the code for system resume, which is completely 
> different.
> 
> Following a system resume, the hardware will be at full power.  We 
> don't
> want the kernel to think that the device is still in runtime suspend;
> otherwise is would never put the device back into low-power mode.

How about adding below lines to the patch?

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 908f27f..7ebe582 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -75,9 +75,11 @@ static int scsi_dev_type_resume(struct device *dev,
         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : 
NULL;
         int err = 0;

-       err = cb(dev, pm);
-       scsi_device_resume(to_scsi_device(dev));
-       dev_dbg(dev, "scsi resume: %d\n", err);
+       if (pm_runtime_active(dev)) {
+               err = cb(dev, pm);
+               scsi_device_resume(to_scsi_device(dev));
+               dev_dbg(dev, "scsi resume: %d\n", err);
+       }

         return err;
  }

Whenever a device is accessed, the issuer or somewhere in the path
should do something like pm_runtime_get_sync (e.g. in sg_open()) or
pm_runtime_resume() (e.g. in blk_queue_enter()), in either sync or
async way. After the job (read/write/ioctl or whatever) is done,
either a pm_runtime_put_sync() or auto runtime suspend puts the device
back into runtime suspended/low-power mode. Since the func
scsi_bus_suspend_common() does nothing if device is already in runtime
suspended mode, scsi_dev_type_resume() should only resume the device
if it is runtime active.

Thanks,

Can Guo.

> Alan Stern
> 
>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>> Cc: Bart Van Assche <bvanassche@acm.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>> 
>> Changes since v1:
>> - Incorporated Bart's comments
>> 
>> ---
>>  drivers/scsi/scsi_pm.c | 24 +-----------------------
>>  1 file changed, 1 insertion(+), 23 deletions(-)
>> 
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index 3717eea..908f27f 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -79,25 +79,6 @@ static int scsi_dev_type_resume(struct device *dev,
>>  	scsi_device_resume(to_scsi_device(dev));
>>  	dev_dbg(dev, "scsi resume: %d\n", err);
>> 
>> -	if (err == 0) {
>> -		pm_runtime_disable(dev);
>> -		err = pm_runtime_set_active(dev);
>> -		pm_runtime_enable(dev);
>> -
>> -		/*
>> -		 * Forcibly set runtime PM status of request queue to "active"
>> -		 * to make sure we can again get requests from the queue
>> -		 * (see also blk_pm_peek_request()).
>> -		 *
>> -		 * The resume hook will correct runtime PM status of the disk.
>> -		 */
>> -		if (!err && scsi_is_sdev_device(dev)) {
>> -			struct scsi_device *sdev = to_scsi_device(dev);
>> -
>> -			blk_set_runtime_active(sdev->request_queue);
>> -		}
>> -	}
>> -
>>  	return err;
>>  }
>> 
>> @@ -165,11 +146,8 @@ static int scsi_bus_resume_common(struct device 
>> *dev,
>>  		 */
>>  		if (strncmp(scsi_scan_type, "async", 5) != 0)
>>  			async_synchronize_full_domain(&scsi_sd_pm_domain);
>> -	} else {
>> -		pm_runtime_disable(dev);
>> -		pm_runtime_set_active(dev);
>> -		pm_runtime_enable(dev);
>>  	}
>> +
>>  	return 0;
>>  }
>> 
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
>> Linux Foundation Collaborative Project.
>>