From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Mario.Limonciello@dell.com
Cc: kbusch@kernel.org, axboe@fb.com, hch@lst.de, sagi@grimberg.me,
linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
Ryan.Hong@dell.com, Crag.Wang@dell.com, sjg@google.com,
Jared.Dominguez@dell.com
Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state
Date: Wed, 18 Sep 2019 23:27:01 +0200 [thread overview]
Message-ID: <5633857.hqcsiBO4aL@kreacher> (raw)
In-Reply-To: <706f61c67b354f3d8f841a82e3d48541@AUSX13MPC105.AMER.DELL.COM>
On Wednesday, September 18, 2019 6:52:31 PM CEST Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Sent: Tuesday, September 17, 2019 4:36 PM
> > To: Keith Busch
> > Cc: Limonciello, Mario; Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux-
> > nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com;
> > Dominguez, Jared
> > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest
> > state
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Tuesday, September 17, 2019 11:24:14 PM CEST Keith Busch wrote:
> > > On Wed, Sep 11, 2019 at 06:42:33PM -0500, Mario Limonciello wrote:
> > > > The action of saving the PCI state will cause numerous PCI configuration
> > > > space reads which depending upon the vendor implementation may cause
> > > > the drive to exit the deepest NVMe state.
> > > >
> > > > In these cases ASPM will typically resolve the PCIe link state and APST
> > > > may resolve the NVMe power state. However it has also been observed
> > > > that this register access after quiesced will cause PC10 failure
> > > > on some device combinations.
> > > >
> > > > To resolve this, move the PCI state saving to before SetFeatures has been
> > > > called. This has been proven to resolve the issue across a 5000 sample
> > > > test on previously failing disk/system combinations.
> > > >
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > ---
> > > > drivers/nvme/host/pci.c | 13 +++++++------
> > > > 1 file changed, 7 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > > > index 732d5b6..9b3fed4 100644
> > > > --- a/drivers/nvme/host/pci.c
> > > > +++ b/drivers/nvme/host/pci.c
> > > > @@ -2894,6 +2894,13 @@ static int nvme_suspend(struct device *dev)
> > > > if (ret < 0)
> > > > goto unfreeze;
> > > >
> > > > + /*
> > > > + * A saved state prevents pci pm from generically controlling the
> > > > + * device's power. If we're using protocol specific settings, we don't
> > > > + * want pci interfering.
> > > > + */
> > > > + pci_save_state(pdev);
> > > > +
> > > > ret = nvme_set_power_state(ctrl, ctrl->npss);
> > > > if (ret < 0)
> > > > goto unfreeze;
> > > > @@ -2908,12 +2915,6 @@ static int nvme_suspend(struct device *dev)
> > > > ret = 0;
> > > > goto unfreeze;
> > > > }
> > > > - /*
> > > > - * A saved state prevents pci pm from generically controlling the
> > > > - * device's power. If we're using protocol specific settings, we don't
> > > > - * want pci interfering.
> > > > - */
> > > > - pci_save_state(pdev);
> > > > unfreeze:
> > > > nvme_unfreeze(ctrl);
> > > > return ret;
> > >
> > > In the event that something else fails after the point you've saved
> > > the state, we need to fallback to the behavior for when the driver
> > > doesn't save the state, right?
> >
> > Depending on whether or not an error is going to be returned.
> >
> > When returning an error, it is not necessary to worry about the saved state,
> > because that will cause the entire system-wide suspend to be aborted.
>
> It looks like in this case an error would be returned.
Not necessarily.
If nvme_set_power_state() returns a positive number, you need to clear
pdev->state_saved before jumping to unfreeze.
Actually, you can drop the "goto unfreeze" after the "ret = 0" (in the
"if (ret)" block) and add the clearing of pdev->state_saved before it.
Let me reply to the original patch, though.
>
> >
> > Otherwise it is sufficient to clear the state_saved flag of the PCI device
> > before returning 0 to make the PCI layer take over.
>
next prev parent reply other threads:[~2019-09-18 21:27 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-11 23:42 [PATCH] nvme-pci: Save PCI state before putting drive into deepest state Mario Limonciello
2019-09-17 21:24 ` Keith Busch
2019-09-17 21:35 ` Rafael J. Wysocki
2019-09-18 16:52 ` Mario.Limonciello
2019-09-18 21:27 ` Rafael J. Wysocki [this message]
2019-09-18 17:28 ` Keith Busch
2019-09-18 21:31 ` Rafael J. Wysocki
2019-09-18 21:42 ` Rafael J. Wysocki
2019-09-18 21:43 ` Mario.Limonciello
2019-09-18 21:57 ` Rafael J. Wysocki
2019-09-18 22:00 ` Mario.Limonciello
2019-09-18 22:03 ` Rafael J. Wysocki
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=5633857.hqcsiBO4aL@kreacher \
--to=rjw@rjwysocki.net \
--cc=Crag.Wang@dell.com \
--cc=Jared.Dominguez@dell.com \
--cc=Mario.Limonciello@dell.com \
--cc=Ryan.Hong@dell.com \
--cc=axboe@fb.com \
--cc=hch@lst.de \
--cc=kbusch@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
--cc=sjg@google.com \
/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).