[PATCH v2 0/8] Introduce sgl_alloc() and sgl_free()

[PATCH v2 0/8] Introduce sgl_alloc() and sgl_free()

[Bart Van Assche]

Hello Jens,

As you know there are multiple drivers that both allocate a scatter/gather
list and populate that list with pages. This patch series moves the code for
allocating and freeing such scatterlists from these drivers into
lib/scatterlist.c. Please consider this patch series for kernel v4.15.

Notes:
- Only the ib_srpt driver changes have been tested but none of the other
  drivers have been retested.
- The next step is to introduce a caching mechanism for these scatterlists
  and make the nvmet/rdma and SCSI target drivers use that caching mechanism
  since for these drivers sgl allocation occurs in the hot path.

Changes compared to v1:
- Moved the sgl_alloc*() and sgl_free() functions from a new source file into
  lib/scatterlist.c.
- Changed the argument order for the sgl_alloc*() functions such that the
  (pointer to) the output argument comes last.

Thanks,

Bart.

Bart Van Assche (8):
  lib/scatterlist: Introduce sgl_alloc() and sgl_free()
  crypto: scompress - use sgl_alloc() and sgl_free()
  nvmet/fc: Use sgl_alloc() and sgl_free()
  nvmet/rdma: Use sgl_alloc() and sgl_free()
  target: Use sgl_alloc_order() and sgl_free()
  scsi/ipr: Use sgl_alloc_order() and sgl_free_order()
  scsi/pmcraid: Remove an unused structure member
  scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()

 crypto/Kconfig                         |   1 +
 crypto/scompress.c                     |  51 +---------------
 drivers/nvme/target/Kconfig            |   2 +
 drivers/nvme/target/fc.c               |  36 +----------
 drivers/nvme/target/rdma.c             |  63 ++------------------
 drivers/scsi/Kconfig                   |   2 +
 drivers/scsi/ipr.c                     |  49 +++------------
 drivers/scsi/ipr.h                     |   2 +-
 drivers/scsi/pmcraid.c                 |  43 ++------------
 drivers/scsi/pmcraid.h                 |   3 +-
 drivers/target/Kconfig                 |   1 +
 drivers/target/target_core_transport.c |  46 ++-------------
 include/linux/scatterlist.h            |  10 ++++
 lib/Kconfig                            |   4 ++
 lib/scatterlist.c                      | 105 +++++++++++++++++++++++++++++++++
 15 files changed, 151 insertions(+), 267 deletions(-)

-- 
2.14.2

[PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Bart Van Assche]

Many kernel drivers contain code that allocates and frees both a
scatterlist and the pages that populate that scatterlist.
Introduce functions in lib/scatterlist.c that perform these tasks
instead of duplicating this functionality in multiple drivers.
Only include these functions in the build if CONFIG_SGL_ALLOC=y
to avoid that the kernel size increases if this functionality is
not used.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 include/linux/scatterlist.h |  10 +++++
 lib/Kconfig                 |   4 ++
 lib/scatterlist.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 4b3286ac60c8..3642511918d5 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -266,6 +266,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
 	unsigned long offset, unsigned long size,
 	gfp_t gfp_mask);
 
+#ifdef CONFIG_SGL_ALLOC
+struct scatterlist *sgl_alloc_order(unsigned long long length,
+				    unsigned int order, bool chainable,
+				    gfp_t gfp, unsigned int *nent_p);
+struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+			      unsigned int *nent_p);
+void sgl_free_order(struct scatterlist *sgl, int order);
+void sgl_free(struct scatterlist *sgl);
+#endif /* CONFIG_SGL_ALLOC */
+
 size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
 		      size_t buflen, off_t skip, bool to_buffer);
 
diff --git a/lib/Kconfig b/lib/Kconfig
index b1445b22a6de..8396c4cfa1ab 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -413,6 +413,10 @@ config HAS_DMA
 	depends on !NO_DMA
 	default y
 
+config SGL_ALLOC
+	bool
+	default n
+
 config DMA_NOOP_OPS
 	bool
 	depends on HAS_DMA && (!64BIT || ARCH_DMA_ADDR_T_64BIT)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index be7b4dd6b68d..e2b5a78ceb44 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -433,6 +433,111 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
 }
 EXPORT_SYMBOL(sg_alloc_table_from_pages);
 
+#ifdef CONFIG_SGL_ALLOC
+
+/**
+ * sgl_alloc_order - allocate a scatterlist and its pages
+ * @length: Length in bytes of the scatterlist. Must be at least one
+ * @order: Second argument for alloc_pages()
+ * @chainable: Whether or not to allocate an extra element in the scatterlist
+ *	for scatterlist chaining purposes
+ * @gfp: Memory allocation flags
+ * @nent_p: [out] Number of entries in the scatterlist that have pages
+ *
+ * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
+ */
+struct scatterlist *sgl_alloc_order(unsigned long long length,
+				    unsigned int order, bool chainable,
+				    gfp_t gfp, unsigned int *nent_p)
+{
+	struct scatterlist *sgl, *sg;
+	struct page *page;
+	unsigned int nent, nalloc;
+	u32 elem_len;
+
+	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
+	/* Check for integer overflow */
+	if (length > (nent << (PAGE_SHIFT + order)))
+		return NULL;
+	nalloc = nent;
+	if (chainable) {
+		/* Check for integer overflow */
+		if (nalloc + 1 < nalloc)
+			return NULL;
+		nalloc++;
+	}
+	sgl = kmalloc_array(nalloc, sizeof(struct scatterlist),
+			    (gfp & ~GFP_DMA) | __GFP_ZERO);
+	if (!sgl)
+		return NULL;
+
+	sg_init_table(sgl, nent);
+	sg = sgl;
+	while (length) {
+		elem_len = min_t(u64, length, PAGE_SIZE << order);
+		page = alloc_pages(gfp, order);
+		if (!page) {
+			sgl_free(sgl);
+			return NULL;
+		}
+
+		sg_set_page(sg, page, elem_len, 0);
+		length -= elem_len;
+		sg = sg_next(sg);
+	}
+	WARN_ON_ONCE(sg);
+	if (nent_p)
+		*nent_p = nent;
+	return sgl;
+}
+EXPORT_SYMBOL(sgl_alloc_order);
+
+/**
+ * sgl_alloc - allocate a scatterlist and its pages
+ * @length: Length in bytes of the scatterlist
+ * @gfp: Memory allocation flags
+ * @nent_p: [out] Number of entries in the scatterlist
+ *
+ * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
+ */
+struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+			      unsigned int *nent_p)
+{
+	return sgl_alloc_order(length, 0, false, gfp, nent_p);
+}
+EXPORT_SYMBOL(sgl_alloc);
+
+/**
+ * sgl_free_order - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ * @order: Second argument for __free_pages()
+ */
+void sgl_free_order(struct scatterlist *sgl, int order)
+{
+	struct scatterlist *sg;
+	struct page *page;
+
+	for (sg = sgl; sg; sg = sg_next(sg)) {
+		page = sg_page(sg);
+		if (page)
+			__free_pages(page, order);
+	}
+	kfree(sgl);
+}
+EXPORT_SYMBOL(sgl_free_order);
+
+/**
+ * sgl_free - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ */
+void sgl_free(struct scatterlist *sgl)
+{
+	sgl_free_order(sgl, 0);
+}
+EXPORT_SYMBOL(sgl_free);
+
+#endif /* CONFIG_SGL_ALLOC */
+
 void __sg_page_iter_start(struct sg_page_iter *piter,
 			  struct scatterlist *sglist, unsigned int nents,
 			  unsigned long pgoffset)
-- 
2.14.2

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/Kconfig     |  1 +
 crypto/scompress.c | 51 ++-------------------------------------------------
 2 files changed, 3 insertions(+), 49 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 0a121f9ddf8e..a0667dd284ff 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -105,6 +105,7 @@ config CRYPTO_KPP
 config CRYPTO_ACOMP2
 	tristate
 	select CRYPTO_ALGAPI2
+	select SGL_ALLOC
 
 config CRYPTO_ACOMP
 	tristate
diff --git a/crypto/scompress.c b/crypto/scompress.c
index 2075e2c4e7df..968bbcf65c94 100644
--- a/crypto/scompress.c
+++ b/crypto/scompress.c
@@ -140,53 +140,6 @@ static int crypto_scomp_init_tfm(struct crypto_tfm *tfm)
 	return ret;
 }
 
