nvme: change cq_vector to a bool and set to true after request_irq

PATCH New Sandeep Mann 4 participants 6 messages Apr 21, 2017 – May 10, 2017 NVMe

Share & tools

Sandeep Mann

author

2017-04-21 23:24 UTC #0 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

Currently nvme_create_queue sets nvmeq->cq_vector early, before calling adapter_alloc_cq/sq. And once the queues have been instantiated, the last step allocates the IRQ (request_irq).

Both adapter_alloc_cq/sq issue ADMIN commands for queue creation. If the PCIe link fails, while either ADMIN command is in flight, the cleanup code in nvme_timeout will call nvme_suspend_queue, which calls free_irq with the vector indexed by nvmeq->cq_vector. This results free_irq on a vector that was never allocated.

This fix changes cq_vector to a bool from an integer index and stores true after the IRQ has been allocated. The index is now derived from the nvmeq->qid.

---
 drivers/nvme/host/pci.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

drivers/nvme/host/pci.c +16 ~~-12~~

index 26a5fd0..c934538 100644
@@ -127,7 +127,7 @@ struct nvme_queue {
 	dma_addr_t cq_dma_addr;
 	u32 __iomem *q_db;
 	u16 q_depth;
-	s16 cq_vector;
+	bool cq_vector;
 	u16 sq_tail;
 	u16 cq_head;
 	u16 qid;

@@ -135,6 +135,11 @@ struct nvme_queue {
 	u8 cqe_seen;
 };
 
+static inline u16 to_irq_vector(struct nvme_queue *q)
+{
+	return q->qid ? q->qid - 1 : 0;
+}
+
 /*
  * The nvme_iod describes the data in an I/O, including the list of PRP
  * entries.  You can't see it in this data structure because C doesn't let

@@ -206,7 +211,7 @@ static unsigned int nvme_cmd_size(struct nvme_dev *dev)
 
 static int nvmeq_irq(struct nvme_queue *nvmeq)
 {
-	return pci_irq_vector(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector);
+	return pci_irq_vector(to_pci_dev(nvmeq->dev->dev), to_irq_vector(nvmeq));
 }
 
 static int nvme_admin_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,

@@ -612,7 +617,7 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	blk_mq_start_request(req);
 
 	spin_lock_irq(&nvmeq->q_lock);
-	if (unlikely(nvmeq->cq_vector < 0)) {
+	if (unlikely(!nvmeq->cq_vector)) {
 		ret = BLK_MQ_RQ_QUEUE_ERROR;
 		spin_unlock_irq(&nvmeq->q_lock);
 		goto out_cleanup_iod;

@@ -712,7 +717,7 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
 		return;
 
-	if (likely(nvmeq->cq_vector >= 0))
+	if (likely(nvmeq->cq_vector))
 		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 	nvmeq->cq_head = head;
 	nvmeq->cq_phase = phase;

@@ -803,7 +808,7 @@ static int adapter_alloc_cq(struct nvme_dev *dev, u16 qid,
 	c.create_cq.cqid = cpu_to_le16(qid);
 	c.create_cq.qsize = cpu_to_le16(nvmeq->q_depth - 1);
 	c.create_cq.cq_flags = cpu_to_le16(flags);
-	c.create_cq.irq_vector = cpu_to_le16(nvmeq->cq_vector);
+	c.create_cq.irq_vector = cpu_to_le16(to_irq_vector(nvmeq));
 
 	return nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0);
 }

@@ -958,13 +963,13 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	int vector;
 
 	spin_lock_irq(&nvmeq->q_lock);
-	if (nvmeq->cq_vector == -1) {
+	if (!nvmeq->cq_vector) {
 		spin_unlock_irq(&nvmeq->q_lock);
 		return 1;
 	}
 	vector = nvmeq_irq(nvmeq);
 	nvmeq->dev->online_queues--;
-	nvmeq->cq_vector = -1;
+	nvmeq->cq_vector = false;
 	spin_unlock_irq(&nvmeq->q_lock);
 
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)

@@ -1063,7 +1068,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
 	nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
 	nvmeq->q_depth = depth;
 	nvmeq->qid = qid;
-	nvmeq->cq_vector = -1;
+	nvmeq->cq_vector = false;
 	dev->queues[qid] = nvmeq;
 	dev->queue_count++;

@@ -1106,7 +1111,6 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	struct nvme_dev *dev = nvmeq->dev;
 	int result;
 
-	nvmeq->cq_vector = qid - 1;
 	result = adapter_alloc_cq(dev, qid, nvmeq);
 	if (result < 0)
 		return result;

@@ -1118,6 +1122,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	result = queue_request_irq(nvmeq);
 	if (result < 0)
 		goto release_sq;
+	nvmeq->cq_vector = true;
 
 	nvme_init_queue(nvmeq, qid);
 	return result;

@@ -1235,12 +1240,11 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	if (result)
 		return result;
 
-	nvmeq->cq_vector = 0;
 	result = queue_request_irq(nvmeq);
 	if (result) {
-		nvmeq->cq_vector = -1;
 		return result;
 	}
+	nvmeq->cq_vector = true;
 
 	return result;
 }

@@ -1462,7 +1466,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 
 	result = queue_request_irq(adminq);
 	if (result) {
-		adminq->cq_vector = -1;
+		adminq->cq_vector = false;
 		return result;
 	}
 	return nvme_create_io_queues(dev);

Signature · 1 line

1.9.1

Christoph Hellwig

maintainer

2017-04-23 07:47 UTC #1 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

nvmeq_irq went away in a commit in the PCI tree, so merging this now would create a bit of a merge problem. Either we can live with the conflict or delay the patch until the first round of merges for -rc1 are done..

drivers/nvme/host/pci.c struct nvme_queue { · line 130

Quoted 1 line

drivers/nvme/host/pci.c +1 -0
+	bool cq_vector;

I think the field is badly misnamed now. Something like a flags field with an NVME_PCI_Q_IS_ALIVE flag might be more suitable now?

Sagi Grimberg

maintainer

2017-04-23 15:16 UTC #2 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

Quoted 4 lines

drivers/nvme/host/pci.c struct nvme_queue { · line 130
Quoted 1 line
drivers/nvme/host/pci.c +1 -0
+	bool cq_vector;
I think the field is badly misnamed now. Something like a flags field with an NVME_PCI_Q_IS_ALIVE flag might be more suitable now?

Agree.

Sandeep Mann

author

2017-04-24 20:25 UTC #3 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

In reply to Christoph Hellwig

Quoted 5 lines

nvmeq_irq went away in a commit in the PCI tree, so merging this now would create a bit of a merge problem. Either we can live with the conflict or delay the patch until the first round of merges for -rc1 are done..

I am not sure if I need to anything here, would you like me provide a diff on top of the PCI tree?

Quoted 4 lines

drivers/nvme/host/pci.c struct nvme_queue { · line 130
Quoted 1 line
drivers/nvme/host/pci.c +1 -0
+	bool cq_vector;
I think the field is badly misnamed now. Something like a flags field with an NVME_PCI_Q_IS_ALIVE flag might be more suitable now?

I will make this change, thanks.

Sandeep

Christoph Hellwig

maintainer

2017-04-25 07:18 UTC #4 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

Quoted 7 lines · Sandeep Mann

Quoted 5 lines
nvmeq_irq went away in a commit in the PCI tree, so merging this now would create a bit of a merge problem. Either we can live with the conflict or delay the patch until the first round of merges for -rc1 are done..
I am not sure if I need to anything here, would you like me provide a diff on top of the PCI tree?

Yes, feel free to work based on that. I think we'll have to wait until both the PCI and block/nvme trees are merged in mainline to apply it, though. And then it will be even more work to backport it to stable, sorry.

Christoph Hellwig

2017-05-10 17:16 UTC #5 lore raw

PATCH 1/1 nvme: change cq_vector to a bool and set to true after request_irq

In reply to Sandeep Mann

Can you repost the patch with the flags comment address against the current git tree from Linus?

Thanks!