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

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

[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

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

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

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

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

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

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

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

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

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

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

[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

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

[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

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

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

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

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