-static void crypto_scomp_sg_free(struct scatterlist *sgl)
-{
-	int i, n;
-	struct page *page;
-
-	if (!sgl)
-		return;
-
-	n = sg_nents(sgl);
-	for_each_sg(sgl, sgl, n, i) {
-		page = sg_page(sgl);
-		if (page)
-			__free_page(page);
-	}
-
-	kfree(sgl);
-}
-
-static struct scatterlist *crypto_scomp_sg_alloc(size_t size, gfp_t gfp)
-{
-	struct scatterlist *sgl;
-	struct page *page;
-	int i, n;
-
-	n = ((size - 1) >> PAGE_SHIFT) + 1;
-
-	sgl = kmalloc_array(n, sizeof(struct scatterlist), gfp);
-	if (!sgl)
-		return NULL;
-
-	sg_init_table(sgl, n);
-
-	for (i = 0; i < n; i++) {
-		page = alloc_page(gfp);
-		if (!page)
-			goto err;
-		sg_set_page(sgl + i, page, PAGE_SIZE, 0);
-	}
-
-	return sgl;
-
-err:
-	sg_mark_end(sgl + i);
-	crypto_scomp_sg_free(sgl);
-	return NULL;
-}
-
 static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
 {
 	struct crypto_acomp *tfm = crypto_acomp_reqtfm(req);
@@ -220,7 +173,7 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
 					      scratch_dst, &req->dlen, *ctx);
 	if (!ret) {
 		if (!req->dst) {
-			req->dst = crypto_scomp_sg_alloc(req->dlen, GFP_ATOMIC);
+			req->dst = sgl_alloc(req->dlen, GFP_ATOMIC, NULL);
 			if (!req->dst)
 				goto out;
 		}
@@ -274,7 +227,7 @@ int crypto_init_scomp_ops_async(struct crypto_tfm *tfm)
 
 	crt->compress = scomp_acomp_compress;
 	crt->decompress = scomp_acomp_decompress;
-	crt->dst_free = crypto_scomp_sg_free;
+	crt->dst_free = sgl_free;
 	crt->reqsize = sizeof(void *);
 
 	return 0;
-- 
2.14.2

[PATCH v2 3/8] nvmet/fc: Use sgl_alloc() and sgl_free()

[Bart Van Assche]

Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: James Smart <james.smart@broadcom.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/Kconfig |  1 +
 drivers/nvme/target/fc.c    | 36 ++----------------------------------
 2 files changed, 3 insertions(+), 34 deletions(-)

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 03e4ab65fe77..4d9715630e21 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -39,6 +39,7 @@ config NVME_TARGET_FC
 	tristate "NVMe over Fabrics FC target driver"
 	depends on NVME_TARGET
 	depends on HAS_DMA
+	select SGL_ALLOC
 	help
 	  This enables the NVMe FC target support, which allows exporting NVMe
 	  devices over FC.
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 58e010bdda3e..20f96038c272 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1683,31 +1683,12 @@ static int
 nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
 {
 	struct scatterlist *sg;
-	struct page *page;
 	unsigned int nent;
-	u32 page_len, length;
-	int i = 0;
 
-	length = fod->total_length;
-	nent = DIV_ROUND_UP(length, PAGE_SIZE);
-	sg = kmalloc_array(nent, sizeof(struct scatterlist), GFP_KERNEL);
+	sg = sgl_alloc(fod->total_length, GFP_KERNEL, &nent);
 	if (!sg)
 		goto out;
 
-	sg_init_table(sg, nent);
-
-	while (length) {
-		page_len = min_t(u32, length, PAGE_SIZE);
-
-		page = alloc_page(GFP_KERNEL);
-		if (!page)
-			goto out_free_pages;
-
-		sg_set_page(&sg[i], page, page_len, 0);
-		length -= page_len;
-		i++;
-	}
-
 	fod->data_sg = sg;
 	fod->data_sg_cnt = nent;
 	fod->data_sg_cnt = fc_dma_map_sg(fod->tgtport->dev, sg, nent,
@@ -1717,14 +1698,6 @@ nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
 
 	return 0;
 
-out_free_pages:
-	while (i > 0) {
-		i--;
-		__free_page(sg_page(&sg[i]));
-	}
-	kfree(sg);
-	fod->data_sg = NULL;
-	fod->data_sg_cnt = 0;
 out:
 	return NVME_SC_INTERNAL;
 }
@@ -1732,18 +1705,13 @@ nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
 static void
 nvmet_fc_free_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
 {
-	struct scatterlist *sg;
-	int count;
-
 	if (!fod->data_sg || !fod->data_sg_cnt)
 		return;
 
 	fc_dma_unmap_sg(fod->tgtport->dev, fod->data_sg, fod->data_sg_cnt,
 				((fod->io_dir == NVMET_FCP_WRITE) ?
 					DMA_FROM_DEVICE : DMA_TO_DEVICE));
-	for_each_sg(fod->data_sg, sg, fod->data_sg_cnt, count)
-		__free_page(sg_page(sg));
-	kfree(fod->data_sg);
+	sgl_free(fod->data_sg);
 	fod->data_sg = NULL;
 	fod->data_sg_cnt = 0;
 }
-- 
2.14.2

[PATCH v2 4/8] nvmet/rdma: Use sgl_alloc() and sgl_free()

[Bart Van Assche]

Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/Kconfig |  1 +
 drivers/nvme/target/rdma.c  | 63 +++------------------------------------------
 2 files changed, 5 insertions(+), 59 deletions(-)

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 4d9715630e21..5f4f8b16685f 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -29,6 +29,7 @@ config NVME_TARGET_RDMA
 	tristate "NVMe over Fabrics RDMA target support"
 	depends on INFINIBAND
 	depends on NVME_TARGET
+	select SGL_ALLOC
 	help
 	  This enables the NVMe RDMA target support, which allows exporting NVMe
 	  devices over RDMA.
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 76d2bb793afe..363d44c10b68 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -185,59 +185,6 @@ nvmet_rdma_put_rsp(struct nvmet_rdma_rsp *rsp)
 	spin_unlock_irqrestore(&rsp->queue->rsps_lock, flags);
 }
 
-static void nvmet_rdma_free_sgl(struct scatterlist *sgl, unsigned int nents)
-{
-	struct scatterlist *sg;
-	int count;
-
-	if (!sgl || !nents)
-		return;
-
-	for_each_sg(sgl, sg, nents, count)
-		__free_page(sg_page(sg));
-	kfree(sgl);
-}
-
-static int nvmet_rdma_alloc_sgl(struct scatterlist **sgl, unsigned int *nents,
-		u32 length)
-{
-	struct scatterlist *sg;
-	struct page *page;
-	unsigned int nent;
-	int i = 0;
-
-	nent = DIV_ROUND_UP(length, PAGE_SIZE);
-	sg = kmalloc_array(nent, sizeof(struct scatterlist), GFP_KERNEL);
-	if (!sg)
-		goto out;
-
-	sg_init_table(sg, nent);
-
-	while (length) {
-		u32 page_len = min_t(u32, length, PAGE_SIZE);
-
-		page = alloc_page(GFP_KERNEL);
-		if (!page)
-			goto out_free_pages;
-
-		sg_set_page(&sg[i], page, page_len, 0);
-		length -= page_len;
-		i++;
-	}
-	*sgl = sg;
-	*nents = nent;
-	return 0;
-
-out_free_pages:
-	while (i > 0) {
-		i--;
-		__free_page(sg_page(&sg[i]));
-	}
-	kfree(sg);
-out:
-	return NVME_SC_INTERNAL;
-}
-
 static int nvmet_rdma_alloc_cmd(struct nvmet_rdma_device *ndev,
 			struct nvmet_rdma_cmd *c, bool admin)
 {
@@ -484,7 +431,7 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)
 	}
 
 	if (rsp->req.sg != &rsp->cmd->inline_sg)
-		nvmet_rdma_free_sgl(rsp->req.sg, rsp->req.sg_cnt);
+		sgl_free(rsp->req.sg);
 
 	if (unlikely(!list_empty_careful(&queue->rsp_wr_wait_list)))
 		nvmet_rdma_process_wr_wait_list(queue);
@@ -620,16 +567,14 @@ static u16 nvmet_rdma_map_sgl_keyed(struct nvmet_rdma_rsp *rsp,
 	u32 len = get_unaligned_le24(sgl->length);
 	u32 key = get_unaligned_le32(sgl->key);
 	int ret;
-	u16 status;
 
 	/* no data command? */
 	if (!len)
 		return 0;
 
-	status = nvmet_rdma_alloc_sgl(&rsp->req.sg, &rsp->req.sg_cnt,
-			len);
-	if (status)
-		return status;
+	rsp->req.sg = sgl_alloc(len, GFP_KERNEL, &rsp->req.sg_cnt);
+	if (!rsp->req.sg)
+		return NVME_SC_INTERNAL;
 
 	ret = rdma_rw_ctx_init(&rsp->rw, cm_id->qp, cm_id->port_num,
 			rsp->req.sg, rsp->req.sg_cnt, 0, addr, key,
-- 
2.14.2

[PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Bart Van Assche]

Use the sgl_alloc_order() and sgl_free() functions instead of open
coding these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/target/Kconfig                 |  1 +
 drivers/target/target_core_transport.c | 46 +++-------------------------------
 2 files changed, 5 insertions(+), 42 deletions(-)

diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
index e2bc99980f75..4c44d7bed01a 100644
--- a/drivers/target/Kconfig
+++ b/drivers/target/Kconfig
@@ -5,6 +5,7 @@ menuconfig TARGET_CORE
 	select CONFIGFS_FS
 	select CRC_T10DIF
 	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
+	select SGL_ALLOC
 	default n
 	help
 	Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 836d552b0385..9bbd08be9d60 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
 
 void target_free_sgl(struct scatterlist *sgl, int nents)
 {
-	struct scatterlist *sg;
-	int count;
-
-	for_each_sg(sgl, sg, nents, count)
-		__free_page(sg_page(sg));
-
-	kfree(sgl);
+	sgl_free(sgl);
 }
 EXPORT_SYMBOL(target_free_sgl);
 
@@ -2423,42 +2417,10 @@ int
 target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
 		 bool zero_page, bool chainable)
 {
-	struct scatterlist *sg;
-	struct page *page;
-	gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
-	unsigned int nalloc, nent;
-	int i = 0;
-
-	nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
-	if (chainable)
-		nalloc++;
-	sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
-	if (!sg)
-		return -ENOMEM;
+	gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
 
-	sg_init_table(sg, nalloc);
-
-	while (length) {
-		u32 page_len = min_t(u32, length, PAGE_SIZE);
-		page = alloc_page(GFP_KERNEL | zero_flag);
-		if (!page)
-			goto out;
-
-		sg_set_page(&sg[i], page, page_len, 0);
-		length -= page_len;
-		i++;
-	}
-	*sgl = sg;
-	*nents = nent;
-	return 0;
-
-out:
-	while (i > 0) {
-		i--;
-		__free_page(sg_page(&sg[i]));
-	}
-	kfree(sg);
-	return -ENOMEM;
+	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
+	return *sgl ? 0 : -ENOMEM;
 }
 EXPORT_SYMBOL(target_alloc_sgl);
 
-- 
2.14.2

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Bart Van Assche]

Use the sgl_alloc_order() and sgl_free_order() functions instead
of open coding these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: linux-scsi@vger.kernel.org
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/scsi/Kconfig |  1 +
 drivers/scsi/ipr.c   | 49 ++++++++-----------------------------------------
 drivers/scsi/ipr.h   |  2 +-
 3 files changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 41366339b950..d11e75e76195 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1058,6 +1058,7 @@ config SCSI_IPR
 	depends on PCI && SCSI && ATA
 	select FW_LOADER
 	select IRQ_POLL
