[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Jens Axboe]

Some P3100 drives have a bug where they think WRRU (weighted round
robin) is always enabled, even though the host doesn't set it. Since
they think it's enabled, they also look at the submission queue
creation priority. We used to set that to MEDIUM by default,
but that was removed in commit 81c1cd98351b. This causes various
issues on that drive. Add a quick to still set MEDIUM priority
for that controller.

Fixes: 81c1cd98351b ("nvme/pci: Don't set reserved SQ create flags")
Cc: stable at vger.kernel.org
Signed-off-by: Jens Axboe <axboe at kernel.dk>

---

v3: Remember to include ctrl...
v2: Too quick a forward port, forgot to bump the quirk bit entry.

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7ded7a51c430..17d2f7cf3fed 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -84,6 +84,11 @@ enum nvme_quirks {
 	 * Supports the LighNVM command set if indicated in vs[1].
 	 */
 	NVME_QUIRK_LIGHTNVM			= (1 << 6),
+
+	/*
+	 * Set MEDIUM priority on SQ creation
+	 */
+	NVME_QUIRK_MEDIUM_PRIO_SQ		= (1 << 7),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fbc71fac6f1e..17a0190bd88f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1093,10 +1093,19 @@ static int adapter_alloc_cq(struct nvme_dev *dev, u16 qid,
 static int adapter_alloc_sq(struct nvme_dev *dev, u16 qid,
 						struct nvme_queue *nvmeq)
 {
+	struct nvme_ctrl *ctrl = &dev->ctrl;
 	struct nvme_command c;
 	int flags = NVME_QUEUE_PHYS_CONTIG;
 
 	/*
+	 * Some drives have a bug that auto-enables WRRU if MEDIUM isn't
+	 * set. Since URGENT priority is zeroes, it makes all queues
+	 * URGENT.
+	 */
+	if (ctrl->quirks & NVME_QUIRK_MEDIUM_PRIO_SQ)
+		flags |= NVME_SQ_PRIO_MEDIUM;
+
+	/*
 	 * Note: we (ab)use the fact that the prp fields survive if no data
 	 * is attached to the request.
 	 */
@@ -2701,7 +2710,8 @@ static const struct pci_device_id nvme_id_table[] = {
 		.driver_data = NVME_QUIRK_STRIPE_SIZE |
 				NVME_QUIRK_DEALLOCATE_ZEROES, },
 	{ PCI_VDEVICE(INTEL, 0xf1a5),	/* Intel 600P/P3100 */
-		.driver_data = NVME_QUIRK_NO_DEEPEST_PS },
+		.driver_data = NVME_QUIRK_NO_DEEPEST_PS |
+				NVME_QUIRK_MEDIUM_PRIO_SQ },
 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
 	{ PCI_DEVICE(0x1c58, 0x0003),	/* HGST adapter */

-- 
Jens Axboe

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Keith Busch]

On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
> Some P3100 drives have a bug where they think WRRU (weighted round
> robin) is always enabled, even though the host doesn't set it. Since
> they think it's enabled, they also look at the submission queue
> creation priority. We used to set that to MEDIUM by default,
> but that was removed in commit 81c1cd98351b. This causes various
> issues on that drive. Add a quick to still set MEDIUM priority
> for that controller.
> 
> Fixes: 81c1cd98351b ("nvme/pci: Don't set reserved SQ create flags")
> Cc: stable at vger.kernel.org
> Signed-off-by: Jens Axboe <axboe at kernel.dk>

Thanks, applied to nvme-4.18.

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Christoph Hellwig]

On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
> Some P3100 drives have a bug where they think WRRU (weighted round
> robin) is always enabled, even though the host doesn't set it. Since
> they think it's enabled, they also look at the submission queue
> creation priority. We used to set that to MEDIUM by default,
> but that was removed in commit 81c1cd98351b. This causes various
> issues on that drive. Add a quick to still set MEDIUM priority
> for that controller.

And this really isn't something that could be fixed with a trivial
firmware uptodate?  I'd really hate to pile on hacks like this if we
can somehow avoid it.

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Keith Busch]

On Tue, May 08, 2018@09:40:14PM -0700, Christoph Hellwig wrote:
> On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
> > Some P3100 drives have a bug where they think WRRU (weighted round
> > robin) is always enabled, even though the host doesn't set it. Since
> > they think it's enabled, they also look at the submission queue
> > creation priority. We used to set that to MEDIUM by default,
> > but that was removed in commit 81c1cd98351b. This causes various
> > issues on that drive. Add a quick to still set MEDIUM priority
> > for that controller.
> 
> And this really isn't something that could be fixed with a trivial
> firmware uptodate?  I'd really hate to pile on hacks like this if we
> can somehow avoid it.

