Replace the dma_alloc_coherent() with dma_zalloc_coherent()

PATCH New Mundu 3 participants 6 messages Jul 15, 2014 – Jul 16, 2014

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Mundu

author

2014-07-15 18:11 UTC #0 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Signed-off-by Mundu <mundu2510 at gmail.com>

drivers/block/nvme-core.c +3 -4

index 28aec2d..63f3e19 100644
@@ -1295,7 +1295,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
 	if (!nvmeq->cqes)
 		goto free_nvmeq;
 
-	nvmeq->sq_cmds = dma_alloc_coherent(dmadev, SQ_SIZE(depth),
+	nvmeq->sq_cmds = dma_zalloc_coherent(dmadev, SQ_SIZE(depth),
 					&nvmeq->sq_dma_addr, GFP_KERNEL);
 	if (!nvmeq->sq_cmds)
 		goto free_cqdma;

@@ -1356,7 +1356,6 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
 	nvmeq->cq_phase = 1;
 	nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
 	memset(nvmeq->cmdid_data, 0, extra);
-	memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
 	nvme_cancel_ios(nvmeq, false);
 	nvmeq->q_suspended = 0;
 	dev->online_queues++;

@@ -1652,7 +1651,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			goto unmap;
 		}
 
-		meta_mem = dma_alloc_coherent(&dev->pci_dev->dev, meta_len,
+		meta_mem = dma_zalloc_coherent(&dev->pci_dev->dev, meta_len,
 						&meta_dma_addr, GFP_KERNEL);
 		if (!meta_mem) {
 			status = -ENOMEM;

@@ -2293,7 +2292,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	dma_addr_t dma_addr;
 	int shift = NVME_CAP_MPSMIN(readq(&dev->bar->cap)) + 12;
 
-	mem = dma_alloc_coherent(&pdev->dev, 8192, &dma_addr, GFP_KERNEL);
+	mem = dma_zalloc_coherent(&pdev->dev, 8192, &dma_addr, GFP_KERNEL);
 	if (!mem)
 		return -ENOMEM;

drivers/block/nvme-scsi.c +12 ~~-13~~

index a4cd6d6..ce08fed 100644
@@ -682,7 +682,7 @@ static int nvme_trans_standard_inquiry_page(struct nvme_ns *ns,
 	u8 cmdque = 0x01 << 1;
 	u8 fw_offset = sizeof(dev->firmware_rev);
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 				&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -786,7 +786,7 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	int xfer_len;
 	__be32 tmp_id = cpu_to_be32(ns->ns_id);
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 					&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -868,7 +868,7 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		goto out_mem;
 	}
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 							&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -1004,7 +1004,7 @@ static int nvme_trans_log_info_exceptions(struct nvme_ns *ns,
 		goto out_mem;
 	}
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev,
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev,
 					sizeof(struct nvme_smart_log),
 					&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {

@@ -1072,7 +1072,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		goto out_mem;
 	}
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev,
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev,
 					sizeof(struct nvme_smart_log),
 					&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {

@@ -1175,7 +1175,7 @@ static int nvme_trans_fill_blk_desc(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	else if (llbaa > 0 && len < MODE_PAGE_LLBAA_BLK_DES_LEN)
 		return SNTI_INTERNAL_ERROR;
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 							&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -1460,7 +1460,7 @@ static int nvme_trans_power_state(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	unsigned ps_desired = 0;
 
 	/* NVMe Controller Identify */
-	mem = dma_alloc_coherent(&dev->pci_dev->dev,
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev,
 				sizeof(struct nvme_id_ctrl),
 				&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {

@@ -1786,7 +1786,7 @@ static int nvme_trans_fmt_set_blk_size_count(struct nvme_ns *ns,
 	 */
 
 	if (ns->mode_select_num_blocks == 0 || ns->mode_select_block_len == 0) {
-		mem = dma_alloc_coherent(&dev->pci_dev->dev,
+		mem = dma_zalloc_coherent(&dev->pci_dev->dev,
 			sizeof(struct nvme_id_ns), &dma_addr, GFP_KERNEL);
 		if (mem == NULL) {
 			res = -ENOMEM;

@@ -1894,7 +1894,7 @@ static int nvme_trans_fmt_send_cmd(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	struct nvme_command c;
 
 	/* Loop thru LBAF's in id_ns to match reqd lbaf, put in cdw10 */
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 							&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -2447,7 +2447,7 @@ static int nvme_trans_read_capacity(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		resp_size = READ_CAP_16_RESP_SIZE;
 	}
 
-	mem = dma_alloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
+	mem = dma_zalloc_coherent(&dev->pci_dev->dev, sizeof(struct nvme_id_ns),
 							&dma_addr, GFP_KERNEL);
 	if (mem == NULL) {
 		res = -ENOMEM;

@@ -2510,7 +2510,7 @@ static int nvme_trans_report_luns(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		goto out;
 	} else {
 		/* NVMe Controller Identify */
-		mem = dma_alloc_coherent(&dev->pci_dev->dev,
+		mem = dma_zalloc_coherent(&dev->pci_dev->dev,
 					sizeof(struct nvme_id_ctrl),
 					&dma_addr, GFP_KERNEL);
 		if (mem == NULL) {

@@ -2876,7 +2876,7 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		goto out;
 	}
 
-	range = dma_alloc_coherent(&dev->pci_dev->dev, ndesc * sizeof(*range),
+	range = dma_zalloc_coherent(&dev->pci_dev->dev, ndesc * sizeof(*range),
 							&dma_addr, GFP_KERNEL);
 	if (!range)
 		goto out;

@@ -2884,7 +2884,6 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	for (i = 0; i < ndesc; i++) {
 		range[i].nlb = cpu_to_le32(be32_to_cpu(plist->desc[i].nlb));
 		range[i].slba = cpu_to_le64(be64_to_cpu(plist->desc[i].slba));
-		range[i].cattr = 0;
 	}
 
 	memset(&c, 0, sizeof(c));

Signature · 1 line

1.9.1

Matthew Wilcox

2014-07-15 18:28 UTC #1 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Quoted 1 line · Mundu

Signed-off-by Mundu <mundu2510 at gmail.com>

why?

mundu agarwal

author

2014-07-16 05:57 UTC #2 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Willy,

There is patch merged for dma_zalloc_coherent for cqee to avoid the memset, where as memset still happing for same in nvme_init_queue. I guess this is not needed, and rest are to maintain the consistancy. I feel instead of mix use of the apis, should be use as clean/single one. There are couple of places during initialization variables are reseting to 0 after allocating the memory using zalloc which we can avoid. In case this patch ok then my next task will be doing the cleanup without any impacting the current functionality.

Regards,
Mundu

Quoted 4 lines · Matthew Wilcox

Quoted 1 line · Mundu
Signed-off-by Mundu <mundu2510 at gmail.com>
why?

Matthew Wilcox

2014-07-16 14:16 UTC #3 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Quoted 10 lines · mundu agarwal

Willy,
There is patch merged for dma_zalloc_coherent for cqee to avoid the memset, where as memset still happing for same in nvme_init_queue. I guess this is not needed, and rest are to maintain the consistancy. I feel instead of mix use of the apis, should be use as clean/single one. There are couple of places during initialization variables are reseting to 0 after allocating the memory using zalloc which we can avoid. In case this patch ok then my next task will be doing the cleanup without any impacting the current functionality.

Better not to guess. Joe's quite careful to only do the ones that are obviously correct. I believe your patches to be incorrect, since some of the places don't need to be zeroed (eg SQEs are zeroed before each use).

In the specific case of nvme_init_queue() zeroing the memory, I think it's necessary on that path because that path is called on resume from D3 and we need to reinitialise the CQ state to all zeroes. Keith should be able to confirm that.

Keith Busch

2014-07-16 14:52 UTC #4 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Quoted 12 lines · Matthew Wilcox

Quoted 10 lines · mundu agarwal
Willy,
There is patch merged for dma_zalloc_coherent for cqee to avoid the memset, where as memset still happing for same in nvme_init_queue. I guess this is not needed, and rest are to maintain the consistancy. I feel instead of mix use of the apis, should be use as clean/single one. There are couple of places during initialization variables are reseting to 0 after allocating the memory using zalloc which we can avoid. In case this patch ok then my next task will be doing the cleanup without any impacting the current functionality.

Quoted 8 lines

Better not to guess. Joe's quite careful to only do the ones that are obviously correct. I believe your patches to be incorrect, since some of the places don't need to be zeroed (eg SQEs are zeroed before each use).
In the specific case of nvme_init_queue() zeroing the memory, I think it's necessary on that path because that path is called on resume from D3 and we need to reinitialise the CQ state to all zeroes. Keith should be able to confirm that.

Yes, we have to clear the cmdid data and completion queues in init for the resume and reset cases otherwise we would leak command id's that were active at the time we shut down the device, or potentially read a stale cq "phase" and misinterpret that as a new completion.

mundu agarwal

author

2014-07-16 17:08 UTC #5 lore raw

PATCH 1/1 Replace the dma_alloc_coherent() with dma_zalloc_coherent()

Thanks Keith/Willy for pointing out the missing link.

Quoted 30 lines · Keith Busch

Quoted 15 lines · Matthew Wilcox
Quoted 11 lines · mundu agarwal
Willy,
There is patch merged for dma_zalloc_coherent for cqee to avoid the memset, where as memset still happing for same in nvme_init_queue. I guess this is not needed, and rest are to maintain the consistancy. I feel instead of mix use of the apis, should be use as clean/single one. There are couple of places during initialization variables are reseting to 0 after allocating the memory using zalloc which we can avoid. In case this patch ok then my next task will be doing the cleanup without any impacting the current functionality.
Quoted 8 lines
Better not to guess. Joe's quite careful to only do the ones that are obviously correct. I believe your patches to be incorrect, since some of the places don't need to be zeroed (eg SQEs are zeroed before each use).
In the specific case of nvme_init_queue() zeroing the memory, I think it's necessary on that path because that path is called on resume from D3 and we need to reinitialise the CQ state to all zeroes. Keith should be able to confirm that.
Yes, we have to clear the cmdid data and completion queues in init for the resume and reset cases otherwise we would leak command id's that were active at the time we shut down the device, or potentially read a stale cq "phase" and misinterpret that as a new completion.