+	select SGL_ALLOC
 	---help---
 	  This driver supports the IBM Power Linux family RAID adapters.
 	  This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index f838bd73befa..6fed5db6255e 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
  **/
 static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
 {
-	int sg_size, order, bsize_elem, num_elem, i, j;
+	int sg_size, order;
 	struct ipr_sglist *sglist;
-	struct scatterlist *scatterlist;
-	struct page *page;
 
 	/* Get the minimum size per scatter/gather element */
 	sg_size = buf_len / (IPR_MAX_SGLIST - 1);
@@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
 	/* Get the actual size per element */
 	order = get_order(sg_size);
 
-	/* Determine the actual number of bytes per element */
-	bsize_elem = PAGE_SIZE * (1 << order);
-
-	/* Determine the actual number of sg entries needed */
-	if (buf_len % bsize_elem)
-		num_elem = (buf_len / bsize_elem) + 1;
-	else
-		num_elem = buf_len / bsize_elem;
-
 	/* Allocate a scatter/gather list for the DMA */
-	sglist = kzalloc(sizeof(struct ipr_sglist) +
-			 (sizeof(struct scatterlist) * (num_elem - 1)),
-			 GFP_KERNEL);
-
+	sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
 	if (sglist == NULL) {
 		ipr_trace;
 		return NULL;
 	}
-
-	scatterlist = sglist->scatterlist;
-	sg_init_table(scatterlist, num_elem);
-
 	sglist->order = order;
-	sglist->num_sg = num_elem;
-
-	/* Allocate a bunch of sg elements */
-	for (i = 0; i < num_elem; i++) {
-		page = alloc_pages(GFP_KERNEL, order);
-		if (!page) {
-			ipr_trace;
-
-			/* Free up what we already allocated */
-			for (j = i - 1; j >= 0; j--)
-				__free_pages(sg_page(&scatterlist[j]), order);
-			kfree(sglist);
-			return NULL;
-		}
-
-		sg_set_page(&scatterlist[i], page, 0, 0);
+	sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
+					      &sglist->num_sg);
+	if (!sglist->scatterlist) {
+		kfree(sglist);
+		return NULL;
 	}
 
 	return sglist;
@@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
  **/
 static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
 {
-	int i;
-
-	for (i = 0; i < sglist->num_sg; i++)
-		__free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
-
+	sgl_free_order(sglist->scatterlist, sglist->order);
 	kfree(sglist);
 }
 
diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
index c7f0e9e3cd7d..93570734cbfb 100644
--- a/drivers/scsi/ipr.h
+++ b/drivers/scsi/ipr.h
@@ -1454,7 +1454,7 @@ struct ipr_sglist {
 	u32 num_sg;
 	u32 num_dma_sg;
 	u32 buffer_len;
-	struct scatterlist scatterlist[1];
+	struct scatterlist *scatterlist;
 };
 
 enum ipr_sdt_state {
-- 
2.14.2

[PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: linux-scsi@vger.kernel.org
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Anil Ravindranath <anil_ravindranath@pmc-sierra.com>
---
 drivers/scsi/pmcraid.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
index 8bfac72a242b..44da91712115 100644
--- a/drivers/scsi/pmcraid.h
+++ b/drivers/scsi/pmcraid.h
@@ -542,7 +542,6 @@ struct pmcraid_sglist {
 	u32 order;
 	u32 num_sg;
 	u32 num_dma_sg;
-	u32 buffer_len;
 	struct scatterlist scatterlist[1];
 };
 
-- 
2.14.2

[PATCH v2 8/8] scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()

[Bart Van Assche]

Use the sgl_alloc_order() and sgl_free_order() functions instead
of open coding these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: linux-scsi@vger.kernel.org
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Anil Ravindranath <anil_ravindranath@pmc-sierra.com>
---
 drivers/scsi/Kconfig   |  1 +
 drivers/scsi/pmcraid.c | 43 ++++---------------------------------------
 drivers/scsi/pmcraid.h |  2 +-
 3 files changed, 6 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index d11e75e76195..632200ec36a6 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1576,6 +1576,7 @@ config ZFCP
 config SCSI_PMCRAID
 	tristate "PMC SIERRA Linux MaxRAID adapter support"
 	depends on PCI && SCSI && NET
+	select SGL_ALLOC
 	---help---
 	  This driver supports the PMC SIERRA MaxRAID adapters.
 
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index b4d6cd8cd1ad..95fc0352f9bb 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3232,12 +3232,7 @@ static int pmcraid_build_ioadl(
  */
 static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
 {
-	int i;
-
-	for (i = 0; i < sglist->num_sg; i++)
-		__free_pages(sg_page(&(sglist->scatterlist[i])),
-			     sglist->order);
-
+	sgl_free_order(sglist->scatterlist, sglist->order);
 	kfree(sglist);
 }
 
@@ -3254,50 +3249,20 @@ static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
 static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
 {
 	struct pmcraid_sglist *sglist;
-	struct scatterlist *scatterlist;
-	struct page *page;
-	int num_elem, i, j;
 	int sg_size;
 	int order;
-	int bsize_elem;
 
 	sg_size = buflen / (PMCRAID_MAX_IOADLS - 1);
 	order = (sg_size > 0) ? get_order(sg_size) : 0;
-	bsize_elem = PAGE_SIZE * (1 << order);
-
-	/* Determine the actual number of sg entries needed */
-	if (buflen % bsize_elem)
-		num_elem = (buflen / bsize_elem) + 1;
-	else
-		num_elem = buflen / bsize_elem;
 
 	/* Allocate a scatter/gather list for the DMA */
-	sglist = kzalloc(sizeof(struct pmcraid_sglist) +
-			 (sizeof(struct scatterlist) * (num_elem - 1)),
-			 GFP_KERNEL);
-
+	sglist = kzalloc(sizeof(struct pmcraid_sglist), GFP_KERNEL);
 	if (sglist == NULL)
 		return NULL;
 
-	scatterlist = sglist->scatterlist;
-	sg_init_table(scatterlist, num_elem);
 	sglist->order = order;
-	sglist->num_sg = num_elem;
-	sg_size = buflen;
-
-	for (i = 0; i < num_elem; i++) {
-		page = alloc_pages(GFP_KERNEL|GFP_DMA|__GFP_ZERO, order);
-		if (!page) {
-			for (j = i - 1; j >= 0; j--)
-				__free_pages(sg_page(&scatterlist[j]), order);
-			kfree(sglist);
-			return NULL;
-		}
-
-		sg_set_page(&scatterlist[i], page,
-			sg_size < bsize_elem ? sg_size : bsize_elem, 0);
-		sg_size -= bsize_elem;
-	}
+	sgl_alloc_order(buflen, order, false,
+			GFP_KERNEL | GFP_DMA | __GFP_ZERO, &sglist->num_sg);
 
 	return sglist;
 }
diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
index 44da91712115..754ef30927e2 100644
--- a/drivers/scsi/pmcraid.h
+++ b/drivers/scsi/pmcraid.h
@@ -542,7 +542,7 @@ struct pmcraid_sglist {
 	u32 order;
 	u32 num_sg;
 	u32 num_dma_sg;
-	struct scatterlist scatterlist[1];
+	struct scatterlist *scatterlist;
 };
 
 /* page D0 inquiry data of focal point resource */
-- 
2.14.2

Re: [PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Many kernel drivers contain code that allocates and frees both a
> scatterlist and the pages that populate that scatterlist.
> Introduce functions in lib/scatterlist.c that perform these tasks
> instead of duplicating this functionality in multiple drivers.
> Only include these functions in the build if CONFIG_SGL_ALLOC=y
> to avoid that the kernel size increases if this functionality is
> not used.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> ---
>  include/linux/scatterlist.h |  10 +++++
>  lib/Kconfig                 |   4 ++
>  lib/scatterlist.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Many kernel drivers contain code that allocates and frees both a
> scatterlist and the pages that populate that scatterlist.
> Introduce functions in lib/scatterlist.c that perform these tasks
> instead of duplicating this functionality in multiple drivers.
> Only include these functions in the build if CONFIG_SGL_ALLOC=y
> to avoid that the kernel size increases if this functionality is
> not used.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> ---
>  include/linux/scatterlist.h |  10 +++++
>  lib/Kconfig                 |   4 ++
>  lib/scatterlist.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  crypto/Kconfig     |  1 +
>  crypto/scompress.c | 51 ++-------------------------------------------------
>  2 files changed, 3 insertions(+), 49 deletions(-)
> Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Herbert Xu <herbert at gondor.apana.org.au>
> ---
>  crypto/Kconfig     |  1 +
>  crypto/scompress.c | 51 ++-------------------------------------------------
>  2 files changed, 3 insertions(+), 49 deletions(-)
> Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 3/8] nvmet/fc: Use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: James Smart <james.smart@broadcom.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/target/Kconfig |  1 +
>  drivers/nvme/target/fc.c    | 36 ++----------------------------------
>  2 files changed, 3 insertions(+), 34 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 3/8] nvmet/fc: Use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: James Smart <james.smart at broadcom.com>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/Kconfig |  1 +
>  drivers/nvme/target/fc.c    | 36 ++----------------------------------
>  2 files changed, 3 insertions(+), 34 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 4/8] nvmet/rdma: Use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/target/Kconfig |  1 +
>  drivers/nvme/target/rdma.c  | 63 +++------------------------------------------
>  2 files changed, 5 insertions(+), 59 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 4/8] nvmet/rdma: Use sgl_alloc() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/Kconfig |  1 +
>  drivers/nvme/target/rdma.c  | 63 +++------------------------------------------
>  2 files changed, 5 insertions(+), 59 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free() functions instead of open
> coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/target/Kconfig                 |  1 +
>  drivers/target/target_core_transport.c | 46 +++-------------------------------
>  2 files changed, 5 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index e2bc99980f75..4c44d7bed01a 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -5,6 +5,7 @@ menuconfig TARGET_CORE
>  	select CONFIGFS_FS
>  	select CRC_T10DIF
>  	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
> +	select SGL_ALLOC
>  	default n
>  	help
>  	Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 836d552b0385..9bbd08be9d60 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
>  
>  void target_free_sgl(struct scatterlist *sgl, int nents)
>  {
> -	struct scatterlist *sg;
> -	int count;
> -
> -	for_each_sg(sgl, sg, nents, count)
> -		__free_page(sg_page(sg));
> -
> -	kfree(sgl);
> +	sgl_free(sgl);
>  }
>  EXPORT_SYMBOL(target_free_sgl);
>  
> @@ -2423,42 +2417,10 @@ int
>  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
>  		 bool zero_page, bool chainable)
>  {
> -	struct scatterlist *sg;
> -	struct page *page;
> -	gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
> -	unsigned int nalloc, nent;
> -	int i = 0;
> -
> -	nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
> -	if (chainable)
> -		nalloc++;
> -	sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
> -	if (!sg)
> -		return -ENOMEM;
> +	gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
>  
> -	sg_init_table(sg, nalloc);
> -
> -	while (length) {
> -		u32 page_len = min_t(u32, length, PAGE_SIZE);
> -		page = alloc_page(GFP_KERNEL | zero_flag);
> -		if (!page)
> -			goto out;
> -
> -		sg_set_page(&sg[i], page, page_len, 0);
> -		length -= page_len;
> -		i++;
> -	}
> -	*sgl = sg;
> -	*nents = nent;
> -	return 0;
> -
> -out:
> -	while (i > 0) {
> -		i--;
> -		__free_page(sg_page(&sg[i]));
> -	}
> -	kfree(sg);
> -	return -ENOMEM;
> +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> +	return *sgl ? 0 : -ENOMEM;
>  }
>  EXPORT_SYMBOL(target_alloc_sgl);
>  
> The calling convention from target_alloc_sgl() is decidedly dodgy.
Can't we convert it into returning the sgl, and remove the first parameter?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free() functions instead of open
> coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Cc: Nicholas A. Bellinger <nab at linux-iscsi.org>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Hannes Reinecke <hare at suse.com>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/target/Kconfig                 |  1 +
>  drivers/target/target_core_transport.c | 46 +++-------------------------------
>  2 files changed, 5 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index e2bc99980f75..4c44d7bed01a 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -5,6 +5,7 @@ menuconfig TARGET_CORE
>  	select CONFIGFS_FS
>  	select CRC_T10DIF
>  	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
> +	select SGL_ALLOC
>  	default n
>  	help
>  	Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 836d552b0385..9bbd08be9d60 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
>  
>  void target_free_sgl(struct scatterlist *sgl, int nents)
>  {
> -	struct scatterlist *sg;
> -	int count;
> -
> -	for_each_sg(sgl, sg, nents, count)
> -		__free_page(sg_page(sg));
> -
> -	kfree(sgl);
> +	sgl_free(sgl);
>  }
>  EXPORT_SYMBOL(target_free_sgl);
>  
> @@ -2423,42 +2417,10 @@ int
>  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
>  		 bool zero_page, bool chainable)
>  {
> -	struct scatterlist *sg;
> -	struct page *page;
> -	gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
> -	unsigned int nalloc, nent;
> -	int i = 0;
> -
> -	nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
> -	if (chainable)
> -		nalloc++;
> -	sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
> -	if (!sg)
> -		return -ENOMEM;
> +	gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
>  
> -	sg_init_table(sg, nalloc);
> -
> -	while (length) {
> -		u32 page_len = min_t(u32, length, PAGE_SIZE);
> -		page = alloc_page(GFP_KERNEL | zero_flag);
> -		if (!page)
> -			goto out;
> -
> -		sg_set_page(&sg[i], page, page_len, 0);
> -		length -= page_len;
> -		i++;
> -	}
> -	*sgl = sg;
> -	*nents = nent;
> -	return 0;
> -
> -out:
> -	while (i > 0) {
> -		i--;
> -		__free_page(sg_page(&sg[i]));
> -	}
> -	kfree(sg);
> -	return -ENOMEM;
> +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> +	return *sgl ? 0 : -ENOMEM;
>  }
>  EXPORT_SYMBOL(target_alloc_sgl);
>  
> The calling convention from target_alloc_sgl() is decidedly dodgy.
Can't we convert it into returning the sgl, and remove the first parameter?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: linux-scsi@vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Brian King <brking@linux.vnet.ibm.com>
> ---
>  drivers/scsi/Kconfig |  1 +
>  drivers/scsi/ipr.c   | 49 ++++++++-----------------------------------------
>  drivers/scsi/ipr.h   |  2 +-
>  3 files changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 41366339b950..d11e75e76195 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1058,6 +1058,7 @@ config SCSI_IPR
>  	depends on PCI && SCSI && ATA
>  	select FW_LOADER
>  	select IRQ_POLL
> +	select SGL_ALLOC
>  	---help---
>  	  This driver supports the IBM Power Linux family RAID adapters.
>  	  This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index f838bd73befa..6fed5db6255e 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
>   **/
>  static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>  {
> -	int sg_size, order, bsize_elem, num_elem, i, j;
> +	int sg_size, order;
>  	struct ipr_sglist *sglist;
> -	struct scatterlist *scatterlist;
> -	struct page *page;
>  
>  	/* Get the minimum size per scatter/gather element */
>  	sg_size = buf_len / (IPR_MAX_SGLIST - 1);
> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>  	/* Get the actual size per element */
>  	order = get_order(sg_size);
>  
> -	/* Determine the actual number of bytes per element */
> -	bsize_elem = PAGE_SIZE * (1 << order);
> -
> -	/* Determine the actual number of sg entries needed */
> -	if (buf_len % bsize_elem)
> -		num_elem = (buf_len / bsize_elem) + 1;
> -	else
> -		num_elem = buf_len / bsize_elem;
> -
>  	/* Allocate a scatter/gather list for the DMA */
> -	sglist = kzalloc(sizeof(struct ipr_sglist) +
> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
> -			 GFP_KERNEL);
> -
> +	sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
>  	if (sglist == NULL) {
>  		ipr_trace;
>  		return NULL;
>  	}
> -
> -	scatterlist = sglist->scatterlist;
> -	sg_init_table(scatterlist, num_elem);
> -
>  	sglist->order = order;
> -	sglist->num_sg = num_elem;
> -
> -	/* Allocate a bunch of sg elements */
> -	for (i = 0; i < num_elem; i++) {
> -		page = alloc_pages(GFP_KERNEL, order);
> -		if (!page) {
> -			ipr_trace;
> -
> -			/* Free up what we already allocated */
> -			for (j = i - 1; j >= 0; j--)
> -				__free_pages(sg_page(&scatterlist[j]), order);
> -			kfree(sglist);
> -			return NULL;
> -		}
> -
> -		sg_set_page(&scatterlist[i], page, 0, 0);
> +	sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
> +					      &sglist->num_sg);
> +	if (!sglist->scatterlist) {
> +		kfree(sglist);
> +		return NULL;
>  	}
>  
>  	return sglist;
> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>   **/
>  static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
>  {
> -	int i;
> -
> -	for (i = 0; i < sglist->num_sg; i++)
> -		__free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
> -
> +	sgl_free_order(sglist->scatterlist, sglist->order);
>  	kfree(sglist);
>  }
>  
> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
> index c7f0e9e3cd7d..93570734cbfb 100644
> --- a/drivers/scsi/ipr.h
> +++ b/drivers/scsi/ipr.h
> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
>  	u32 num_sg;
>  	u32 num_dma_sg;
>  	u32 buffer_len;
> -	struct scatterlist scatterlist[1];
> +	struct scatterlist *scatterlist;
>  };
>  
>  enum ipr_sdt_state {
> 
Not sure if this is a valid conversion.
Originally the driver would allocate a single buffer; with this buffer
we have two distinct buffers.
Given that this is used to download the microcode I'm not sure if this
isn't a hardware-dependent structure which requires a single buffer
including the sglist.
Brian, can you shed some light here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Cc: linux-scsi at vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen at oracle.com>
> Cc: Brian King <brking at linux.vnet.ibm.com>
> ---
>  drivers/scsi/Kconfig |  1 +
>  drivers/scsi/ipr.c   | 49 ++++++++-----------------------------------------
>  drivers/scsi/ipr.h   |  2 +-
>  3 files changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 41366339b950..d11e75e76195 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1058,6 +1058,7 @@ config SCSI_IPR
>  	depends on PCI && SCSI && ATA
>  	select FW_LOADER
>  	select IRQ_POLL
> +	select SGL_ALLOC
>  	---help---
>  	  This driver supports the IBM Power Linux family RAID adapters.
>  	  This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index f838bd73befa..6fed5db6255e 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
>   **/
>  static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>  {
> -	int sg_size, order, bsize_elem, num_elem, i, j;
> +	int sg_size, order;
>  	struct ipr_sglist *sglist;
> -	struct scatterlist *scatterlist;
> -	struct page *page;
>  
>  	/* Get the minimum size per scatter/gather element */
>  	sg_size = buf_len / (IPR_MAX_SGLIST - 1);
> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>  	/* Get the actual size per element */
>  	order = get_order(sg_size);
>  
> -	/* Determine the actual number of bytes per element */
> -	bsize_elem = PAGE_SIZE * (1 << order);
> -
> -	/* Determine the actual number of sg entries needed */
> -	if (buf_len % bsize_elem)
> -		num_elem = (buf_len / bsize_elem) + 1;
> -	else
> -		num_elem = buf_len / bsize_elem;
> -
>  	/* Allocate a scatter/gather list for the DMA */
> -	sglist = kzalloc(sizeof(struct ipr_sglist) +
> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
> -			 GFP_KERNEL);
> -
> +	sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
>  	if (sglist == NULL) {
>  		ipr_trace;
>  		return NULL;
>  	}
> -
> -	scatterlist = sglist->scatterlist;
> -	sg_init_table(scatterlist, num_elem);
> -
>  	sglist->order = order;
> -	sglist->num_sg = num_elem;
> -
> -	/* Allocate a bunch of sg elements */
> -	for (i = 0; i < num_elem; i++) {
> -		page = alloc_pages(GFP_KERNEL, order);
> -		if (!page) {
> -			ipr_trace;
> -
> -			/* Free up what we already allocated */
> -			for (j = i - 1; j >= 0; j--)
> -				__free_pages(sg_page(&scatterlist[j]), order);
> -			kfree(sglist);
> -			return NULL;
> -		}
> -
> -		sg_set_page(&scatterlist[i], page, 0, 0);
> +	sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
> +					      &sglist->num_sg);
> +	if (!sglist->scatterlist) {
> +		kfree(sglist);
> +		return NULL;
>  	}
>  
>  	return sglist;
> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>   **/
>  static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
>  {
> -	int i;
> -
> -	for (i = 0; i < sglist->num_sg; i++)
> -		__free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
> -
> +	sgl_free_order(sglist->scatterlist, sglist->order);
>  	kfree(sglist);
>  }
>  
> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
> index c7f0e9e3cd7d..93570734cbfb 100644
> --- a/drivers/scsi/ipr.h
> +++ b/drivers/scsi/ipr.h
> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
>  	u32 num_sg;
>  	u32 num_dma_sg;
>  	u32 buffer_len;
> -	struct scatterlist scatterlist[1];
> +	struct scatterlist *scatterlist;
>  };
>  
>  enum ipr_sdt_state {
> 
Not sure if this is a valid conversion.
Originally the driver would allocate a single buffer; with this buffer
we have two distinct buffers.
Given that this is used to download the microcode I'm not sure if this
isn't a hardware-dependent structure which requires a single buffer
including the sglist.
Brian, can you shed some light here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: linux-scsi@vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Anil Ravindranath <anil_ravindranath@pmc-sierra.com>
> ---
>  drivers/scsi/pmcraid.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 8bfac72a242b..44da91712115 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,6 @@ struct pmcraid_sglist {
>  	u32 order;
>  	u32 num_sg;
>  	u32 num_dma_sg;
> -	u32 buffer_len;
>  	struct scatterlist scatterlist[1];
>  };
>  
> 
This actually is the same story that we've had with ipr (and, looking at
the code, those two drivers look awfully similar ...).
pmcraid_sglist looks as if it's a hardware-dependent structure, so just
removing one entry from the middle of a structure might not be a good idea.
But this is something for the pmcraid folks to clarify.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Cc: linux-scsi at vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen at oracle.com>
> Cc: Anil Ravindranath <anil_ravindranath at pmc-sierra.com>
> ---
>  drivers/scsi/pmcraid.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 8bfac72a242b..44da91712115 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,6 @@ struct pmcraid_sglist {
>  	u32 order;
>  	u32 num_sg;
>  	u32 num_dma_sg;
> -	u32 buffer_len;
>  	struct scatterlist scatterlist[1];
>  };
>  
> 
This actually is the same story that we've had with ipr (and, looking at
the code, those two drivers look awfully similar ...).
pmcraid_sglist looks as if it's a hardware-dependent structure, so just
removing one entry from the middle of a structure might not be a good idea.
But this is something for the pmcraid folks to clarify.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 8/8] scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: linux-scsi@vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Anil Ravindranath <anil_ravindranath@pmc-sierra.com>
> ---
>  drivers/scsi/Kconfig   |  1 +
>  drivers/scsi/pmcraid.c | 43 ++++---------------------------------------
>  drivers/scsi/pmcraid.h |  2 +-
>  3 files changed, 6 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index d11e75e76195..632200ec36a6 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1576,6 +1576,7 @@ config ZFCP
>  config SCSI_PMCRAID
>  	tristate "PMC SIERRA Linux MaxRAID adapter support"
>  	depends on PCI && SCSI && NET
> +	select SGL_ALLOC
>  	---help---
>  	  This driver supports the PMC SIERRA MaxRAID adapters.
>  
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index b4d6cd8cd1ad..95fc0352f9bb 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -3232,12 +3232,7 @@ static int pmcraid_build_ioadl(
>   */
>  static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
>  {
> -	int i;
> -
> -	for (i = 0; i < sglist->num_sg; i++)
> -		__free_pages(sg_page(&(sglist->scatterlist[i])),
> -			     sglist->order);
> -
> +	sgl_free_order(sglist->scatterlist, sglist->order);
>  	kfree(sglist);
>  }
>  
> @@ -3254,50 +3249,20 @@ static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
>  static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
>  {
>  	struct pmcraid_sglist *sglist;
> -	struct scatterlist *scatterlist;
> -	struct page *page;
> -	int num_elem, i, j;
>  	int sg_size;
>  	int order;
> -	int bsize_elem;
>  
>  	sg_size = buflen / (PMCRAID_MAX_IOADLS - 1);
>  	order = (sg_size > 0) ? get_order(sg_size) : 0;
> -	bsize_elem = PAGE_SIZE * (1 << order);
> -
> -	/* Determine the actual number of sg entries needed */
> -	if (buflen % bsize_elem)
> -		num_elem = (buflen / bsize_elem) + 1;
> -	else
> -		num_elem = buflen / bsize_elem;
>  
>  	/* Allocate a scatter/gather list for the DMA */
> -	sglist = kzalloc(sizeof(struct pmcraid_sglist) +
> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
> -			 GFP_KERNEL);
> -
> +	sglist = kzalloc(sizeof(struct pmcraid_sglist), GFP_KERNEL);
>  	if (sglist == NULL)
>  		return NULL;
>  
> -	scatterlist = sglist->scatterlist;
> -	sg_init_table(scatterlist, num_elem);
>  	sglist->order = order;
> -	sglist->num_sg = num_elem;
> -	sg_size = buflen;
> -
> -	for (i = 0; i < num_elem; i++) {
> -		page = alloc_pages(GFP_KERNEL|GFP_DMA|__GFP_ZERO, order);
> -		if (!page) {
> -			for (j = i - 1; j >= 0; j--)
> -				__free_pages(sg_page(&scatterlist[j]), order);
> -			kfree(sglist);
> -			return NULL;
> -		}
> -
> -		sg_set_page(&scatterlist[i], page,
> -			sg_size < bsize_elem ? sg_size : bsize_elem, 0);
> -		sg_size -= bsize_elem;
> -	}
> +	sgl_alloc_order(buflen, order, false,
> +			GFP_KERNEL | GFP_DMA | __GFP_ZERO, &sglist->num_sg);
>  
>  	return sglist;
>  }
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 44da91712115..754ef30927e2 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,7 @@ struct pmcraid_sglist {
>  	u32 order;
>  	u32 num_sg;
>  	u32 num_dma_sg;
> -	struct scatterlist scatterlist[1];
> +	struct scatterlist *scatterlist;
>  };
>  
>  /* page D0 inquiry data of focal point resource */
> 
The comment to ipr applied here, too.
We need input from the pmcraid folks if this is a valid conversion.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 8/8] scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Cc: linux-scsi at vger.kernel.org
> Cc: Martin K. Petersen <martin.petersen at oracle.com>
> Cc: Anil Ravindranath <anil_ravindranath at pmc-sierra.com>
> ---
>  drivers/scsi/Kconfig   |  1 +
>  drivers/scsi/pmcraid.c | 43 ++++---------------------------------------
>  drivers/scsi/pmcraid.h |  2 +-
>  3 files changed, 6 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index d11e75e76195..632200ec36a6 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1576,6 +1576,7 @@ config ZFCP
>  config SCSI_PMCRAID
>  	tristate "PMC SIERRA Linux MaxRAID adapter support"
>  	depends on PCI && SCSI && NET
> +	select SGL_ALLOC
>  	---help---
>  	  This driver supports the PMC SIERRA MaxRAID adapters.
>  
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index b4d6cd8cd1ad..95fc0352f9bb 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -3232,12 +3232,7 @@ static int pmcraid_build_ioadl(
>   */
>  static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
>  {
> -	int i;
> -
> -	for (i = 0; i < sglist->num_sg; i++)
> -		__free_pages(sg_page(&(sglist->scatterlist[i])),
> -			     sglist->order);
> -
> +	sgl_free_order(sglist->scatterlist, sglist->order);
>  	kfree(sglist);
>  }
>  
> @@ -3254,50 +3249,20 @@ static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
>  static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
>  {
>  	struct pmcraid_sglist *sglist;
> -	struct scatterlist *scatterlist;
> -	struct page *page;
> -	int num_elem, i, j;
>  	int sg_size;
>  	int order;
> -	int bsize_elem;
>  
>  	sg_size = buflen / (PMCRAID_MAX_IOADLS - 1);
>  	order = (sg_size > 0) ? get_order(sg_size) : 0;
> -	bsize_elem = PAGE_SIZE * (1 << order);
> -
> -	/* Determine the actual number of sg entries needed */
> -	if (buflen % bsize_elem)
> -		num_elem = (buflen / bsize_elem) + 1;
> -	else
> -		num_elem = buflen / bsize_elem;
>  
>  	/* Allocate a scatter/gather list for the DMA */
> -	sglist = kzalloc(sizeof(struct pmcraid_sglist) +
> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
> -			 GFP_KERNEL);
> -
> +	sglist = kzalloc(sizeof(struct pmcraid_sglist), GFP_KERNEL);
>  	if (sglist == NULL)
>  		return NULL;
>  
> -	scatterlist = sglist->scatterlist;
> -	sg_init_table(scatterlist, num_elem);
>  	sglist->order = order;
> -	sglist->num_sg = num_elem;
> -	sg_size = buflen;
> -
> -	for (i = 0; i < num_elem; i++) {
> -		page = alloc_pages(GFP_KERNEL|GFP_DMA|__GFP_ZERO, order);
> -		if (!page) {
> -			for (j = i - 1; j >= 0; j--)
> -				__free_pages(sg_page(&scatterlist[j]), order);
> -			kfree(sglist);
> -			return NULL;
> -		}
> -
> -		sg_set_page(&scatterlist[i], page,
> -			sg_size < bsize_elem ? sg_size : bsize_elem, 0);
> -		sg_size -= bsize_elem;
> -	}
> +	sgl_alloc_order(buflen, order, false,
> +			GFP_KERNEL | GFP_DMA | __GFP_ZERO, &sglist->num_sg);
>  
>  	return sglist;
>  }
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 44da91712115..754ef30927e2 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,7 @@ struct pmcraid_sglist {
>  	u32 order;
>  	u32 num_sg;
>  	u32 num_dma_sg;
> -	struct scatterlist scatterlist[1];
> +	struct scatterlist *scatterlist;
>  };
>  
>  /* page D0 inquiry data of focal point resource */
> 
The comment to ipr applied here, too.
We need input from the pmcraid folks if this is a valid conversion.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Johannes Thumshirn]

