* [PATCH v2 0/2] nvme APST quirk updates, take two
@ 2017-04-20 16:11 Andy Lutomirski
2017-04-20 16:11 ` [PATCH v2 1/2] nvme: Adjust the Samsung APST quirk Andy Lutomirski
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andy Lutomirski @ 2017-04-20 16:11 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig,
Sagi Grimberg, Keith Busch, Andy Lutomirski
Hi Jens-
These are just the quirk updates, split out. The patches are
unchanged.
I think that, even if we want to apply a broader quirk for 4.11, we
should still apply these so that we can cleanly revert the broader
quirk later. IOW, let's get the known regressions fixed before we
get too excited about the unknown regressions.
Andy Lutomirski (2):
nvme: Adjust the Samsung APST quirk
nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA"
drivers/nvme/host/core.c | 28 ++++++++++++++++++++--------
drivers/nvme/host/nvme.h | 5 +++++
drivers/nvme/host/pci.c | 26 +++++++++++++++++++++++++-
3 files changed, 50 insertions(+), 9 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/2] nvme: Adjust the Samsung APST quirk 2017-04-20 16:11 [PATCH v2 0/2] nvme APST quirk updates, take two Andy Lutomirski @ 2017-04-20 16:11 ` Andy Lutomirski 2017-04-20 16:11 ` [PATCH v2 2/2] nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA" Andy Lutomirski 2017-04-20 16:13 ` [PATCH v2 0/2] nvme APST quirk updates, take two Jens Axboe 2 siblings, 0 replies; 10+ messages in thread From: Andy Lutomirski @ 2017-04-20 16:11 UTC (permalink / raw) To: Jens Axboe Cc: linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig, Sagi Grimberg, Keith Busch, Andy Lutomirski, Niranjan Sivakumar I got a couple more reports: the Samsung APST issues appears to affect multiple 950-series devices in Dell XPS 15 9550 and Precision 5510 laptops. Change the quirk: rather than blacklisting the firmware on the first problematic SSD that was reported, disable APST on all 144d:a802 devices if they're installed in the two affected Dell models. While we're at it, disable only the deepest sleep state instead of all of them -- the reporters say that this is sufficient to fix the problem. (I have a device that appears to be entirely identical to one of the affected devices, but I have a different Dell laptop, so it's not the case that all Samsung devices with firmware BXW75D0Q are broken under all circumstances.) Samsung engineers have an affected system, and hopefully they'll give us a better workaround some time soon. In the mean time, this should minimize regressions. See https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184 Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Cc: Niranjan Sivakumar <ns253@cornell.edu> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- drivers/nvme/host/core.c | 23 +++++++++++++---------- drivers/nvme/host/nvme.h | 5 +++++ drivers/nvme/host/pci.c | 26 +++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 9583a5f58a1d..d618b20ce6c6 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1316,6 +1316,14 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl) table->entries[state] = target; /* + * Don't allow transitions to the deepest state + * if it's quirked off. + */ + if (state == ctrl->npss && + (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) + continue; + + /* * Is this state a useful non-operational state for * higher-power states to autonomously transition to? */ @@ -1387,16 +1395,6 @@ struct nvme_core_quirk_entry { }; static const struct nvme_core_quirk_entry core_quirks[] = { - /* - * Seen on a Samsung "SM951 NVMe SAMSUNG 256GB": using APST causes - * the controller to go out to lunch. It dies when the watchdog - * timer reads CSTS and gets 0xffffffff. - */ - { - .vid = 0x144d, - .fr = "BXW75D0Q", - .quirks = NVME_QUIRK_NO_APST, - }, }; /* match is null-terminated but idstr is space-padded. */ @@ -1481,6 +1479,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) } } + if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) { + dev_warn(ctrl->dev, "forcibly allowing all power states due to nvme_core.force_apst -- use at your own risk\n"); + ctrl->quirks &= ~NVME_QUIRK_NO_DEEPEST_PS; + } + ctrl->oacs = le16_to_cpu(id->oacs); ctrl->vid = le16_to_cpu(id->vid); ctrl->oncs = le16_to_cpup(&id->oncs); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 2aa20e3e5675..ab2d6ec7eb5c 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -83,6 +83,11 @@ enum nvme_quirks { * APST should not be used. */ NVME_QUIRK_NO_APST = (1 << 4), + + /* + * The deepest sleep state should not be used. + */ + NVME_QUIRK_NO_DEEPEST_PS = (1 << 5), }; /* diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 26a5fd05fe88..5d309535abbd 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -19,6 +19,7 @@ #include <linux/blk-mq-pci.h> #include <linux/cpu.h> #include <linux/delay.h> +#include <linux/dmi.h> #include <linux/errno.h> #include <linux/fs.h> #include <linux/genhd.h> @@ -1943,10 +1944,31 @@ static int nvme_dev_map(struct nvme_dev *dev) return -ENODEV; } +static unsigned long check_dell_samsung_bug(struct pci_dev *pdev) +{ + if (pdev->vendor == 0x144d && pdev->device == 0xa802) { + /* + * Several Samsung devices seem to drop off the PCIe bus + * randomly when APST is on and uses the deepest sleep state. + * This has been observed on a Samsung "SM951 NVMe SAMSUNG + * 256GB", a "PM951 NVMe SAMSUNG 512GB", and a "Samsung SSD + * 950 PRO 256GB", but it seems to be restricted to two Dell + * laptops. + */ + if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") && + (dmi_match(DMI_PRODUCT_NAME, "XPS 15 9550") || + dmi_match(DMI_PRODUCT_NAME, "Precision 5510"))) + return NVME_QUIRK_NO_DEEPEST_PS; + } + + return 0; +} + static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) { int node, result = -ENOMEM; struct nvme_dev *dev; + unsigned long quirks = id->driver_data; node = dev_to_node(&pdev->dev); if (node == NUMA_NO_NODE) @@ -1978,8 +2000,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (result) goto put_pci; + quirks |= check_dell_samsung_bug(pdev); + result = nvme_init_ctrl(&dev->ctrl, &pdev->dev, &nvme_pci_ctrl_ops, - id->driver_data); + quirks); if (result) goto release_pools; -- 2.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA" 2017-04-20 16:11 [PATCH v2 0/2] nvme APST quirk updates, take two Andy Lutomirski 2017-04-20 16:11 ` [PATCH v2 1/2] nvme: Adjust the Samsung APST quirk Andy Lutomirski @ 2017-04-20 16:11 ` Andy Lutomirski 2017-04-20 16:13 ` [PATCH v2 0/2] nvme APST quirk updates, take two Jens Axboe 2 siblings, 0 replies; 10+ messages in thread From: Andy Lutomirski @ 2017-04-20 16:11 UTC (permalink / raw) To: Jens Axboe Cc: linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig, Sagi Grimberg, Keith Busch, Andy Lutomirski There's a report that it malfunctions with APST on. See https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184 Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- drivers/nvme/host/core.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index d618b20ce6c6..1fbcdcdada78 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1395,6 +1395,15 @@ struct nvme_core_quirk_entry { }; static const struct nvme_core_quirk_entry core_quirks[] = { + { + /* + * This Toshiba device seems to die using any APST states. See: + * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184/comments/11 + */ + .vid = 0x1179, + .mn = "THNSF5256GPUK TOSHIBA", + .quirks = NVME_QUIRK_NO_APST, + } }; /* match is null-terminated but idstr is space-padded. */ -- 2.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 16:11 [PATCH v2 0/2] nvme APST quirk updates, take two Andy Lutomirski 2017-04-20 16:11 ` [PATCH v2 1/2] nvme: Adjust the Samsung APST quirk Andy Lutomirski 2017-04-20 16:11 ` [PATCH v2 2/2] nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA" Andy Lutomirski @ 2017-04-20 16:13 ` Jens Axboe 2017-04-20 16:15 ` Sagi Grimberg 2 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2017-04-20 16:13 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig, Sagi Grimberg, Keith Busch On 04/20/2017 10:11 AM, Andy Lutomirski wrote: > Hi Jens- > > These are just the quirk updates, split out. The patches are > unchanged. > > I think that, even if we want to apply a broader quirk for 4.11, we > should still apply these so that we can cleanly revert the broader > quirk later. IOW, let's get the known regressions fixed before we > get too excited about the unknown regressions. This looks good to me, and 4.11 appropriate. I'll expedite this through the block tree, if Keith/Sagi/Christoph agrees on this being the right approach for 4.11. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 16:13 ` [PATCH v2 0/2] nvme APST quirk updates, take two Jens Axboe @ 2017-04-20 16:15 ` Sagi Grimberg 2017-04-20 16:29 ` Keith Busch 0 siblings, 1 reply; 10+ messages in thread From: Sagi Grimberg @ 2017-04-20 16:15 UTC (permalink / raw) To: Jens Axboe, Andy Lutomirski Cc: linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig, Keith Busch >> Hi Jens- >> >> These are just the quirk updates, split out. The patches are >> unchanged. >> >> I think that, even if we want to apply a broader quirk for 4.11, we >> should still apply these so that we can cleanly revert the broader >> quirk later. IOW, let's get the known regressions fixed before we >> get too excited about the unknown regressions. > > This looks good to me, and 4.11 appropriate. I'll expedite this > through the block tree, if Keith/Sagi/Christoph agrees on this > being the right approach for 4.11. I'm perfectly fine with this going to 4.11 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 16:15 ` Sagi Grimberg @ 2017-04-20 16:29 ` Keith Busch 2017-04-20 16:30 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Keith Busch @ 2017-04-20 16:29 UTC (permalink / raw) To: Sagi Grimberg Cc: Jens Axboe, Andy Lutomirski, linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig On Thu, Apr 20, 2017 at 07:15:15PM +0300, Sagi Grimberg wrote: > > > > Hi Jens- > > > > > > These are just the quirk updates, split out. The patches are > > > unchanged. > > > > > > I think that, even if we want to apply a broader quirk for 4.11, we > > > should still apply these so that we can cleanly revert the broader > > > quirk later. IOW, let's get the known regressions fixed before we > > > get too excited about the unknown regressions. > > > > This looks good to me, and 4.11 appropriate. I'll expedite this > > through the block tree, if Keith/Sagi/Christoph agrees on this > > being the right approach for 4.11. > > I'm perfectly fine with this going to 4.11 All good with me as well. Reviewed-by: Keith Busch <keith.busch@intel.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 16:29 ` Keith Busch @ 2017-04-20 16:30 ` Jens Axboe 2017-04-20 16:38 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2017-04-20 16:30 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg Cc: Andy Lutomirski, linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig On 04/20/2017 10:29 AM, Keith Busch wrote: > On Thu, Apr 20, 2017 at 07:15:15PM +0300, Sagi Grimberg wrote: >> >>>> Hi Jens- >>>> >>>> These are just the quirk updates, split out. The patches are >>>> unchanged. >>>> >>>> I think that, even if we want to apply a broader quirk for 4.11, we >>>> should still apply these so that we can cleanly revert the broader >>>> quirk later. IOW, let's get the known regressions fixed before we >>>> get too excited about the unknown regressions. >>> >>> This looks good to me, and 4.11 appropriate. I'll expedite this >>> through the block tree, if Keith/Sagi/Christoph agrees on this >>> being the right approach for 4.11. >> >> I'm perfectly fine with this going to 4.11 > > All good with me as well. > > Reviewed-by: Keith Busch <keith.busch@intel.com> Great, I have queued it up. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 16:30 ` Jens Axboe @ 2017-04-20 16:38 ` Jens Axboe [not found] ` <CALCETrUQ_npTbm7eEeoFJP3V9y-keq9bjS0hD+t=k-AXSDMteA@mail.gmail.com> 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2017-04-20 16:38 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg Cc: Andy Lutomirski, linux-kernel, Kai-Heng Feng, linux-nvme, Christoph Hellwig On 04/20/2017 10:30 AM, Jens Axboe wrote: > On 04/20/2017 10:29 AM, Keith Busch wrote: >> On Thu, Apr 20, 2017 at 07:15:15PM +0300, Sagi Grimberg wrote: >>> >>>>> Hi Jens- >>>>> >>>>> These are just the quirk updates, split out. The patches are >>>>> unchanged. >>>>> >>>>> I think that, even if we want to apply a broader quirk for 4.11, we >>>>> should still apply these so that we can cleanly revert the broader >>>>> quirk later. IOW, let's get the known regressions fixed before we >>>>> get too excited about the unknown regressions. >>>> >>>> This looks good to me, and 4.11 appropriate. I'll expedite this >>>> through the block tree, if Keith/Sagi/Christoph agrees on this >>>> being the right approach for 4.11. >>> >>> I'm perfectly fine with this going to 4.11 >> >> All good with me as well. >> >> Reviewed-by: Keith Busch <keith.busch@intel.com> > > Great, I have queued it up. Spoke too soon. Andy, did you compile this? drivers/nvme/host/core.c: In function ‘nvme_init_identify’: drivers/nvme/host/core.c:1524:6: error: ‘force_apst’ undeclared (first use in this function) if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) { ^~~~~~~~~~ drivers/nvme/host/core.c:1524:6: note: each undeclared identifier is reported only once for each function it appears in No, you did not. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CALCETrUQ_npTbm7eEeoFJP3V9y-keq9bjS0hD+t=k-AXSDMteA@mail.gmail.com>]
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two [not found] ` <CALCETrUQ_npTbm7eEeoFJP3V9y-keq9bjS0hD+t=k-AXSDMteA@mail.gmail.com> @ 2017-04-20 17:08 ` Jens Axboe 2017-04-20 18:56 ` Andy Lutomirski 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2017-04-20 17:08 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-nvme, Keith Busch, Kai-Heng Feng, Christoph Hellwig, linux-kernel, Sagi Grimberg On 04/20/2017 11:06 AM, Andy Lutomirski wrote: > > > On Apr 20, 2017 9:38 AM, "Jens Axboe" <axboe@kernel.dk <mailto:axboe@kernel.dk>> wrote: > > On 04/20/2017 10:30 AM, Jens Axboe wrote: > > On 04/20/2017 10:29 AM, Keith Busch wrote: > >> On Thu, Apr 20, 2017 at 07:15:15PM +0300, Sagi Grimberg wrote: > >>> > >>>>> Hi Jens- > >>>>> > >>>>> These are just the quirk updates, split out. The patches are > >>>>> unchanged. > >>>>> > >>>>> I think that, even if we want to apply a broader quirk for 4.11, we > >>>>> should still apply these so that we can cleanly revert the broader > >>>>> quirk later. IOW, let's get the known regressions fixed before we > >>>>> get too excited about the unknown regressions. > >>>> > >>>> This looks good to me, and 4.11 appropriate. I'll expedite this > >>>> through the block tree, if Keith/Sagi/Christoph agrees on this > >>>> being the right approach for 4.11. > >>> > >>> I'm perfectly fine with this going to 4.11 > >> > >> All good with me as well. > >> > >> Reviewed-by: Keith Busch <keith.busch@intel.com <mailto:keith.busch@intel.com>> > > > > Great, I have queued it up. > > Spoke too soon. Andy, did you compile this? > > drivers/nvme/host/core.c: In function ‘nvme_init_identify’: > drivers/nvme/host/core.c:1524:6: error: ‘force_apst’ undeclared (first use in this function) > if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) { > ^~~~~~~~~~ > drivers/nvme/host/core.c:1524:6: note: each undeclared identifier is reported only once for each function it appears in > > No, you did not. > > > I compiled the end result but not the middle. D'oh. Better version coming in an hour or two. This is from the end-result. I think you compiled the previous series, but not the revised v2 one you sent out. Because none of those patches add force_apst. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] nvme APST quirk updates, take two 2017-04-20 17:08 ` Jens Axboe @ 2017-04-20 18:56 ` Andy Lutomirski 0 siblings, 0 replies; 10+ messages in thread From: Andy Lutomirski @ 2017-04-20 18:56 UTC (permalink / raw) To: Jens Axboe Cc: linux-nvme, Keith Busch, Kai-Heng Feng, Christoph Hellwig, linux-kernel, Sagi Grimberg On Thu, Apr 20, 2017 at 10:08 AM, Jens Axboe <axboe@kernel.dk> wrote: > On 04/20/2017 11:06 AM, Andy Lutomirski wrote: >> >> >> On Apr 20, 2017 9:38 AM, "Jens Axboe" <axboe@kernel.dk <mailto:axboe@kernel.dk>> wrote: >> >> On 04/20/2017 10:30 AM, Jens Axboe wrote: >> > On 04/20/2017 10:29 AM, Keith Busch wrote: >> >> On Thu, Apr 20, 2017 at 07:15:15PM +0300, Sagi Grimberg wrote: >> >>> >> >>>>> Hi Jens- >> >>>>> >> >>>>> These are just the quirk updates, split out. The patches are >> >>>>> unchanged. >> >>>>> >> >>>>> I think that, even if we want to apply a broader quirk for 4.11, we >> >>>>> should still apply these so that we can cleanly revert the broader >> >>>>> quirk later. IOW, let's get the known regressions fixed before we >> >>>>> get too excited about the unknown regressions. >> >>>> >> >>>> This looks good to me, and 4.11 appropriate. I'll expedite this >> >>>> through the block tree, if Keith/Sagi/Christoph agrees on this >> >>>> being the right approach for 4.11. >> >>> >> >>> I'm perfectly fine with this going to 4.11 >> >> >> >> All good with me as well. >> >> >> >> Reviewed-by: Keith Busch <keith.busch@intel.com <mailto:keith.busch@intel.com>> >> > >> > Great, I have queued it up. >> >> Spoke too soon. Andy, did you compile this? >> >> drivers/nvme/host/core.c: In function ‘nvme_init_identify’: >> drivers/nvme/host/core.c:1524:6: error: ‘force_apst’ undeclared (first use in this function) >> if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) { >> ^~~~~~~~~~ >> drivers/nvme/host/core.c:1524:6: note: each undeclared identifier is reported only once for each function it appears in >> >> No, you did not. >> >> >> I compiled the end result but not the middle. D'oh. Better version coming in an hour or two. > > This is from the end-result. I think you compiled the previous series, > but not the revised v2 one you sent out. Because none of those patches > add force_apst. I was differently dumb. I did git rebase -i and rearranged everything, but I did git rebase --continue before running make, so I tested the buggy patch with the patch that added force_apst on top. Anyway, this time I'm going to compile, boot, and play with the exact patches I send before I send them. (Sadly, removing the debug patch from the series has made this type of testing harder. Oh well.) --Andy ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-20 18:56 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20 16:11 [PATCH v2 0/2] nvme APST quirk updates, take two Andy Lutomirski
2017-04-20 16:11 ` [PATCH v2 1/2] nvme: Adjust the Samsung APST quirk Andy Lutomirski
2017-04-20 16:11 ` [PATCH v2 2/2] nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA" Andy Lutomirski
2017-04-20 16:13 ` [PATCH v2 0/2] nvme APST quirk updates, take two Jens Axboe
2017-04-20 16:15 ` Sagi Grimberg
2017-04-20 16:29 ` Keith Busch
2017-04-20 16:30 ` Jens Axboe
2017-04-20 16:38 ` Jens Axboe
[not found] ` <CALCETrUQ_npTbm7eEeoFJP3V9y-keq9bjS0hD+t=k-AXSDMteA@mail.gmail.com>
2017-04-20 17:08 ` Jens Axboe
2017-04-20 18:56 ` Andy Lutomirski
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).