linux-kernel - Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore

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] [day] [month] [year] [list]

Message-ID: <20201123030202.GA694907@rowland.harvard.edu>
Date:   Sun, 22 Nov 2020 22:02:02 -0500
From:   Alan Stern <stern@...land.harvard.edu>
To:     Can Guo <cang@...eaurora.org>
Cc:     asutoshd@...eaurora.org, nguyenb@...eaurora.org,
        hongwus@...eaurora.org, ziqichen@...eaurora.org,
        rnayak@...eaurora.org, linux-scsi@...r.kernel.org,
        kernel-team@...roid.com, saravanak@...gle.com, salyzyn@...gle.com,
        Stanley Chu <stanley.chu@...iatek.com>,
        Bart Van Assche <bvanassche@....org>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone
 during system resume/thaw/restore

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