Thanks Bart,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Johannes Thumshirn]

Thanks Bart,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v2 1/8] lib/scatterlist: Introduce sgl_alloc() and sgl_free()

[Johannes Thumshirn]

Thanks Bart,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Bart Van Assche]

On Tue, 2017-10-17 at 08:21 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Cc: linux-scsi@vger.kernel.org
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Anil Ravindranath <anil_ravindranath@pmc-sierra.com>
> > ---
> >  drivers/scsi/pmcraid.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> > index 8bfac72a242b..44da91712115 100644
> > --- a/drivers/scsi/pmcraid.h
> > +++ b/drivers/scsi/pmcraid.h
> > @@ -542,7 +542,6 @@ struct pmcraid_sglist {
> >  	u32 order;
> >  	u32 num_sg;
> >  	u32 num_dma_sg;
> > -	u32 buffer_len;
> >  	struct scatterlist scatterlist[1];
> >  };
> >  
> > 
> 
> This actually is the same story that we've had with ipr (and, looking at
> the code, those two drivers look awfully similar ...).
> pmcraid_sglist looks as if it's a hardware-dependent structure, so just
> removing one entry from the middle of a structure might not be a good idea.
> But this is something for the pmcraid folks to clarify.

Hello Hannes,

Sorry but I don't see how a structure that contains a struct scatterlist
could be hardware-dependent?

