From: Alan Stern <stern@rowland.harvard.edu>
To: Can Guo <cang@codeaurora.org>
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
Date: Sun, 22 Nov 2020 22:02:02 -0500 [thread overview]
Message-ID: <20201123030202.GA694907@rowland.harvard.edu> (raw)
In-Reply-To: <ff2975f88cc452d134b8bf24c55bec09@codeaurora.org>
On Mon, Nov 23, 2020 at 09:23:53AM +0800, Can Guo wrote:
> 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.
You're starting to think along the right lines, but you are ignoring all
the other work that people have already done for handling these cases.
Please read Documentation/driver-api/pm/devices.rst very carefully,
especially the parts about returning a positive value from the ->prepare
callback (also known as "direct-complete" and related to the
DPM_FLAG_NO_DIRECT_COMPLETE and DPM_FLAG_SMART_PREPARE flags) and the
parts about the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME
flags. Then think about what you want to accomplish and write a patch
that takes all this information into account.
Key point: At no time should any part of the kernel think that the
device is in a low-power state when it is actually in a high-power
state, or vice versa.
Alan Stern
prev parent reply other threads:[~2020-11-23 3:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-20 8:37 [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore Can Guo
2020-11-20 16:35 ` Alan Stern
2020-11-21 17:00 ` Bart Van Assche
2020-11-21 17:32 ` Alan Stern
2020-11-23 1:23 ` Can Guo
2020-11-23 3:02 ` Alan Stern [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201123030202.GA694907@rowland.harvard.edu \
--to=stern@rowland.harvard.edu \
--cc=asutoshd@codeaurora.org \
--cc=bvanassche@acm.org \
--cc=cang@codeaurora.org \
--cc=hongwus@codeaurora.org \
--cc=jejb@linux.ibm.com \
--cc=kernel-team@android.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=matthias.bgg@gmail.com \
--cc=nguyenb@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=salyzyn@google.com \
--cc=saravanak@google.com \
--cc=stanley.chu@mediatek.com \
--cc=ziqichen@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).