nvme: add quirk to force medium priority for SQ creation

PATCH Superseded Jens Axboe 3 participants 8 messages May 8, 2018 – May 15, 2018 1 message not in the archive NVMe

Share & tools

Jens Axboe

author maintainer

2018-05-08 16:25 UTC #0 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

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.

drivers/nvme/host/nvme.h +5 -0

index 7ded7a51c430..17d2f7cf3fed 100644
@@ -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),
 };
 
 /*

drivers/nvme/host/pci.c +11 -1

index fbc71fac6f1e..17a0190bd88f 100644
@@ -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 */

Signature · 1 line

Jens Axboe

Keith Busch

Applied

2018-05-08 16:35 UTC #1 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

Quoted 11 lines · 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>

Thanks, applied to nvme-4.18.

Keith Busch

2018-05-11 17:02 UTC #2 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

Quoted 9 lines · Keith Busch

Quoted 8 lines · 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.

Quoted 5 lines

Quoted 3 lines
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.

Jens Axboe

author maintainer

2018-05-11 21:05 UTC #3 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

Quoted 14 lines · Keith Busch

Quoted 13 lines · Keith Busch
Quoted 11 lines · 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>

Quoted 5 lines

Quoted 1 line
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...

Signature · 1 line

Jens Axboe

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jens Axboe

author maintainer

2018-05-15 15:23 UTC #5 lore raw

Question about stream number

Quoted 6 lines · dingxiang

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.

Quoted 1 line

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.

Signature · 1 line

Jens Axboe

Christoph Hellwig

2018-05-09 04:40 UTC #6 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

In reply to Jens Axboe

Quoted 7 lines · 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.

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.

Keith Busch

2018-05-09 14:05 UTC #7 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

Quoted 9 lines · Christoph Hellwig

Quoted 7 lines · 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.

Quoted 3 lines

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.

Jens Axboe

author maintainer

2018-05-09 14:16 UTC #8 lore raw

PATCH v3 1/1 nvme: add quirk to force medium priority for SQ creation

Quoted 10 lines · Keith Busch

Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · 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.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 1 line

Jens Axboe