Thanks,

Bart.

Re: [PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Bart Van Assche]

T24gVHVlLCAyMDE3LTEwLTE3IGF0IDA4OjIxICswMjAwLCBIYW5uZXMgUmVpbmVja2Ugd3JvdGU6
DQo+IE9uIDEwLzE3LzIwMTcgMTI6NDkgQU0sIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiBT
aWduZWQtb2ZmLWJ5OiBCYXJ0IFZhbiBBc3NjaGUgPGJhcnQudmFuYXNzY2hlQHdkYy5jb20+DQo+
ID4gUmV2aWV3ZWQtYnk6IEpvaGFubmVzIFRodW1zaGlybiA8anRodW1zaGlybkBzdXNlLmRlPg0K
PiA+IENjOiBsaW51eC1zY3NpQHZnZXIua2VybmVsLm9yZw0KPiA+IENjOiBNYXJ0aW4gSy4gUGV0
ZXJzZW4gPG1hcnRpbi5wZXRlcnNlbkBvcmFjbGUuY29tPg0KPiA+IENjOiBBbmlsIFJhdmluZHJh
bmF0aCA8YW5pbF9yYXZpbmRyYW5hdGhAcG1jLXNpZXJyYS5jb20+DQo+ID4gLS0tDQo+ID4gIGRy
aXZlcnMvc2NzaS9wbWNyYWlkLmggfCAxIC0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDEgZGVsZXRp
b24oLSkNCj4gPiANCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zY3NpL3BtY3JhaWQuaCBiL2Ry
aXZlcnMvc2NzaS9wbWNyYWlkLmgNCj4gPiBpbmRleCA4YmZhYzcyYTI0MmIuLjQ0ZGE5MTcxMjEx
NSAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3Njc2kvcG1jcmFpZC5oDQo+ID4gKysrIGIvZHJp
dmVycy9zY3NpL3BtY3JhaWQuaA0KPiA+IEBAIC01NDIsNyArNTQyLDYgQEAgc3RydWN0IHBtY3Jh
aWRfc2dsaXN0IHsNCj4gPiAgCXUzMiBvcmRlcjsNCj4gPiAgCXUzMiBudW1fc2c7DQo+ID4gIAl1
MzIgbnVtX2RtYV9zZzsNCj4gPiAtCXUzMiBidWZmZXJfbGVuOw0KPiA+ICAJc3RydWN0IHNjYXR0
ZXJsaXN0IHNjYXR0ZXJsaXN0WzFdOw0KPiA+ICB9Ow0KPiA+ICANCj4gPiANCj4gDQo+IFRoaXMg
YWN0dWFsbHkgaXMgdGhlIHNhbWUgc3RvcnkgdGhhdCB3ZSd2ZSBoYWQgd2l0aCBpcHIgKGFuZCwg
bG9va2luZyBhdA0KPiB0aGUgY29kZSwgdGhvc2UgdHdvIGRyaXZlcnMgbG9vayBhd2Z1bGx5IHNp
bWlsYXIgLi4uKS4NCj4gcG1jcmFpZF9zZ2xpc3QgbG9va3MgYXMgaWYgaXQncyBhIGhhcmR3YXJl
LWRlcGVuZGVudCBzdHJ1Y3R1cmUsIHNvIGp1c3QNCj4gcmVtb3Zpbmcgb25lIGVudHJ5IGZyb20g
dGhlIG1pZGRsZSBvZiBhIHN0cnVjdHVyZSBtaWdodCBub3QgYmUgYSBnb29kIGlkZWEuDQo+IEJ1
dCB0aGlzIGlzIHNvbWV0aGluZyBmb3IgdGhlIHBtY3JhaWQgZm9sa3MgdG8gY2xhcmlmeS4NCg0K
SGVsbG8gSGFubmVzLA0KDQpTb3JyeSBidXQgSSBkb24ndCBzZWUgaG93IGEgc3RydWN0dXJlIHRo
YXQgY29udGFpbnMgYSBzdHJ1Y3Qgc2NhdHRlcmxpc3QNCmNvdWxkIGJlIGhhcmR3YXJlLWRlcGVu
ZGVudD8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

[PATCH v2 7/8] scsi/pmcraid: Remove an unused structure member

[Bart Van Assche]

