[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

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)

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 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

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)

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()

[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.

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()

[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 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

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 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

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 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

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)

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.

[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

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)

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

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()

[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

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 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

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)

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.

[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 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)