* [PATCH] nvme-pci: Save PCI state before putting drive into deepest state @ 2019-09-11 23:42 Mario Limonciello 2019-09-17 21:24 ` Keith Busch ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Mario Limonciello @ 2019-09-11 23:42 UTC (permalink / raw) To: Keith Busch Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez, Mario Limonciello 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 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 17:28 ` Keith Busch 2019-09-18 21:31 ` Rafael J. Wysocki 2 siblings, 1 reply; 12+ messages in thread From: Keith Busch @ 2019-09-17 21:24 UTC (permalink / raw) To: Mario Limonciello Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-17 21:24 ` Keith Busch @ 2019-09-17 21:35 ` Rafael J. Wysocki 2019-09-18 16:52 ` Mario.Limonciello 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-17 21:35 UTC (permalink / raw) To: Keith Busch Cc: Mario Limonciello, Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez 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. Otherwise it is sufficient to clear the state_saved flag of the PCI device before returning 0 to make the PCI layer take over. ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-17 21:35 ` Rafael J. Wysocki @ 2019-09-18 16:52 ` Mario.Limonciello 2019-09-18 21:27 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Mario.Limonciello @ 2019-09-18 16:52 UTC (permalink / raw) To: rjw, kbusch Cc: axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez > -----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. > > Otherwise it is sufficient to clear the state_saved flag of the PCI device > before returning 0 to make the PCI layer take over. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-18 16:52 ` Mario.Limonciello @ 2019-09-18 21:27 ` Rafael J. Wysocki 0 siblings, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-18 21:27 UTC (permalink / raw) To: Mario.Limonciello Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez 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. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 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-18 17:28 ` Keith Busch 2019-09-18 21:31 ` Rafael J. Wysocki 2 siblings, 0 replies; 12+ messages in thread From: Keith Busch @ 2019-09-18 17:28 UTC (permalink / raw) To: Mario Limonciello Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez On Wed, Sep 11, 2019 at 06:42:33PM -0500, Mario Limonciello wrote: > --- > 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; We would need to clear the saved state here, though. You can also infact remove the unfreeze label and goto. > } > - /* > - * 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; > -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 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-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 2 siblings, 2 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-18 21:31 UTC (permalink / raw) To: Mario Limonciello Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez, Linux PCI, Linux PM On Thursday, September 12, 2019 1:42:33 AM CEST 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. This sounds reasonable to me, but it would be nice to CC that to linux-pm and/or linux-pci too. > 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) This is the case in which the PCI layer is expected to put the device into D3, so you need pdev->state_saved = 0; at this point, because you have saved the config space already. > ret = 0; > goto unfreeze; And here you don't need to jump to "unfreeze" any more. > } > - /* > - * 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; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-18 21:31 ` Rafael J. Wysocki @ 2019-09-18 21:42 ` Rafael J. Wysocki 2019-09-18 21:43 ` Mario.Limonciello 1 sibling, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-18 21:42 UTC (permalink / raw) To: Mario Limonciello, Keith Busch Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML, Ryan Hong, Crag Wang, sjg, Jared Dominguez, Linux PCI, Linux PM On Wednesday, September 18, 2019 11:31:19 PM CEST Rafael J. Wysocki wrote: > On Thursday, September 12, 2019 1:42:33 AM CEST 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. > > This sounds reasonable to me, but it would be nice to CC that to linux-pm > and/or linux-pci too. > > > 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) > > This is the case in which the PCI layer is expected to put the device into > D3, so you need > > pdev->state_saved = 0; > > at this point, because you have saved the config space already. > > > ret = 0; > > goto unfreeze; > > And here you don't need to jump to "unfreeze" any more. BTW, doing nvme_dev_disable() before nvme_unfreeze() looks odd to me. Maybe it would be better to do "unfreeze" and then "disable" in this case? > > > } > > - /* > > - * 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; > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 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 1 sibling, 1 reply; 12+ messages in thread From: Mario.Limonciello @ 2019-09-18 21:43 UTC (permalink / raw) To: rjw Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez, linux-pci, linux-pm > -----Original Message----- > From: Rafael J. Wysocki <rjw@rjwysocki.net> > Sent: Wednesday, September 18, 2019 4:31 PM > To: Limonciello, Mario > Cc: Keith Busch; Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux- > nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com; > Dominguez, Jared; Linux PCI; Linux PM > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest > state > > > [EXTERNAL EMAIL] > > On Thursday, September 12, 2019 1:42:33 AM CEST 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. > > This sounds reasonable to me, but it would be nice to CC that to linux-pm > and/or linux-pci too. > > > 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) > > This is the case in which the PCI layer is expected to put the device into > D3, so you need > > pdev->state_saved = 0; > > at this point, because you have saved the config space already. > > > ret = 0; > > goto unfreeze; > > And here you don't need to jump to "unfreeze" any more. > > > } > > - /* > > - * 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; > > > > > Thanks, I actually followed up with something along that line in a v2 sent out today. My apology you weren't in CC, but here is a weblink to it. http://lists.infradead.org/pipermail/linux-nvme/2019-September/027251.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-18 21:43 ` Mario.Limonciello @ 2019-09-18 21:57 ` Rafael J. Wysocki 2019-09-18 22:00 ` Mario.Limonciello 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-18 21:57 UTC (permalink / raw) To: Mario.Limonciello Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez, linux-pci, linux-pm On Wednesday, September 18, 2019 11:43:28 PM CEST Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Rafael J. Wysocki <rjw@rjwysocki.net> > > Sent: Wednesday, September 18, 2019 4:31 PM > > To: Limonciello, Mario > > Cc: Keith Busch; Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux- > > nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com; > > Dominguez, Jared; Linux PCI; Linux PM > > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest > > state > > > > > > [EXTERNAL EMAIL] > > > > On Thursday, September 12, 2019 1:42:33 AM CEST 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. > > > > This sounds reasonable to me, but it would be nice to CC that to linux-pm > > and/or linux-pci too. > > > > > 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) > > > > This is the case in which the PCI layer is expected to put the device into > > D3, so you need > > > > pdev->state_saved = 0; > > > > at this point, because you have saved the config space already. > > > > > ret = 0; > > > goto unfreeze; > > > > And here you don't need to jump to "unfreeze" any more. > > > > > } > > > - /* > > > - * 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; > > > > > > > > > > > Thanks, I actually followed up with something along that line in a v2 sent out > today. My apology you weren't in CC, but here is a weblink to it. > http://lists.infradead.org/pipermail/linux-nvme/2019-September/027251.html > I don't think that pci_load_saved_state() will work, because it sets state_saved at the end again (if all goes well). You simply only need to clear state_saved here. ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-18 21:57 ` Rafael J. Wysocki @ 2019-09-18 22:00 ` Mario.Limonciello 2019-09-18 22:03 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Mario.Limonciello @ 2019-09-18 22:00 UTC (permalink / raw) To: rjw Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez, linux-pci, linux-pm > -----Original Message----- > From: Rafael J. Wysocki <rjw@rjwysocki.net> > Sent: Wednesday, September 18, 2019 4:57 PM > To: Limonciello, Mario > Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me; linux- > nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Hong, Ryan; Wang, > Crag; sjg@google.com; Dominguez, Jared; linux-pci@vger.kernel.org; linux- > pm@vger.kernel.org > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest > state > > > [EXTERNAL EMAIL] > > On Wednesday, September 18, 2019 11:43:28 PM CEST > Mario.Limonciello@dell.com wrote: > > > -----Original Message----- > > > From: Rafael J. Wysocki <rjw@rjwysocki.net> > > > Sent: Wednesday, September 18, 2019 4:31 PM > > > To: Limonciello, Mario > > > Cc: Keith Busch; Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux- > > > nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com; > > > Dominguez, Jared; Linux PCI; Linux PM > > > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into > deepest > > > state > > > > > > > > > [EXTERNAL EMAIL] > > > > > > On Thursday, September 12, 2019 1:42:33 AM CEST 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. > > > > > > This sounds reasonable to me, but it would be nice to CC that to linux-pm > > > and/or linux-pci too. > > > > > > > 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) > > > > > > This is the case in which the PCI layer is expected to put the device into > > > D3, so you need > > > > > > pdev->state_saved = 0; > > > > > > at this point, because you have saved the config space already. > > > > > > > ret = 0; > > > > goto unfreeze; > > > > > > And here you don't need to jump to "unfreeze" any more. > > > > > > > } > > > > - /* > > > > - * 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; > > > > > > > > > > > > > > > > > Thanks, I actually followed up with something along that line in a v2 sent out > > today. My apology you weren't in CC, but here is a weblink to it. > > http://lists.infradead.org/pipermail/linux-nvme/2019-September/027251.html > > > > I don't think that pci_load_saved_state() will work, because it sets > state_saved at the end again (if all goes well). You simply only need to > clear state_saved here. Explicitly calling it with NULL as the saved state to restore seemed to have that effect of clearing state (there is an explicit check in there if it's NULL to just return 0). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state 2019-09-18 22:00 ` Mario.Limonciello @ 2019-09-18 22:03 ` Rafael J. Wysocki 0 siblings, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2019-09-18 22:03 UTC (permalink / raw) To: Mario Limonciello Cc: Rafael J. Wysocki, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, Linux Kernel Mailing List, Ryan.Hong, Crag.Wang, sjg, Jared.Dominguez, Linux PCI, Linux PM On Thu, Sep 19, 2019 at 12:00 AM <Mario.Limonciello@dell.com> wrote: > > > -----Original Message----- > > From: Rafael J. Wysocki <rjw@rjwysocki.net> > > Sent: Wednesday, September 18, 2019 4:57 PM > > To: Limonciello, Mario > > Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me; linux- > > nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Hong, Ryan; Wang, > > Crag; sjg@google.com; Dominguez, Jared; linux-pci@vger.kernel.org; linux- > > pm@vger.kernel.org > > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest > > state > > > > > > [EXTERNAL EMAIL] > > > > On Wednesday, September 18, 2019 11:43:28 PM CEST > > Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > > > From: Rafael J. Wysocki <rjw@rjwysocki.net> > > > > Sent: Wednesday, September 18, 2019 4:31 PM > > > > To: Limonciello, Mario > > > > Cc: Keith Busch; Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux- > > > > nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com; > > > > Dominguez, Jared; Linux PCI; Linux PM > > > > Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into > > deepest > > > > state > > > > > > > > > > > > [EXTERNAL EMAIL] > > > > > > > > On Thursday, September 12, 2019 1:42:33 AM CEST 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. > > > > > > > > This sounds reasonable to me, but it would be nice to CC that to linux-pm > > > > and/or linux-pci too. > > > > > > > > > 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) > > > > > > > > This is the case in which the PCI layer is expected to put the device into > > > > D3, so you need > > > > > > > > pdev->state_saved = 0; > > > > > > > > at this point, because you have saved the config space already. > > > > > > > > > ret = 0; > > > > > goto unfreeze; > > > > > > > > And here you don't need to jump to "unfreeze" any more. > > > > > > > > > } > > > > > - /* > > > > > - * 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; > > > > > > > > > > > > > > > > > > > > > > > Thanks, I actually followed up with something along that line in a v2 sent out > > > today. My apology you weren't in CC, but here is a weblink to it. > > > http://lists.infradead.org/pipermail/linux-nvme/2019-September/027251.html > > > > > > > I don't think that pci_load_saved_state() will work, because it sets > > state_saved at the end again (if all goes well). You simply only need to > > clear state_saved here. > > Explicitly calling it with NULL as the saved state to restore seemed to have that effect > of clearing state (there is an explicit check in there if it's NULL to just return 0). Ah, OK, right. I still would rather clear the flag directly, though, as using pci_load_saved_state() for that is just more convoluted. :-) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-18 22:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).