On Tue, 2017-10-17@08:21 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> > Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> > Cc: linux-scsi at vger.kernel.org
> > Cc: Martin K. Petersen <martin.petersen at oracle.com>
> > Cc: Anil Ravindranath <anil_ravindranath at pmc-sierra.com>
> > ---
> >  drivers/scsi/pmcraid.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> > index 8bfac72a242b..44da91712115 100644
> > --- a/drivers/scsi/pmcraid.h
> > +++ b/drivers/scsi/pmcraid.h
> > @@ -542,7 +542,6 @@ struct pmcraid_sglist {
> >  	u32 order;
> >  	u32 num_sg;
> >  	u32 num_dma_sg;
> > -	u32 buffer_len;
> >  	struct scatterlist scatterlist[1];
> >  };
> >  
> > 
> 
> This actually is the same story that we've had with ipr (and, looking at
> the code, those two drivers look awfully similar ...).
> pmcraid_sglist looks as if it's a hardware-dependent structure, so just
> removing one entry from the middle of a structure might not be a good idea.
> But this is something for the pmcraid folks to clarify.

Hello Hannes,

Sorry but I don't see how a structure that contains a struct scatterlist
could be hardware-dependent?

Thanks,

Bart.

Re: [PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Bart Van Assche]

On Tue, 2017-10-17 at 08:14 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > [ ... ]
> >  void target_free_sgl(struct scatterlist *sgl, int nents)
> >  {
> > -	struct scatterlist *sg;
> > -	int count;
> > -
> > -	for_each_sg(sgl, sg, nents, count)
> > -		__free_page(sg_page(sg));
> > -
> > -	kfree(sgl);
> > +	sgl_free(sgl);
> >  }
> >  EXPORT_SYMBOL(target_free_sgl);
> >  
> > @@ -2423,42 +2417,10 @@ int
> >  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
> >  		 bool zero_page, bool chainable)
> >  {
> > -	[ ... ]
> > +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> > +	return *sgl ? 0 : -ENOMEM;
> >  }
> >  EXPORT_SYMBOL(target_alloc_sgl);
> >  
> > The calling convention from target_alloc_sgl() is decidedly dodgy.
> 
> Can't we convert it into returning the sgl, and remove the first parameter?

Hello Hannes,

Another option is to remove the target_alloc_sgl() and target_free_sgl() functions
and to make LIO target drivers call sgl_alloc_order() and sgl_free_order() directly.
Do you perhaps have a preference for one of these two approaches?

Thanks,

Bart.

Re: [PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Bart Van Assche]

T24gVHVlLCAyMDE3LTEwLTE3IGF0IDA4OjE0ICswMjAwLCBIYW5uZXMgUmVpbmVja2Ugd3JvdGU6
DQo+IE9uIDEwLzE3LzIwMTcgMTI6NDkgQU0sIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiBb
IC4uLiBdDQo+ID4gIHZvaWQgdGFyZ2V0X2ZyZWVfc2dsKHN0cnVjdCBzY2F0dGVybGlzdCAqc2ds
LCBpbnQgbmVudHMpDQo+ID4gIHsNCj4gPiAtCXN0cnVjdCBzY2F0dGVybGlzdCAqc2c7DQo+ID4g
LQlpbnQgY291bnQ7DQo+ID4gLQ0KPiA+IC0JZm9yX2VhY2hfc2coc2dsLCBzZywgbmVudHMsIGNv
dW50KQ0KPiA+IC0JCV9fZnJlZV9wYWdlKHNnX3BhZ2Uoc2cpKTsNCj4gPiAtDQo+ID4gLQlrZnJl
ZShzZ2wpOw0KPiA+ICsJc2dsX2ZyZWUoc2dsKTsNCj4gPiAgfQ0KPiA+ICBFWFBPUlRfU1lNQk9M
KHRhcmdldF9mcmVlX3NnbCk7DQo+ID4gIA0KPiA+IEBAIC0yNDIzLDQyICsyNDE3LDEwIEBAIGlu
dA0KPiA+ICB0YXJnZXRfYWxsb2Nfc2dsKHN0cnVjdCBzY2F0dGVybGlzdCAqKnNnbCwgdW5zaWdu
ZWQgaW50ICpuZW50cywgdTMyIGxlbmd0aCwNCj4gPiAgCQkgYm9vbCB6ZXJvX3BhZ2UsIGJvb2wg
Y2hhaW5hYmxlKQ0KPiA+ICB7DQo+ID4gLQlbIC4uLiBdDQo+ID4gKwkqc2dsID0gc2dsX2FsbG9j
X29yZGVyKGxlbmd0aCwgMCwgY2hhaW5hYmxlLCBnZnAsIG5lbnRzKTsNCj4gPiArCXJldHVybiAq
c2dsID8gMCA6IC1FTk9NRU07DQo+ID4gIH0NCj4gPiAgRVhQT1JUX1NZTUJPTCh0YXJnZXRfYWxs
b2Nfc2dsKTsNCj4gPiAgDQo+ID4gVGhlIGNhbGxpbmcgY29udmVudGlvbiBmcm9tIHRhcmdldF9h
bGxvY19zZ2woKSBpcyBkZWNpZGVkbHkgZG9kZ3kuDQo+IA0KPiBDYW4ndCB3ZSBjb252ZXJ0IGl0
IGludG8gcmV0dXJuaW5nIHRoZSBzZ2wsIGFuZCByZW1vdmUgdGhlIGZpcnN0IHBhcmFtZXRlcj8N
Cg0KSGVsbG8gSGFubmVzLA0KDQpBbm90aGVyIG9wdGlvbiBpcyB0byByZW1vdmUgdGhlIHRhcmdl
dF9hbGxvY19zZ2woKSBhbmQgdGFyZ2V0X2ZyZWVfc2dsKCkgZnVuY3Rpb25zDQphbmQgdG8gbWFr
ZSBMSU8gdGFyZ2V0IGRyaXZlcnMgY2FsbCBzZ2xfYWxsb2Nfb3JkZXIoKSBhbmQgc2dsX2ZyZWVf
b3JkZXIoKSBkaXJlY3RseS4NCkRvIHlvdSBwZXJoYXBzIGhhdmUgYSBwcmVmZXJlbmNlIGZvciBv
bmUgb2YgdGhlc2UgdHdvIGFwcHJvYWNoZXM/DQoNClRoYW5rcywNCg0KQmFydC4=

[PATCH v2 5/8] target: Use sgl_alloc_order() and sgl_free()

[Bart Van Assche]

On Tue, 2017-10-17@08:14 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > [ ... ]
> >  void target_free_sgl(struct scatterlist *sgl, int nents)
> >  {
> > -	struct scatterlist *sg;
> > -	int count;
> > -
> > -	for_each_sg(sgl, sg, nents, count)
> > -		__free_page(sg_page(sg));
> > -
> > -	kfree(sgl);
> > +	sgl_free(sgl);
> >  }
> >  EXPORT_SYMBOL(target_free_sgl);
> >  
> > @@ -2423,42 +2417,10 @@ int
> >  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
> >  		 bool zero_page, bool chainable)
> >  {
> > -	[ ... ]
> > +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> > +	return *sgl ? 0 : -ENOMEM;
> >  }
> >  EXPORT_SYMBOL(target_alloc_sgl);
> >  
> > The calling convention from target_alloc_sgl() is decidedly dodgy.
> 
> Can't we convert it into returning the sgl, and remove the first parameter?

Hello Hannes,

Another option is to remove the target_alloc_sgl() and target_free_sgl() functions
and to make LIO target drivers call sgl_alloc_order() and sgl_free_order() directly.
Do you perhaps have a preference for one of these two approaches?

Thanks,

Bart.

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Brian King]