Agreed, we don't like resorting to this option; we have only 32 entries
in the quirk table before we're out of bits. In this case, though, all
device side options for this issue were explored before considering a
driver-side work-around.

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Jens Axboe]

On 5/9/18 8:05 AM, Keith Busch wrote:
> On Tue, May 08, 2018@09:40:14PM -0700, Christoph Hellwig wrote:
>> On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
>>> Some P3100 drives have a bug where they think WRRU (weighted round
>>> robin) is always enabled, even though the host doesn't set it. Since
>>> they think it's enabled, they also look at the submission queue
>>> creation priority. We used to set that to MEDIUM by default,
>>> but that was removed in commit 81c1cd98351b. This causes various
>>> issues on that drive. Add a quick to still set MEDIUM priority
>>> for that controller.
>>
>> And this really isn't something that could be fixed with a trivial
>> firmware uptodate?  I'd really hate to pile on hacks like this if we
>> can somehow avoid it.
> 
> Agreed, we don't like resorting to this option; we have only 32 entries
> in the quirk table before we're out of bits. In this case, though, all
> device side options for this issue were explored before considering a
> driver-side work-around.

Indeed, this has to be a software work-around, unfortunately.

-- 
Jens Axboe

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Keith Busch]

On Tue, May 08, 2018@10:35:39AM -0600, Keith Busch wrote:
> On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
> > Some P3100 drives have a bug where they think WRRU (weighted round
> > robin) is always enabled, even though the host doesn't set it. Since
> > they think it's enabled, they also look at the submission queue
> > creation priority. We used to set that to MEDIUM by default,
> > but that was removed in commit 81c1cd98351b. This causes various
> > issues on that drive. Add a quick to still set MEDIUM priority
> > for that controller.
> > 
> > Fixes: 81c1cd98351b ("nvme/pci: Don't set reserved SQ create flags")
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Jens Axboe <axboe at kernel.dk>
> 
> Thanks, applied to nvme-4.18.

Actually, any concern if I apply for the next 4.17-rc instead? I know
this breakage isn't all that recent, but there seems to be heightened
visibility on getting this work-around to users.

[PATCH v3] nvme: add quirk to force medium priority for SQ creation

[Jens Axboe]

On 5/11/18 11:02 AM, Keith Busch wrote:
> On Tue, May 08, 2018@10:35:39AM -0600, Keith Busch wrote:
>> On Tue, May 08, 2018@10:25:15AM -0600, Jens Axboe wrote:
>>> Some P3100 drives have a bug where they think WRRU (weighted round
>>> robin) is always enabled, even though the host doesn't set it. Since
>>> they think it's enabled, they also look at the submission queue
>>> creation priority. We used to set that to MEDIUM by default,
>>> but that was removed in commit 81c1cd98351b. This causes various
>>> issues on that drive. Add a quick to still set MEDIUM priority
>>> for that controller.
>>>
>>> Fixes: 81c1cd98351b ("nvme/pci: Don't set reserved SQ create flags")
>>> Cc: stable at vger.kernel.org
>>> Signed-off-by: Jens Axboe <axboe at kernel.dk>
>>
>> Thanks, applied to nvme-4.18.
> 
> Actually, any concern if I apply for the next 4.17-rc instead? I know
> this breakage isn't all that recent, but there seems to be heightened
> visibility on getting this work-around to users.

Fine with me, it's trivial and the sooner the better...

-- 
Jens Axboe

Question about stream number

[Jens Axboe]

On 5/15/18 6:46 AM, dingxiang wrote:
> Hi?Jens
>      In current NVMe driver, the stream number is set to 4:
> 
>         /ctrl->nr_streams = min_t(unsigned, ctrl->nssa, BLK_MAX_WRITE_HINTS - 1);/
> 
>    Why the number is 4 rather than /ctrl->nssa? /

Because we only expose 4 different streams through the fcntl() get/set
rw hint API.

>    There is no limit in nvme spec 1.3, can I change it  if I need ?

There is a limit, but it's much higher :-)

There's nothing preventing you from changing it. But it would have to be
from an in-kernel user right now, since we don't have a way of passing
in more than 4 separate streams from userspace as it stands.

-- 
Jens Axboe