Hi,
These are some cleanups for NVMe-pci driver, and no functional
changes, please help to review. Thanks.
Baolin Wang (5):
nvme-pci: Fix some comments issues
nvme-pci: Add a blank line after declarations
nvme-pci: Remove redundant segment validation
nvme-pci: Use the consistent return type of nvme_pci_iod_alloc_size()
nvme-pci: Use standard block status macro
drivers/nvme/host/pci.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
--
1.8.3.1
Fix comments' typo and remove whitespaces before tabs to cleanup
checkpatch errors.
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/nvme/host/pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c283e8d..a3d0c86 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1260,9 +1260,9 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
}
/*
- * Shutdown the controller immediately and schedule a reset if the
- * command was already aborted once before and still hasn't been
- * returned to the driver, or if this is the admin queue.
+ * Shutdown the controller immediately and schedule a reset if the
+ * command was already aborted once before and still hasn't been
+ * returned to the driver, or if this is the admin queue.
*/
if (!nvmeq->qid || iod->aborted) {
dev_warn(dev->ctrl.device,
@@ -2002,7 +2002,7 @@ static void nvme_calc_irq_sets(struct irq_affinity *affd, unsigned int nrirqs)
unsigned int nr_read_queues, nr_write_queues = dev->nr_write_queues;
/*
- * If there is no interupt available for queues, ensure that
+ * If there is no interrupt available for queues, ensure that
* the default queue is set to 1. The affinity set size is
* also set to one, but the irq core ignores it for this case.
*
--
1.8.3.1
We've validated the segment counts before calling nvme_map_data(),
so there is no need to validate again in nvme_pci_use_sgls() only
called from nvme_map_data().
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/nvme/host/pci.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d0e9bbf..63bfb8b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -501,9 +501,6 @@ static inline bool nvme_pci_use_sgls(struct nvme_dev *dev, struct request *req)
int nseg = blk_rq_nr_phys_segments(req);
unsigned int avg_seg_size;
- if (nseg == 0)
- return false;
-
avg_seg_size = DIV_ROUND_UP(blk_rq_payload_bytes(req), nseg);
if (!(dev->ctrl.sgls & ((1 << 0) | (1 << 1))))
--
1.8.3.1
Add a blank line after declarations to make code more readable.
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/nvme/host/pci.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a3d0c86..d0e9bbf 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1019,6 +1019,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
static irqreturn_t nvme_irq_check(int irq, void *data)
{
struct nvme_queue *nvmeq = data;
+
if (nvme_cqe_pending(nvmeq))
return IRQ_WAKE_THREAD;
return IRQ_NONE;
@@ -1401,6 +1402,7 @@ static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,
if (q_size_aligned * nr_io_queues > dev->cmb_size) {
u64 mem_per_q = div_u64(dev->cmb_size, nr_io_queues);
+
mem_per_q = round_down(mem_per_q, dev->ctrl.page_size);
q_depth = div_u64(mem_per_q, entry_size);
@@ -2875,6 +2877,7 @@ static void nvme_reset_done(struct pci_dev *pdev)
static void nvme_shutdown(struct pci_dev *pdev)
{
struct nvme_dev *dev = pci_get_drvdata(pdev);
+
nvme_disable_prepare_reset(dev, true);
}
@@ -3005,6 +3008,7 @@ static int nvme_suspend(struct device *dev)
static int nvme_simple_suspend(struct device *dev)
{
struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev));
+
return nvme_disable_prepare_reset(ndev, true);
}
--
1.8.3.1
The nvme_pci_iod_alloc_size() should return 'size_t' type to keep
consistent.
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/nvme/host/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 63bfb8b..235ba34 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -362,7 +362,7 @@ static int nvme_pci_npages_sgl(unsigned int num_seg)
return DIV_ROUND_UP(num_seg * sizeof(struct nvme_sgl_desc), PAGE_SIZE);
}
-static unsigned int nvme_pci_iod_alloc_size(struct nvme_dev *dev,
+static size_t nvme_pci_iod_alloc_size(struct nvme_dev *dev,
unsigned int size, unsigned int nseg, bool use_sgl)
{
size_t alloc_size;
--
1.8.3.1
Use standard block status macro.
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/nvme/host/pci.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 235ba34..616163a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -762,7 +762,7 @@ static blk_status_t nvme_setup_prp_simple(struct nvme_dev *dev,
cmnd->dptr.prp1 = cpu_to_le64(iod->first_dma);
if (bv->bv_len > first_prp_len)
cmnd->dptr.prp2 = cpu_to_le64(iod->first_dma + first_prp_len);
- return 0;
+ return BLK_STS_OK;
}
static blk_status_t nvme_setup_sgl_simple(struct nvme_dev *dev,
@@ -780,7 +780,7 @@ static blk_status_t nvme_setup_sgl_simple(struct nvme_dev *dev,
cmnd->dptr.sgl.addr = cpu_to_le64(iod->first_dma);
cmnd->dptr.sgl.length = cpu_to_le32(iod->dma_len);
cmnd->dptr.sgl.type = NVME_SGL_FMT_DATA_DESC << 4;
- return 0;
+ return BLK_STS_OK;
}
static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
@@ -844,7 +844,7 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
if (dma_mapping_error(dev->dev, iod->meta_dma))
return BLK_STS_IOERR;
cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
- return 0;
+ return BLK_STS_OK;
}
/*
--
1.8.3.1
Reviewed-by: Sagi Grimberg <[email protected]>
Reviewed-by: Sagi Grimberg <[email protected]>
Reviewed-by: Sagi Grimberg <[email protected]>
Reviewed-by: Sagi Grimberg <[email protected]>
On 7/2/20 7:54 PM, Baolin Wang wrote:
> Add a blank line after declarations to make code more readable.
>
> Signed-off-by: Baolin Wang<[email protected]>
> ---
Looks good to me.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
On 7/2/20 7:54 PM, Baolin Wang wrote:
> Fix comments' typo and remove whitespaces before tabs to cleanup
> checkpatch errors.
>
> Signed-off-by: Baolin Wang<[email protected]>
Looks good to me.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
On 7/2/20 7:54 PM, Baolin Wang wrote:
> We've validated the segment counts before calling nvme_map_data(),
> so there is no need to validate again in nvme_pci_use_sgls() only
> called from nvme_map_data().
>
> Signed-off-by: Baolin Wang<[email protected]>
Indeed we do call blk_rq_nr_phys_segments() in nvme_queue_rq().
Looks good to me.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
On 7/2/20 7:55 PM, Baolin Wang wrote:
> The nvme_pci_iod_alloc_size() should return 'size_t' type to keep
> consistent.
>
> Signed-off-by: Baolin Wang<[email protected]>
Looks good.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
On 7/2/20 7:55 PM, Baolin Wang wrote:
> Use standard block status macro.
>
> Signed-off-by: Baolin Wang<[email protected]>
Looks good.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
On Fri, Jul 03, 2020 at 10:49:24AM +0800, Baolin Wang wrote:
> static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
> @@ -844,7 +844,7 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
> if (dma_mapping_error(dev->dev, iod->meta_dma))
> return BLK_STS_IOERR;
> cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
> - return 0;
> + return BLK_STS_OK;
> }
This is fine, though it takes knowing that this value is 0 for the
subsequent 'if (!ret)' check to make sense. Maybe those should change to
'if (ret != BLK_STS_OK)' so the check uses the same symbol as the
return, and will always work in the unlikely event that the defines
are reordered.
On Tue, Jul 07, 2020 at 12:01:23PM -0700, Keith Busch wrote:
> On Fri, Jul 03, 2020 at 10:49:24AM +0800, Baolin Wang wrote:
> > static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
> > @@ -844,7 +844,7 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
> > if (dma_mapping_error(dev->dev, iod->meta_dma))
> > return BLK_STS_IOERR;
> > cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
> > - return 0;
> > + return BLK_STS_OK;
> > }
>
> This is fine, though it takes knowing that this value is 0 for the
> subsequent 'if (!ret)' check to make sense. Maybe those should change to
> 'if (ret != BLK_STS_OK)' so the check uses the same symbol as the
> return, and will always work in the unlikely event that the defines
> are reordered.
Okay, I will send another patch to convert to 'if (ret != BLK_STS_OK)'
validation. Thanks.
On Tue, Jul 07, 2020 at 12:01:23PM -0700, Keith Busch wrote:
> On Fri, Jul 03, 2020 at 10:49:24AM +0800, Baolin Wang wrote:
> > static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
> > @@ -844,7 +844,7 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
> > if (dma_mapping_error(dev->dev, iod->meta_dma))
> > return BLK_STS_IOERR;
> > cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
> > - return 0;
> > + return BLK_STS_OK;
> > }
>
> This is fine, though it takes knowing that this value is 0 for the
> subsequent 'if (!ret)' check to make sense. Maybe those should change to
> 'if (ret != BLK_STS_OK)' so the check uses the same symbol as the
> return, and will always work in the unlikely event that the defines
> are reordered.
If you think this version is inconsistent I'd rather drop this patch.
The assumption that 0 == BLK_STS_OK is inherent all over the code.