On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>> Use the sgl_alloc_order() and sgl_free_order() functions instead
>> of open coding these functions.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>> Cc: linux-scsi@vger.kernel.org
>> Cc: Martin K. Petersen <martin.petersen@oracle.com>
>> Cc: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>  drivers/scsi/Kconfig |  1 +
>>  drivers/scsi/ipr.c   | 49 ++++++++-----------------------------------------
>>  drivers/scsi/ipr.h   |  2 +-
>>  3 files changed, 10 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 41366339b950..d11e75e76195 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -1058,6 +1058,7 @@ config SCSI_IPR
>>  	depends on PCI && SCSI && ATA
>>  	select FW_LOADER
>>  	select IRQ_POLL
>> +	select SGL_ALLOC
>>  	---help---
>>  	  This driver supports the IBM Power Linux family RAID adapters.
>>  	  This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
>> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
>> index f838bd73befa..6fed5db6255e 100644
>> --- a/drivers/scsi/ipr.c
>> +++ b/drivers/scsi/ipr.c
>> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
>>   **/
>>  static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>  {
>> -	int sg_size, order, bsize_elem, num_elem, i, j;
>> +	int sg_size, order;
>>  	struct ipr_sglist *sglist;
>> -	struct scatterlist *scatterlist;
>> -	struct page *page;
>>  
>>  	/* Get the minimum size per scatter/gather element */
>>  	sg_size = buf_len / (IPR_MAX_SGLIST - 1);
>> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>  	/* Get the actual size per element */
>>  	order = get_order(sg_size);
>>  
>> -	/* Determine the actual number of bytes per element */
>> -	bsize_elem = PAGE_SIZE * (1 << order);
>> -
>> -	/* Determine the actual number of sg entries needed */
>> -	if (buf_len % bsize_elem)
>> -		num_elem = (buf_len / bsize_elem) + 1;
>> -	else
>> -		num_elem = buf_len / bsize_elem;
>> -
>>  	/* Allocate a scatter/gather list for the DMA */
>> -	sglist = kzalloc(sizeof(struct ipr_sglist) +
>> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
>> -			 GFP_KERNEL);
>> -
>> +	sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
>>  	if (sglist == NULL) {
>>  		ipr_trace;
>>  		return NULL;
>>  	}
>> -
>> -	scatterlist = sglist->scatterlist;
>> -	sg_init_table(scatterlist, num_elem);
>> -
>>  	sglist->order = order;
>> -	sglist->num_sg = num_elem;
>> -
>> -	/* Allocate a bunch of sg elements */
>> -	for (i = 0; i < num_elem; i++) {
>> -		page = alloc_pages(GFP_KERNEL, order);
>> -		if (!page) {
>> -			ipr_trace;
>> -
>> -			/* Free up what we already allocated */
>> -			for (j = i - 1; j >= 0; j--)
>> -				__free_pages(sg_page(&scatterlist[j]), order);
>> -			kfree(sglist);
>> -			return NULL;
>> -		}
>> -
>> -		sg_set_page(&scatterlist[i], page, 0, 0);
>> +	sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
>> +					      &sglist->num_sg);
>> +	if (!sglist->scatterlist) {
>> +		kfree(sglist);
>> +		return NULL;
>>  	}
>>  
>>  	return sglist;
>> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>   **/
>>  static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
>>  {
>> -	int i;
>> -
>> -	for (i = 0; i < sglist->num_sg; i++)
>> -		__free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
>> -
>> +	sgl_free_order(sglist->scatterlist, sglist->order);
>>  	kfree(sglist);
>>  }
>>  
>> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
>> index c7f0e9e3cd7d..93570734cbfb 100644
>> --- a/drivers/scsi/ipr.h
>> +++ b/drivers/scsi/ipr.h
>> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
>>  	u32 num_sg;
>>  	u32 num_dma_sg;
>>  	u32 buffer_len;
>> -	struct scatterlist scatterlist[1];
>> +	struct scatterlist *scatterlist;
>>  };
>>  
>>  enum ipr_sdt_state {
>>
> Not sure if this is a valid conversion.
> Originally the driver would allocate a single buffer; with this buffer
> we have two distinct buffers.
> Given that this is used to download the microcode I'm not sure if this
> isn't a hardware-dependent structure which requires a single buffer
> including the sglist.
> Brian, can you shed some light here?

The struct ipr_sglist is not a hardware defined data structure, so on initial
glance, this should be OK. I'll load it up and give it a try to make sure
it doesn't break code download.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Brian King]

On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>> Use the sgl_alloc_order() and sgl_free_order() functions instead
>> of open coding these functions.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
>> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
>> Cc: linux-scsi at vger.kernel.org
>> Cc: Martin K. Petersen <martin.petersen at oracle.com>
>> Cc: Brian King <brking at linux.vnet.ibm.com>
>> ---
>>  drivers/scsi/Kconfig |  1 +
>>  drivers/scsi/ipr.c   | 49 ++++++++-----------------------------------------
>>  drivers/scsi/ipr.h   |  2 +-
>>  3 files changed, 10 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 41366339b950..d11e75e76195 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -1058,6 +1058,7 @@ config SCSI_IPR
>>  	depends on PCI && SCSI && ATA
>>  	select FW_LOADER
>>  	select IRQ_POLL
>> +	select SGL_ALLOC
>>  	---help---
>>  	  This driver supports the IBM Power Linux family RAID adapters.
>>  	  This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
>> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
>> index f838bd73befa..6fed5db6255e 100644
>> --- a/drivers/scsi/ipr.c
>> +++ b/drivers/scsi/ipr.c
>> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
>>   **/
>>  static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>  {
>> -	int sg_size, order, bsize_elem, num_elem, i, j;
>> +	int sg_size, order;
>>  	struct ipr_sglist *sglist;
>> -	struct scatterlist *scatterlist;
>> -	struct page *page;
>>  
>>  	/* Get the minimum size per scatter/gather element */
>>  	sg_size = buf_len / (IPR_MAX_SGLIST - 1);
>> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>  	/* Get the actual size per element */
>>  	order = get_order(sg_size);
>>  
>> -	/* Determine the actual number of bytes per element */
>> -	bsize_elem = PAGE_SIZE * (1 << order);
>> -
>> -	/* Determine the actual number of sg entries needed */
>> -	if (buf_len % bsize_elem)
>> -		num_elem = (buf_len / bsize_elem) + 1;
>> -	else
>> -		num_elem = buf_len / bsize_elem;
>> -
>>  	/* Allocate a scatter/gather list for the DMA */
>> -	sglist = kzalloc(sizeof(struct ipr_sglist) +
>> -			 (sizeof(struct scatterlist) * (num_elem - 1)),
>> -			 GFP_KERNEL);
>> -
>> +	sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
>>  	if (sglist == NULL) {
>>  		ipr_trace;
>>  		return NULL;
>>  	}
>> -
>> -	scatterlist = sglist->scatterlist;
>> -	sg_init_table(scatterlist, num_elem);
>> -
>>  	sglist->order = order;
>> -	sglist->num_sg = num_elem;
>> -
>> -	/* Allocate a bunch of sg elements */
>> -	for (i = 0; i < num_elem; i++) {
>> -		page = alloc_pages(GFP_KERNEL, order);
>> -		if (!page) {
>> -			ipr_trace;
>> -
>> -			/* Free up what we already allocated */
>> -			for (j = i - 1; j >= 0; j--)
>> -				__free_pages(sg_page(&scatterlist[j]), order);
>> -			kfree(sglist);
>> -			return NULL;
>> -		}
>> -
>> -		sg_set_page(&scatterlist[i], page, 0, 0);
>> +	sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
>> +					      &sglist->num_sg);
>> +	if (!sglist->scatterlist) {
>> +		kfree(sglist);
>> +		return NULL;
>>  	}
>>  
>>  	return sglist;
>> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>>   **/
>>  static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
>>  {
>> -	int i;
>> -
>> -	for (i = 0; i < sglist->num_sg; i++)
>> -		__free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
>> -
>> +	sgl_free_order(sglist->scatterlist, sglist->order);
>>  	kfree(sglist);
>>  }
>>  
>> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
>> index c7f0e9e3cd7d..93570734cbfb 100644
>> --- a/drivers/scsi/ipr.h
>> +++ b/drivers/scsi/ipr.h
>> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
>>  	u32 num_sg;
>>  	u32 num_dma_sg;
>>  	u32 buffer_len;
>> -	struct scatterlist scatterlist[1];
>> +	struct scatterlist *scatterlist;
>>  };
>>  
>>  enum ipr_sdt_state {
>>
> Not sure if this is a valid conversion.
> Originally the driver would allocate a single buffer; with this buffer
> we have two distinct buffers.
> Given that this is used to download the microcode I'm not sure if this
> isn't a hardware-dependent structure which requires a single buffer
> including the sglist.
> Brian, can you shed some light here?

The struct ipr_sglist is not a hardware defined data structure, so on initial
glance, this should be OK. I'll load it up and give it a try to make sure
it doesn't break code download.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Bart Van Assche]

On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> > On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > > [ ... ]
> > 
> > Not sure if this is a valid conversion.
> > Originally the driver would allocate a single buffer; with this buffer
> > we have two distinct buffers.
> > Given that this is used to download the microcode I'm not sure if this
> > isn't a hardware-dependent structure which requires a single buffer
> > including the sglist.
> > Brian, can you shed some light here?
> 
> The struct ipr_sglist is not a hardware defined data structure, so on initial
> glance, this should be OK. I'll load it up and give it a try to make sure
> it doesn't break code download.

Hello Brian,

Have you already obtained any test results?

Thanks,

Bart.

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Bart Van Assche]

T24gV2VkLCAyMDE3LTEwLTE4IGF0IDE1OjU3IC0wNTAwLCBCcmlhbiBLaW5nIHdyb3RlOg0KPiBP
biAxMC8xNy8yMDE3IDAxOjE5IEFNLCBIYW5uZXMgUmVpbmVja2Ugd3JvdGU6DQo+ID4gT24gMTAv
MTcvMjAxNyAxMjo0OSBBTSwgQmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiA+ID4gWyAuLi4gXQ0K
PiA+IA0KPiA+IE5vdCBzdXJlIGlmIHRoaXMgaXMgYSB2YWxpZCBjb252ZXJzaW9uLg0KPiA+IE9y
aWdpbmFsbHkgdGhlIGRyaXZlciB3b3VsZCBhbGxvY2F0ZSBhIHNpbmdsZSBidWZmZXI7IHdpdGgg
dGhpcyBidWZmZXINCj4gPiB3ZSBoYXZlIHR3byBkaXN0aW5jdCBidWZmZXJzLg0KPiA+IEdpdmVu
IHRoYXQgdGhpcyBpcyB1c2VkIHRvIGRvd25sb2FkIHRoZSBtaWNyb2NvZGUgSSdtIG5vdCBzdXJl
IGlmIHRoaXMNCj4gPiBpc24ndCBhIGhhcmR3YXJlLWRlcGVuZGVudCBzdHJ1Y3R1cmUgd2hpY2gg
cmVxdWlyZXMgYSBzaW5nbGUgYnVmZmVyDQo+ID4gaW5jbHVkaW5nIHRoZSBzZ2xpc3QuDQo+ID4g
QnJpYW4sIGNhbiB5b3Ugc2hlZCBzb21lIGxpZ2h0IGhlcmU/DQo+IA0KPiBUaGUgc3RydWN0IGlw
cl9zZ2xpc3QgaXMgbm90IGEgaGFyZHdhcmUgZGVmaW5lZCBkYXRhIHN0cnVjdHVyZSwgc28gb24g
aW5pdGlhbA0KPiBnbGFuY2UsIHRoaXMgc2hvdWxkIGJlIE9LLiBJJ2xsIGxvYWQgaXQgdXAgYW5k
IGdpdmUgaXQgYSB0cnkgdG8gbWFrZSBzdXJlDQo+IGl0IGRvZXNuJ3QgYnJlYWsgY29kZSBkb3du
bG9hZC4NCg0KSGVsbG8gQnJpYW4sDQoNCkhhdmUgeW91IGFscmVhZHkgb2J0YWluZWQgYW55IHRl
c3QgcmVzdWx0cz8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Bart Van Assche]

On Wed, 2017-10-18@15:57 -0500, Brian King wrote:
> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> > On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > > [ ... ]
> > 
> > Not sure if this is a valid conversion.
> > Originally the driver would allocate a single buffer; with this buffer
> > we have two distinct buffers.
> > Given that this is used to download the microcode I'm not sure if this
> > isn't a hardware-dependent structure which requires a single buffer
> > including the sglist.
> > Brian, can you shed some light here?
> 
> The struct ipr_sglist is not a hardware defined data structure, so on initial
> glance, this should be OK. I'll load it up and give it a try to make sure
> it doesn't break code download.

Hello Brian,

Have you already obtained any test results?

Thanks,

Bart.

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Brian King]

On 10/30/2017 03:37 PM, Bart Van Assche wrote:
> On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>> [ ... ]
>>>
>>> Not sure if this is a valid conversion.
>>> Originally the driver would allocate a single buffer; with this buffer
>>> we have two distinct buffers.
>>> Given that this is used to download the microcode I'm not sure if this
>>> isn't a hardware-dependent structure which requires a single buffer
>>> including the sglist.
>>> Brian, can you shed some light here?
>>
>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>> glance, this should be OK. I'll load it up and give it a try to make sure
>> it doesn't break code download.
> 
> Hello Brian,
> 
> Have you already obtained any test results?

Bart,

Yes. I tried this out on an ipr adapter and it looks fine.

Acked-by: Brian King <brking@linux.vnet.ibm.com>

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Brian King]

On 10/30/2017 03:37 PM, Bart Van Assche wrote:
> On Wed, 2017-10-18@15:57 -0500, Brian King wrote:
>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>> [ ... ]
>>>
>>> Not sure if this is a valid conversion.
>>> Originally the driver would allocate a single buffer; with this buffer
>>> we have two distinct buffers.
>>> Given that this is used to download the microcode I'm not sure if this
>>> isn't a hardware-dependent structure which requires a single buffer
>>> including the sglist.
>>> Brian, can you shed some light here?
>>
>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>> glance, this should be OK. I'll load it up and give it a try to make sure
>> it doesn't break code download.
> 
> Hello Brian,
> 
> Have you already obtained any test results?

Bart,

Yes. I tried this out on an ipr adapter and it looks fine.

Acked-by: Brian King <brking at linux.vnet.ibm.com>

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/30/2017 10:01 PM, Brian King wrote:
> On 10/30/2017 03:37 PM, Bart Van Assche wrote:
>> On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
>>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>>> [ ... ]
>>>>
>>>> Not sure if this is a valid conversion.
>>>> Originally the driver would allocate a single buffer; with this buffer
>>>> we have two distinct buffers.
>>>> Given that this is used to download the microcode I'm not sure if this
>>>> isn't a hardware-dependent structure which requires a single buffer
>>>> including the sglist.
>>>> Brian, can you shed some light here?
>>>
>>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>>> glance, this should be OK. I'll load it up and give it a try to make sure
>>> it doesn't break code download.
>>
>> Hello Brian,
>>
>> Have you already obtained any test results?
> 
> Bart,
> 
> Yes. I tried this out on an ipr adapter and it looks fine.
> 
> Acked-by: Brian King <brking@linux.vnet.ibm.com>
> 
Thanks for the confirmation.

Bart, you can add my

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Hannes Reinecke]

On 10/30/2017 10:01 PM, Brian King wrote:
> On 10/30/2017 03:37 PM, Bart Van Assche wrote:
>> On Wed, 2017-10-18@15:57 -0500, Brian King wrote:
>>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>>> [ ... ]
>>>>
>>>> Not sure if this is a valid conversion.
>>>> Originally the driver would allocate a single buffer; with this buffer
>>>> we have two distinct buffers.
>>>> Given that this is used to download the microcode I'm not sure if this
>>>> isn't a hardware-dependent structure which requires a single buffer
>>>> including the sglist.
>>>> Brian, can you shed some light here?
>>>
>>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>>> glance, this should be OK. I'll load it up and give it a try to make sure
>>> it doesn't break code download.
>>
>> Hello Brian,
>>
>> Have you already obtained any test results?
> 
> Bart,
> 
> Yes. I tried this out on an ipr adapter and it looks fine.
> 
> Acked-by: Brian King <brking at linux.vnet.ibm.com>
> 
Thanks for the confirmation.

Bart, you can add my

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>

Ard and/or Herbert, can you please have a look at this patch and let us know
whether or not it looks fine to you?

Thanks,

Bart.

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

T24gTW9uLCAyMDE3LTEwLTE2IGF0IDE1OjQ5IC0wNzAwLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6
DQo+IFVzZSB0aGUgc2dsX2FsbG9jKCkgYW5kIHNnbF9mcmVlKCkgZnVuY3Rpb25zIGluc3RlYWQg
b2Ygb3BlbiBjb2RpbmcNCj4gdGhlc2UgZnVuY3Rpb25zLg0KPiANCj4gU2lnbmVkLW9mZi1ieTog
QmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KPiBDYzogQXJkIEJpZXNo
ZXV2ZWwgPGFyZC5iaWVzaGV1dmVsQGxpbmFyby5vcmc+DQo+IENjOiBIZXJiZXJ0IFh1IDxoZXJi
ZXJ0QGdvbmRvci5hcGFuYS5vcmcuYXU+DQoNCkFyZCBhbmQvb3IgSGVyYmVydCwgY2FuIHlvdSBw
bGVhc2UgaGF2ZSBhIGxvb2sgYXQgdGhpcyBwYXRjaCBhbmQgbGV0IHVzIGtub3cNCndoZXRoZXIg
b3Igbm90IGl0IGxvb2tzIGZpbmUgdG8geW91Pw0KDQpUaGFua3MsDQoNCkJhcnQu

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

On Mon, 2017-10-16@15:49 -0700, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Herbert Xu <herbert at gondor.apana.org.au>

Ard and/or Herbert, can you please have a look at this patch and let us know
whether or not it looks fine to you?

Thanks,

Bart.

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Ard Biesheuvel]

On 1 November 2017 at 14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
>> Use the sgl_alloc() and sgl_free() functions instead of open coding
>> these functions.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>
> Ard and/or Herbert, can you please have a look at this patch and let us know
> whether or not it looks fine to you?
>

The patch itself does not look unreasonable, but I can't find
sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
on this patch only, I can only assume that you are adding this as part
of the series, but without any context, I can't really review this,
sorry.

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Ard Biesheuvel]

On 1 November 2017@14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Mon, 2017-10-16@15:49 -0700, Bart Van Assche wrote:
>> Use the sgl_alloc() and sgl_free() functions instead of open coding
>> these functions.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> Cc: Herbert Xu <herbert at gondor.apana.org.au>
>
> Ard and/or Herbert, can you please have a look at this patch and let us know
> whether or not it looks fine to you?
>

The patch itself does not look unreasonable, but I can't find
sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
on this patch only, I can only assume that you are adding this as part
of the series, but without any context, I can't really review this,
sorry.

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

On Wed, 2017-11-01 at 15:17 +0000, Ard Biesheuvel wrote:
> On 1 November 2017 at 14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
> > > these functions.
> > > 
> > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > 
> > Ard and/or Herbert, can you please have a look at this patch and let us know
> > whether or not it looks fine to you?
> 
> The patch itself does not look unreasonable, but I can't find
> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
> on this patch only, I can only assume that you are adding this as part
> of the series, but without any context, I can't really review this,
> sorry.

Hello Ard,

Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
series as one can see here:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28485.html.

Thanks,

Bart.

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

T24gV2VkLCAyMDE3LTExLTAxIGF0IDE1OjE3ICswMDAwLCBBcmQgQmllc2hldXZlbCB3cm90ZToN
Cj4gT24gMSBOb3ZlbWJlciAyMDE3IGF0IDE0OjUwLCBCYXJ0IFZhbiBBc3NjaGUgPEJhcnQuVmFu
QXNzY2hlQHdkYy5jb20+IHdyb3RlOg0KPiA+IE9uIE1vbiwgMjAxNy0xMC0xNiBhdCAxNTo0OSAt
MDcwMCwgQmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiA+ID4gVXNlIHRoZSBzZ2xfYWxsb2MoKSBh
bmQgc2dsX2ZyZWUoKSBmdW5jdGlvbnMgaW5zdGVhZCBvZiBvcGVuIGNvZGluZw0KPiA+ID4gdGhl
c2UgZnVuY3Rpb25zLg0KPiA+ID4gDQo+ID4gPiBTaWduZWQtb2ZmLWJ5OiBCYXJ0IFZhbiBBc3Nj
aGUgPGJhcnQudmFuYXNzY2hlQHdkYy5jb20+DQo+ID4gPiBDYzogQXJkIEJpZXNoZXV2ZWwgPGFy
ZC5iaWVzaGV1dmVsQGxpbmFyby5vcmc+DQo+ID4gPiBDYzogSGVyYmVydCBYdSA8aGVyYmVydEBn
b25kb3IuYXBhbmEub3JnLmF1Pg0KPiA+IA0KPiA+IEFyZCBhbmQvb3IgSGVyYmVydCwgY2FuIHlv
dSBwbGVhc2UgaGF2ZSBhIGxvb2sgYXQgdGhpcyBwYXRjaCBhbmQgbGV0IHVzIGtub3cNCj4gPiB3
aGV0aGVyIG9yIG5vdCBpdCBsb29rcyBmaW5lIHRvIHlvdT8NCj4gDQo+IFRoZSBwYXRjaCBpdHNl
bGYgZG9lcyBub3QgbG9vayB1bnJlYXNvbmFibGUsIGJ1dCBJIGNhbid0IGZpbmQNCj4gc2dsX2Fs
bG9jKCkgYW55d2hlcmUgaW4gdGhlIHNvdXJjZSB0cmVlLiBHaXZlbiB0aGF0IHlvdSBoYXZlIGNj
J2VkIG1lDQo+IG9uIHRoaXMgcGF0Y2ggb25seSwgSSBjYW4gb25seSBhc3N1bWUgdGhhdCB5b3Ug
YXJlIGFkZGluZyB0aGlzIGFzIHBhcnQNCj4gb2YgdGhlIHNlcmllcywgYnV0IHdpdGhvdXQgYW55
IGNvbnRleHQsIEkgY2FuJ3QgcmVhbGx5IHJldmlldyB0aGlzLA0KPiBzb3JyeS4NCg0KSGVsbG8g
QXJkLA0KDQpEbyB5b3UgZXhwZWN0IHRvIGJlIENjLWVkIHBlcnNvbmFsbHkgb3IgaXMgQ2MtaW5n
IHRoZSBsaW51eC1jcnlwdG8gbWFpbGluZw0KbGlzdCBzdWZmaWNpZW50PyBUaGUgbGludXgtY3J5
cHRvIG1haWxpbmcgbGlzdCB3YXMgQ2MtZWQgZm9yIHRoZSBlbnRpcmUgcGF0Y2gNCnNlcmllcyBh
cyBvbmUgY2FuIHNlZSBoZXJlOg0KaHR0cHM6Ly93d3cubWFpbC1hcmNoaXZlLmNvbS9saW51eC1j
cnlwdG9Admdlci5rZXJuZWwub3JnL21zZzI4NDg1Lmh0bWwuDQoNClRoYW5rcywNCg0KQmFydC4=

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Bart Van Assche]

On Wed, 2017-11-01@15:17 +0000, Ard Biesheuvel wrote:
> On 1 November 2017@14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > On Mon, 2017-10-16@15:49 -0700, Bart Van Assche wrote:
> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
> > > these functions.
> > > 
> > > Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> > > Cc: Herbert Xu <herbert at gondor.apana.org.au>
> > 
> > Ard and/or Herbert, can you please have a look at this patch and let us know
> > whether or not it looks fine to you?
> 
> The patch itself does not look unreasonable, but I can't find
> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
> on this patch only, I can only assume that you are adding this as part
> of the series, but without any context, I can't really review this,
> sorry.

Hello Ard,

Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
series as one can see here:
https://www.mail-archive.com/linux-crypto at vger.kernel.org/msg28485.html.

Thanks,

Bart.

Re: [PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Ard Biesheuvel]

On 1 November 2017 at 15:45, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Wed, 2017-11-01 at 15:17 +0000, Ard Biesheuvel wrote:
>> On 1 November 2017 at 14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>> > On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
>> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
>> > > these functions.
>> > >
>> > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > > Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> >
>> > Ard and/or Herbert, can you please have a look at this patch and let us know
>> > whether or not it looks fine to you?
>>
>> The patch itself does not look unreasonable, but I can't find
>> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
>> on this patch only, I can only assume that you are adding this as part
>> of the series, but without any context, I can't really review this,
>> sorry.
>
> Hello Ard,
>
> Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
> list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
> series as one can see here:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28485.html.
>

I guess people's opinions may differ regarding what they want to be
cc'ed on, but in general, you should at least cc everyone on the cover
letter if you cc them on individual patches, and in my case, I'd
rather have the whole series even if only a single patch is relevant
to me.

[PATCH v2 2/8] crypto: scompress - use sgl_alloc() and sgl_free()

[Ard Biesheuvel]

On 1 November 2017@15:45, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Wed, 2017-11-01@15:17 +0000, Ard Biesheuvel wrote:
>> On 1 November 2017@14:50, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>> > On Mon, 2017-10-16@15:49 -0700, Bart Van Assche wrote:
>> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
>> > > these functions.
>> > >
>> > > Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
>> > > Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> > > Cc: Herbert Xu <herbert at gondor.apana.org.au>
>> >
>> > Ard and/or Herbert, can you please have a look at this patch and let us know
>> > whether or not it looks fine to you?
>>
>> The patch itself does not look unreasonable, but I can't find
>> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
>> on this patch only, I can only assume that you are adding this as part
>> of the series, but without any context, I can't really review this,
>> sorry.
>
> Hello Ard,
>
> Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
> list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
> series as one can see here:
> https://www.mail-archive.com/linux-crypto at vger.kernel.org/msg28485.html.
>

I guess people's opinions may differ regarding what they want to be
cc'ed on, but in general, you should at least cc everyone on the cover
letter if you cc them on individual patches, and in my case, I'd
rather have the whole series even if only a single patch is relevant
to me.