[PATCH V5 00/16] use sg helper to operate scatterlist

[PATCH V5 00/16] use sg helper to operate scatterlist

[Ming Lei]

Hi,

Scsi MQ makes a large static allocation for the first scatter gather
list chunk for the driver to use.  This is a performance headache we'd
like to fix by reducing the size of the allocation to a 2 element
array.  Doing this will break the current guarantee that any driver
using SG_ALL doesn't need to use the scatterlist iterators and can get
away with directly dereferencing the array.  Thus we need to update all
drivers to use the scatterlist iterators and remove direct indexing of
the scatterlist array before reducing the initial scatterlist
allocation size in SCSI.

So convert drivers to use scatterlist helper.

There are two types of direct access on scatterlist in SCSI drivers:

1) operate on the scatterlist via scsi_sglist(scmd) directly, then one
local variable of 'struct scatterlist *' is involved.

2) scsi_sglist(scmd) is stored to cmd->SCp.buffer and the scatterlist is
used via cmd->SCp.buffer.

The following coccinelle semantic patch is developed for finding the
above two types of direct scatterlist uses:

	@@ struct scatterlist *p; @@
	(
	- ++p
	+ p = sg_next(p)
	|
	- p++
	+ p = sg_next(p)
	|
	- p = p + 1
	+ p = sg_next(p)
	|
	- p += 1
	+ p = sg_next(p)
	|
	- --p
	+ p = sg_non_exist_prev(p)
	|
	- p--
	+ p = sg_non_exist_prev(p)
	|
	- p = p - 1
	+ p = sg_non_exist_prev(p)
	|
	- p -= 1
	+ p = sg_non_exist_prev(p)
	)
	
	@@
	struct scatterlist *p;
	expression data != 0;
	@@
	- p[data]
	+ '!!!!!!use sg iterator helper!!!!!!'
	
	@@
	struct scatterlist[] p;
	expression data != 0;
	@@
	- p[data]
	+ '!!!!!!use sg iterator helper!!!!!!'
	
	
	@@ struct scsi_cmnd *scmd; @@
	(
	-	scmd->SCp.buffer++
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	++scmd->SCp.buffer
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer += 1
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer = scmd->SCp.buffer + 1
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer--
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	--scmd->SCp.buffer
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer -= 1
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer = scmd->SCp.buffer - 1
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	)
	
	@@
	struct scsi_cmnd *scmd;
	expression data != 0;
	@@
	- scmd->SCp.buffer[data]
	+ '!!!!!!use sg iterator helper!!!!!!'


The 1st 10 patches are for handling type #1, and the other 6 patches
for handling type #2, and all the 16 are found by the above coccinelle
semantic patch.

V5:
	- one patch style fix in 5/11
	- re-write convertion for 'staging: rtsx' as suggested by Christoph
	- fix another issue in aha152x by Finn, and change the author to
	  Finn now
	- add reviewed-by tag

V4:
	- fix building failure on pmcraid's conversion 
	- improve the coccinelle semantic patch to cover both two types of
	scatterlist direct use
	- driver 'staging: rtsx' is covered

V3:
	- update commit log and cover letter, most of words are from
	James Bottomley	

V2:
	- use coccinelle semantic patch for finding direct sgl uses from
	scsi command(9 drivers found)
	- run 'git grep -E "SCp.buffer"' to find direct sgl uses
	from SCp.buffer(6 drivers are found)



Finn Thain (2):
  scsi: aha152x: use sg helper to operate scatterlist
  NCR5380: Support chained sg lists

Ming Lei (14):
  scsi: vmw_pscsi: use sg helper to operate scatterlist
  scsi: advansys: use sg helper to operate scatterlist
  scsi: lpfc: use sg helper to operate scatterlist
  scsi: mvumi: use sg helper to operate scatterlist
  scsi: ipr: use sg helper to operate scatterlist
  scsi: pmcraid: use sg helper to operate scatterlist
  usb: image: microtek: use sg helper to operate scatterlist
  staging: unisys: visorhba: use sg helper to operate scatterlist
  staging: rtsx: use sg helper to operate scatterlist
  s390: zfcp_fc: use sg helper to operate scatterlist
  scsi: imm: use sg helper to operate scatterlist
  scsi: pcmcia: nsp_cs: use sg helper to operate scatterlist
  scsi: ppa: use sg helper to operate scatterlist
  scsi: wd33c93: use sg helper to operate scatterlist

 drivers/s390/scsi/zfcp_fc.c                   |  4 +-
 drivers/scsi/NCR5380.c                        | 41 ++++++++---------
 drivers/scsi/advansys.c                       |  2 +-
 drivers/scsi/aha152x.c                        | 46 +++++++++----------
 drivers/scsi/imm.c                            |  2 +-
 drivers/scsi/ipr.c                            | 29 ++++++------
 drivers/scsi/lpfc/lpfc_nvmet.c                |  3 +-
 drivers/scsi/mvumi.c                          |  9 ++--
 drivers/scsi/pcmcia/nsp_cs.c                  |  4 +-
 drivers/scsi/pmcraid.c                        | 14 +++---
 drivers/scsi/ppa.c                            |  2 +-
 drivers/scsi/vmw_pvscsi.c                     |  2 +-
 drivers/scsi/wd33c93.c                        |  2 +-
 drivers/staging/rts5208/rtsx_transport.c      | 32 ++++++-------
 drivers/staging/rts5208/rtsx_transport.h      |  2 +-
 drivers/staging/rts5208/spi.c                 | 14 +++---
 .../staging/unisys/visorhba/visorhba_main.c   |  9 ++--
 drivers/usb/image/microtek.c                  | 20 ++++----
 drivers/usb/image/microtek.h                  |  2 +-
 19 files changed, 115 insertions(+), 124 deletions(-)

-- 
2.20.1

[PATCH V5 01/16] scsi: vmw_pscsi: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the scatterlist
array.

This way allows us to pre-allocate one small scatterlist, which can be chained
with one runtime allocated scatterlist if the pre-allocated one isn't enough
for the whole request.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/vmw_pvscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmw_pvscsi.c
index ecee4b3ff073..d71abd416eb4 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmw_pvscsi.c
@@ -335,7 +335,7 @@ static void pvscsi_create_sg(struct pvscsi_ctx *ctx,
 	BUG_ON(count > PVSCSI_MAX_NUM_SG_ENTRIES_PER_SEGMENT);
 
 	sge = &ctx->sgl->sge[0];
-	for (i = 0; i < count; i++, sg++) {
+	for (i = 0; i < count; i++, sg = sg_next(sg)) {
 		sge[i].addr   = sg_dma_address(sg);
 		sge[i].length = sg_dma_len(sg);
 		sge[i].flags  = 0;
-- 
2.20.1

[PATCH V5 02/16] scsi: advansys: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/advansys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index d37584403c33..b87de8d3d844 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -7714,7 +7714,7 @@ adv_get_sglist(struct asc_board *boardp, adv_req_t *reqp,
 				sg_block->sg_ptr = 0L; /* Last ADV_SG_BLOCK in list. */
 				return ADV_SUCCESS;
 			}
-			slp++;
+			slp = sg_next(slp);
 		}
 		sg_block->sg_cnt = NO_OF_SG_PER_BLOCK;
 		prev_sg_block = sg_block;
-- 
2.20.1

[PATCH V5 03/16] scsi: lpfc: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one isn't
enough for the whole request.

Reviewed by: Ewan D. Milne <emilne@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/lpfc/lpfc_nvmet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index f3d9a5545164..3f803982bd1e 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -2887,8 +2887,7 @@ lpfc_nvmet_prep_fcp_wqe(struct lpfc_hba *phba,
 	nvmewqe->drvrTimeout = (phba->fc_ratov * 3) + LPFC_DRVR_TIMEOUT;
 	nvmewqe->context1 = ndlp;
 
-	for (i = 0; i < rsp->sg_cnt; i++) {
-		sgel = &rsp->sg[i];
+	for_each_sg(rsp->sg, sgel, rsp->sg_cnt, i) {
 		physaddr = sg_dma_address(sgel);
 		cnt = sg_dma_len(sgel);
 		sgl->addr_hi = putPaddrHigh(physaddr);
-- 
2.20.1

[PATCH V5 04/16] scsi: mvumi: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/mvumi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index a5410615edac..0022cd31500a 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -211,8 +211,7 @@ static int mvumi_make_sgl(struct mvumi_hba *mhba, struct scsi_cmnd *scmd,
 	unsigned int sgnum = scsi_sg_count(scmd);
 	dma_addr_t busaddr;
 
-	sg = scsi_sglist(scmd);
-	*sg_count = dma_map_sg(&mhba->pdev->dev, sg, sgnum,
+	*sg_count = dma_map_sg(&mhba->pdev->dev, scsi_sglist(scmd), sgnum,
 			       scmd->sc_data_direction);
 	if (*sg_count > mhba->max_sge) {
 		dev_err(&mhba->pdev->dev,
@@ -222,12 +221,12 @@ static int mvumi_make_sgl(struct mvumi_hba *mhba, struct scsi_cmnd *scmd,
 			     scmd->sc_data_direction);
 		return -1;
 	}
-	for (i = 0; i < *sg_count; i++) {
-		busaddr = sg_dma_address(&sg[i]);
+	scsi_for_each_sg(scmd, sg, *sg_count, i) {
+		busaddr = sg_dma_address(sg);
 		m_sg->baseaddr_l = cpu_to_le32(lower_32_bits(busaddr));
 		m_sg->baseaddr_h = cpu_to_le32(upper_32_bits(busaddr));
 		m_sg->flags = 0;
-		sgd_setsz(mhba, m_sg, cpu_to_le32(sg_dma_len(&sg[i])));
+		sgd_setsz(mhba, m_sg, cpu_to_le32(sg_dma_len(sg)));
 		if ((i + 1) == *sg_count)
 			m_sg->flags |= 1U << mhba->eot_flag;
 
-- 
2.20.1

[PATCH V5 05/16] scsi: ipr: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/ipr.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 6d053e220153..bf17540affbc 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -3915,22 +3915,23 @@ static int ipr_copy_ucode_buffer(struct ipr_sglist *sglist,
 				 u8 *buffer, u32 len)
 {
 	int bsize_elem, i, result = 0;
-	struct scatterlist *scatterlist;
+	struct scatterlist *sg;
 	void *kaddr;
 
 	/* Determine the actual number of bytes per element */
 	bsize_elem = PAGE_SIZE * (1 << sglist->order);
 
-	scatterlist = sglist->scatterlist;
+	sg = sglist->scatterlist;
 
-	for (i = 0; i < (len / bsize_elem); i++, buffer += bsize_elem) {
-		struct page *page = sg_page(&scatterlist[i]);
+	for (i = 0; i < (len / bsize_elem); i++, sg = sg_next(sg),
+			buffer += bsize_elem) {
+		struct page *page = sg_page(sg);
 
 		kaddr = kmap(page);
 		memcpy(kaddr, buffer, bsize_elem);
 		kunmap(page);
 
-		scatterlist[i].length = bsize_elem;
+		sg->length = bsize_elem;
 
 		if (result != 0) {
 			ipr_trace;
@@ -3939,13 +3940,13 @@ static int ipr_copy_ucode_buffer(struct ipr_sglist *sglist,
 	}
 
 	if (len % bsize_elem) {
-		struct page *page = sg_page(&scatterlist[i]);
+		struct page *page = sg_page(sg);
 
 		kaddr = kmap(page);
 		memcpy(kaddr, buffer, len % bsize_elem);
 		kunmap(page);
 
-		scatterlist[i].length = len % bsize_elem;
+		sg->length = len % bsize_elem;
 	}
 
 	sglist->buffer_len = len;
@@ -3966,6 +3967,7 @@ static void ipr_build_ucode_ioadl64(struct ipr_cmnd *ipr_cmd,
 	struct ipr_ioarcb *ioarcb = &ipr_cmd->ioarcb;
 	struct ipr_ioadl64_desc *ioadl64 = ipr_cmd->i.ioadl64;
 	struct scatterlist *scatterlist = sglist->scatterlist;
+	struct scatterlist *sg;
 	int i;
 
 	ipr_cmd->dma_use_sg = sglist->num_dma_sg;
@@ -3974,10 +3976,10 @@ static void ipr_build_ucode_ioadl64(struct ipr_cmnd *ipr_cmd,
 
 	ioarcb->ioadl_len =
 		cpu_to_be32(sizeof(struct ipr_ioadl64_desc) * ipr_cmd->dma_use_sg);
-	for (i = 0; i < ipr_cmd->dma_use_sg; i++) {
+	for_each_sg(scatterlist, sg, ipr_cmd->dma_use_sg, i) {
 		ioadl64[i].flags = cpu_to_be32(IPR_IOADL_FLAGS_WRITE);
-		ioadl64[i].data_len = cpu_to_be32(sg_dma_len(&scatterlist[i]));
-		ioadl64[i].address = cpu_to_be64(sg_dma_address(&scatterlist[i]));
+		ioadl64[i].data_len = cpu_to_be32(sg_dma_len(sg));
+		ioadl64[i].address = cpu_to_be64(sg_dma_address(sg));
 	}
 
 	ioadl64[i-1].flags |= cpu_to_be32(IPR_IOADL_FLAGS_LAST);
@@ -3997,6 +3999,7 @@ static void ipr_build_ucode_ioadl(struct ipr_cmnd *ipr_cmd,
 	struct ipr_ioarcb *ioarcb = &ipr_cmd->ioarcb;
 	struct ipr_ioadl_desc *ioadl = ipr_cmd->i.ioadl;
 	struct scatterlist *scatterlist = sglist->scatterlist;
+	struct scatterlist *sg;
 	int i;
 
 	ipr_cmd->dma_use_sg = sglist->num_dma_sg;
@@ -4006,11 +4009,11 @@ static void ipr_build_ucode_ioadl(struct ipr_cmnd *ipr_cmd,
 	ioarcb->ioadl_len =
 		cpu_to_be32(sizeof(struct ipr_ioadl_desc) * ipr_cmd->dma_use_sg);
 
-	for (i = 0; i < ipr_cmd->dma_use_sg; i++) {
+	for_each_sg(scatterlist, sg, ipr_cmd->dma_use_sg, i) {
 		ioadl[i].flags_and_data_len =
-			cpu_to_be32(IPR_IOADL_FLAGS_WRITE | sg_dma_len(&scatterlist[i]));
+			cpu_to_be32(IPR_IOADL_FLAGS_WRITE | sg_dma_len(sg));
 		ioadl[i].address =
-			cpu_to_be32(sg_dma_address(&scatterlist[i]));
+			cpu_to_be32(sg_dma_address(sg));
 	}
 
 	ioadl[i-1].flags_and_data_len |=
-- 
2.20.1

[PATCH V5 06/16] scsi: pmcraid: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/pmcraid.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index e338d7a4f571..286cac59cb5f 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3270,7 +3270,7 @@ static int pmcraid_copy_sglist(
 	int direction
 )
 {
-	struct scatterlist *scatterlist;
+	struct scatterlist *sg;
 	void *kaddr;
 	int bsize_elem;
 	int i;
@@ -3279,10 +3279,10 @@ static int pmcraid_copy_sglist(
 	/* Determine the actual number of bytes per element */
 	bsize_elem = PAGE_SIZE * (1 << sglist->order);
 
-	scatterlist = sglist->scatterlist;
+	sg = sglist->scatterlist;
 
-	for (i = 0; i < (len / bsize_elem); i++, buffer += bsize_elem) {
-		struct page *page = sg_page(&scatterlist[i]);
+	for (i = 0; i < (len / bsize_elem); i++, sg = sg_next(sg), buffer += bsize_elem) {
+		struct page *page = sg_page(sg);
 
 		kaddr = kmap(page);
 		if (direction == DMA_TO_DEVICE)
@@ -3297,11 +3297,11 @@ static int pmcraid_copy_sglist(
 			return -EFAULT;
 		}
 
-		scatterlist[i].length = bsize_elem;
+		sg->length = bsize_elem;
 	}
 
 	if (len % bsize_elem) {
-		struct page *page = sg_page(&scatterlist[i]);
+		struct page *page = sg_page(sg);
 
 		kaddr = kmap(page);
 
@@ -3312,7 +3312,7 @@ static int pmcraid_copy_sglist(
 
 		kunmap(page);
 
-		scatterlist[i].length = len % bsize_elem;
+		sg->length = len % bsize_elem;
 	}
 
 	if (rc) {
-- 
2.20.1

[PATCH V5 07/16] usb: image: microtek: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Cc: Oliver Neukum <oliver@neukum.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/usb/image/microtek.c | 20 ++++++++------------
 drivers/usb/image/microtek.h |  2 +-
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
index 607be1f4fe27..0a57c2cc8e5a 100644
--- a/drivers/usb/image/microtek.c
+++ b/drivers/usb/image/microtek.c
@@ -488,7 +488,6 @@ static void mts_command_done( struct urb *transfer )
 
 static void mts_do_sg (struct urb* transfer)
 {
-	struct scatterlist * sg;
 	int status = transfer->status;
 	MTS_INT_INIT();
 
@@ -500,13 +499,12 @@ static void mts_do_sg (struct urb* transfer)
 		mts_transfer_cleanup(transfer);
         }
 
-	sg = scsi_sglist(context->srb);
-	context->fragment++;
+	context->curr_sg = sg_next(context->curr_sg);
 	mts_int_submit_urb(transfer,
 			   context->data_pipe,
-			   sg_virt(&sg[context->fragment]),
-			   sg[context->fragment].length,
-			   context->fragment + 1 == scsi_sg_count(context->srb) ?
+			   sg_virt(context->curr_sg),
+			   context->curr_sg->length,
+			   sg_is_last(context->curr_sg) ?
 			   mts_data_done : mts_do_sg);
 }
 
@@ -526,22 +524,20 @@ static void
 mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
 {
 	int pipe;
-	struct scatterlist * sg;
-	
+
 	MTS_DEBUG_GOT_HERE();
 
 	desc->context.instance = desc;
 	desc->context.srb = srb;
-	desc->context.fragment = 0;
 
 	if (!scsi_bufflen(srb)) {
 		desc->context.data = NULL;
 		desc->context.data_length = 0;
 		return;
 	} else {
-		sg = scsi_sglist(srb);
-		desc->context.data = sg_virt(&sg[0]);
-		desc->context.data_length = sg[0].length;
+		desc->context.curr_sg = scsi_sglist(srb);
+		desc->context.data = sg_virt(desc->context.curr_sg);
+		desc->context.data_length = desc->context.curr_sg->length;
 	}
 
 
diff --git a/drivers/usb/image/microtek.h b/drivers/usb/image/microtek.h
index 66685e59241a..7bd5f4639c4a 100644
--- a/drivers/usb/image/microtek.h
+++ b/drivers/usb/image/microtek.h
@@ -21,7 +21,7 @@ struct mts_transfer_context
 	void *data;
 	unsigned data_length;
 	int data_pipe;
-	int fragment;
+	struct scatterlist *curr_sg;
 
 	u8 *scsi_status; /* status returned from ep_response after command completion */
 };
-- 
2.20.1

[PATCH V5 08/16] staging: unisys: visorhba: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Cc: devel@driverdev.osuosl.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/staging/unisys/visorhba/visorhba_main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 2dad36a05518..dd979ee4dcf1 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -871,12 +871,11 @@ static void do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
 			return;
 		}
 
-		sg = scsi_sglist(scsicmd);
-		for (i = 0; i < scsi_sg_count(scsicmd); i++) {
-			this_page_orig = kmap_atomic(sg_page(sg + i));
+		scsi_for_each_sg(scsicmd, sg, scsi_sg_count(scsicmd), i) {
+			this_page_orig = kmap_atomic(sg_page(sg));
 			this_page = (void *)((unsigned long)this_page_orig |
-					     sg[i].offset);
-			memcpy(this_page, buf + bufind, sg[i].length);
+					     sg->offset);
+			memcpy(this_page, buf + bufind, sg->length);
 			kunmap_atomic(this_page_orig);
 		}
 		kfree(buf);
-- 
2.20.1

[PATCH V5 09/16] staging: rtsx: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Cc: Kim Bradley <kim.jamie.bradley@gmail.com>
Cc: devel@driverdev.osuosl.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/staging/rts5208/rtsx_transport.c | 32 +++++++++++-------------
 drivers/staging/rts5208/rtsx_transport.h |  2 +-
 drivers/staging/rts5208/spi.c            | 14 ++++++-----
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c
index 8277d7895608..7a9f42ccebec 100644
--- a/drivers/staging/rts5208/rtsx_transport.c
+++ b/drivers/staging/rts5208/rtsx_transport.c
@@ -32,7 +32,7 @@
 unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer,
 				       unsigned int buflen,
 				       struct scsi_cmnd *srb,
-				       unsigned int *index,
+				       struct scatterlist **sg,
 				       unsigned int *offset,
 				       enum xfer_buf_dir dir)
 {
@@ -60,23 +60,20 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer,
 	 * each page has to be kmap()'ed separately.
 	 */
 	} else {
-		struct scatterlist *sg =
-				(struct scatterlist *)scsi_sglist(srb)
-				+ *index;
-
 		/*
 		 * This loop handles a single s-g list entry, which may
 		 * include multiple pages.  Find the initial page structure
 		 * and the starting offset within the page, and update
-		 * the *offset and *index values for the next loop.
+		 * the *offset and current sg for the next loop.
 		 */
 		cnt = 0;
-		while (cnt < buflen && *index < scsi_sg_count(srb)) {
-			struct page *page = sg_page(sg) +
-					((sg->offset + *offset) >> PAGE_SHIFT);
-			unsigned int poff = (sg->offset + *offset) &
+		while (cnt < buflen && *sg) {
+			struct page *page = sg_page(*sg) +
+					(((*sg)->offset + *offset) >>
+					 PAGE_SHIFT);
+			unsigned int poff = ((*sg)->offset + *offset) &
 					    (PAGE_SIZE - 1);
-			unsigned int sglen = sg->length - *offset;
+			unsigned int sglen = (*sg)->length - *offset;
 
 			if (sglen > buflen - cnt) {
 				/* Transfer ends within this s-g entry */
@@ -85,8 +82,7 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer,
 			} else {
 				/* Transfer continues to next s-g entry */
 				*offset = 0;
-				++*index;
-				++sg;
+				*sg = sg_next(*sg);
 			}
 
 			while (sglen > 0) {
@@ -120,9 +116,10 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer,
 void rtsx_stor_set_xfer_buf(unsigned char *buffer,
 			    unsigned int buflen, struct scsi_cmnd *srb)
 {
-	unsigned int index = 0, offset = 0;
+	unsigned int offset = 0;
+	struct scatterlist *sg = scsi_sglist(srb);
 
-	rtsx_stor_access_xfer_buf(buffer, buflen, srb, &index, &offset,
+	rtsx_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 				  TO_XFER_BUF);
 	if (buflen < scsi_bufflen(srb))
 		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
@@ -131,9 +128,10 @@ void rtsx_stor_set_xfer_buf(unsigned char *buffer,
 void rtsx_stor_get_xfer_buf(unsigned char *buffer,
 			    unsigned int buflen, struct scsi_cmnd *srb)
 {
-	unsigned int index = 0, offset = 0;
+	unsigned int offset = 0;
+	struct scatterlist *sg = scsi_sglist(srb);
 
-	rtsx_stor_access_xfer_buf(buffer, buflen, srb, &index, &offset,
+	rtsx_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
 				  FROM_XFER_BUF);
 	if (buflen < scsi_bufflen(srb))
 		scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
diff --git a/drivers/staging/rts5208/rtsx_transport.h b/drivers/staging/rts5208/rtsx_transport.h
index 097efed24b79..e3ebc3759d92 100644
--- a/drivers/staging/rts5208/rtsx_transport.h
+++ b/drivers/staging/rts5208/rtsx_transport.h
@@ -20,7 +20,7 @@
 unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer,
 				       unsigned int buflen,
 				       struct scsi_cmnd *srb,
-				       unsigned int *index,
+				       struct scatterlist **sg,
 				       unsigned int *offset,
 				       enum xfer_buf_dir dir);
 void rtsx_stor_set_xfer_buf(unsigned char *buffer, unsigned int buflen,
diff --git a/drivers/staging/rts5208/spi.c b/drivers/staging/rts5208/spi.c
index f1e9e80044ed..af10916ff00b 100644
--- a/drivers/staging/rts5208/spi.c
+++ b/drivers/staging/rts5208/spi.c
@@ -554,7 +554,8 @@ int spi_read_flash_id(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 int spi_read_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 {
 	int retval;
-	unsigned int index = 0, offset = 0;
+	unsigned int offset = 0;
+	struct scatterlist *sg = scsi_sglist(srb);
 	u8 ins, slow_read;
 	u32 addr;
 	u16 len;
@@ -631,7 +632,7 @@ int spi_read_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 			return STATUS_FAIL;
 		}
 
-		rtsx_stor_access_xfer_buf(buf, pagelen, srb, &index, &offset,
+		rtsx_stor_access_xfer_buf(buf, pagelen, srb, &sg, &offset,
 					  TO_XFER_BUF);
 
 		addr += pagelen;
@@ -651,7 +652,8 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 	u32 addr;
 	u16 len;
 	u8 *buf;
-	unsigned int index = 0, offset = 0;
+	unsigned int offset = 0;
+	struct scatterlist *sg = scsi_sglist(srb);
 
 	spi_set_err_code(chip, SPI_NO_ERR);
 
@@ -679,7 +681,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 				return STATUS_FAIL;
 			}
 
-			rtsx_stor_access_xfer_buf(buf, 1, srb, &index, &offset,
+			rtsx_stor_access_xfer_buf(buf, 1, srb, &sg, &offset,
 						  FROM_XFER_BUF);
 
 			rtsx_init_cmd(chip);
@@ -722,7 +724,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 			return STATUS_ERROR;
 
 		while (len) {
-			rtsx_stor_access_xfer_buf(buf, 1, srb, &index, &offset,
+			rtsx_stor_access_xfer_buf(buf, 1, srb, &sg, &offset,
 						  FROM_XFER_BUF);
 
 			rtsx_init_cmd(chip);
@@ -788,7 +790,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 
 			rtsx_send_cmd_no_wait(chip);
 
-			rtsx_stor_access_xfer_buf(buf, pagelen, srb, &index,
+			rtsx_stor_access_xfer_buf(buf, pagelen, srb, &sg,
 						  &offset, FROM_XFER_BUF);
 
 			retval = rtsx_transfer_data(chip, 0, buf, pagelen, 0,
-- 
2.20.1

[PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Cc: Steffen Maier <maier@linux.ibm.com>
Cc: Benjamin Block <bblock@linux.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Acked-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/s390/scsi/zfcp_fc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
index 33eddb02ee30..b018b61bd168 100644
--- a/drivers/s390/scsi/zfcp_fc.c
+++ b/drivers/s390/scsi/zfcp_fc.c
@@ -620,7 +620,7 @@ static void zfcp_fc_sg_free_table(struct scatterlist *sg, int count)
 {
 	int i;
 
-	for (i = 0; i < count; i++, sg++)
+	for (i = 0; i < count; i++, sg = sg_next(sg))
 		if (sg)
 			free_page((unsigned long) sg_virt(sg));
 		else
@@ -641,7 +641,7 @@ static int zfcp_fc_sg_setup_table(struct scatterlist *sg, int count)
 	int i;
 
 	sg_init_table(sg, count);
-	for (i = 0; i < count; i++, sg++) {
+	for (i = 0; i < count; i++, sg = sg_next(sg)) {
 		addr = (void *) get_zeroed_page(GFP_KERNEL);
 		if (!addr) {
 			zfcp_fc_sg_free_table(sg, i);
-- 
2.20.1

[PATCH V5 11/16] scsi: aha152x: use sg helper to operate scatterlist

[Ming Lei]

From: Finn Thain <fthain@telegraphics.com.au>

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Finn added the change to replace SCp.buffers_residual with sg_is_last()
for fixing updating it, and the similar change has been applied on
NCR5380.c

Cc: Finn Thain <fthain@telegraphics.com.au>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/aha152x.c | 46 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 97872838b983..f07f3fa9b58d 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
 	   SCp.ptr              : buffer pointer
 	   SCp.this_residual    : buffer length
 	   SCp.buffer           : next buffer
-	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
 
 	if ((phase & resetting) || !scsi_sglist(SCpnt)) {
@@ -956,13 +955,11 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
 		SCpnt->SCp.this_residual = 0;
 		scsi_set_resid(SCpnt, 0);
 		SCpnt->SCp.buffer           = NULL;
-		SCpnt->SCp.buffers_residual = 0;
 	} else {
 		scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
 		SCpnt->SCp.buffer           = scsi_sglist(SCpnt);
 		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);
 		SCpnt->SCp.this_residual    = SCpnt->SCp.buffer->length;
-		SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
 	}
 
 	DO_LOCK(flags);
@@ -2030,10 +2027,9 @@ static void datai_run(struct Scsi_Host *shpnt)
 				}
 
 				if (CURRENT_SC->SCp.this_residual == 0 &&
-				    CURRENT_SC->SCp.buffers_residual > 0) {
+				    !sg_is_last(CURRENT_SC->SCp.buffer)) {
 					/* advance to next buffer */
-					CURRENT_SC->SCp.buffers_residual--;
-					CURRENT_SC->SCp.buffer++;
+					CURRENT_SC->SCp.buffer = sg_next(CURRENT_SC->SCp.buffer);
 					CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer);
 					CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length;
 				}
@@ -2136,10 +2132,10 @@ static void datao_run(struct Scsi_Host *shpnt)
 			CMD_INC_RESID(CURRENT_SC, -2 * data_count);
 		}
 
-		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {
+		if (CURRENT_SC->SCp.this_residual == 0 &&
+		    !sg_is_last(CURRENT_SC->SCp.buffer)) {
 			/* advance to next buffer */
-			CURRENT_SC->SCp.buffers_residual--;
-			CURRENT_SC->SCp.buffer++;
+			CURRENT_SC->SCp.buffer = sg_next(CURRENT_SC->SCp.buffer);
 			CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer);
 			CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length;
 		}
@@ -2158,22 +2154,26 @@ static void datao_run(struct Scsi_Host *shpnt)
 static void datao_end(struct Scsi_Host *shpnt)
 {
 	if(TESTLO(DMASTAT, DFIFOEMP)) {
-		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
-			GETSTCNT();
+		u32 datao_cnt = GETSTCNT();
+		int datao_out = DATA_LEN - scsi_get_resid(CURRENT_SC);
+		int done;
+		struct scatterlist *sg = scsi_sglist(CURRENT_SC);
 
-		CMD_INC_RESID(CURRENT_SC, data_count);
+		CMD_INC_RESID(CURRENT_SC, datao_out - datao_cnt);
 
-		data_count -= CURRENT_SC->SCp.ptr -
-			SG_ADDRESS(CURRENT_SC->SCp.buffer);
-		while(data_count>0) {
-			CURRENT_SC->SCp.buffer--;
-			CURRENT_SC->SCp.buffers_residual++;
-			data_count -= CURRENT_SC->SCp.buffer->length;
+		done = scsi_bufflen(CURRENT_SC) - scsi_get_resid(CURRENT_SC);
+		/* Locate the first SG entry not yet sent */
+		while (done > 0 && !sg_is_last(sg)) {
+			if (done < sg->length)
+				break;
+			done -= sg->length;
+			sg = sg_next(sg);
 		}
-		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
-			data_count;
-		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
-			data_count;
+
+		CURRENT_SC->SCp.buffer = sg;
+		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) + done;
+		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length -
+			done;
 	}
 
 	SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
@@ -2501,7 +2501,7 @@ static void get_command(struct seq_file *m, struct scsi_cmnd * ptr)
 
 	seq_printf(m, "); resid=%d; residual=%d; buffers=%d; phase |",
 		scsi_get_resid(ptr), ptr->SCp.this_residual,
-		ptr->SCp.buffers_residual);
+		sg_nents(ptr->SCp.buffer) - 1);
 
 	if (ptr->SCp.phase & not_issued)
 		seq_puts(m, "not issued|");
-- 
2.20.1

[PATCH V5 12/16] scsi: imm: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/imm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/imm.c b/drivers/scsi/imm.c
index 64ae418d29f3..56d29f157749 100644
--- a/drivers/scsi/imm.c
+++ b/drivers/scsi/imm.c
@@ -686,7 +686,7 @@ static int imm_completion(struct scsi_cmnd *cmd)
 		if (cmd->SCp.buffer && !cmd->SCp.this_residual) {
 			/* if scatter/gather, advance to the next segment */
 			if (cmd->SCp.buffers_residual--) {
-				cmd->SCp.buffer++;
+				cmd->SCp.buffer = sg_next(cmd->SCp.buffer);
 				cmd->SCp.this_residual =
 				    cmd->SCp.buffer->length;
 				cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
-- 
2.20.1

[PATCH V5 13/16] scsi: pcmcia: nsp_cs: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/pcmcia/nsp_cs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pcmcia/nsp_cs.c b/drivers/scsi/pcmcia/nsp_cs.c
index a81748e6e8fb..97416e1dcc5b 100644
--- a/drivers/scsi/pcmcia/nsp_cs.c
+++ b/drivers/scsi/pcmcia/nsp_cs.c
@@ -789,7 +789,7 @@ static void nsp_pio_read(struct scsi_cmnd *SCpnt)
 		    SCpnt->SCp.buffers_residual != 0 ) {
 			//nsp_dbg(NSP_DEBUG_DATA_IO, "scatterlist next timeout=%d", time_out);
 			SCpnt->SCp.buffers_residual--;
-			SCpnt->SCp.buffer++;
+			SCpnt->SCp.buffer = sg_next(SCpnt->SCp.buffer);
 			SCpnt->SCp.ptr		 = BUFFER_ADDR;
 			SCpnt->SCp.this_residual = SCpnt->SCp.buffer->length;
 			time_out = 1000;
@@ -887,7 +887,7 @@ static void nsp_pio_write(struct scsi_cmnd *SCpnt)
 		    SCpnt->SCp.buffers_residual != 0 ) {
 			//nsp_dbg(NSP_DEBUG_DATA_IO, "scatterlist next");
 			SCpnt->SCp.buffers_residual--;
-			SCpnt->SCp.buffer++;
+			SCpnt->SCp.buffer = sg_next(SCpnt->SCp.buffer);
 			SCpnt->SCp.ptr		 = BUFFER_ADDR;
 			SCpnt->SCp.this_residual = SCpnt->SCp.buffer->length;
 			time_out = 1000;
-- 
2.20.1

[PATCH V5 14/16] scsi: ppa: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/ppa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
index 35213082e933..a406cc825426 100644
--- a/drivers/scsi/ppa.c
+++ b/drivers/scsi/ppa.c
@@ -590,7 +590,7 @@ static int ppa_completion(struct scsi_cmnd *cmd)
 		if (cmd->SCp.buffer && !cmd->SCp.this_residual) {
 			/* if scatter/gather, advance to the next segment */
 			if (cmd->SCp.buffers_residual--) {
-				cmd->SCp.buffer++;
+				cmd->SCp.buffer = sg_next(cmd->SCp.buffer);
 				cmd->SCp.this_residual =
 				    cmd->SCp.buffer->length;
 				cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
-- 
2.20.1

[PATCH V5 15/16] scsi: wd33c93: use sg helper to operate scatterlist

[Ming Lei]

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/wd33c93.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
index 74be04f2357c..ae5935c0a149 100644
--- a/drivers/scsi/wd33c93.c
+++ b/drivers/scsi/wd33c93.c
@@ -744,7 +744,7 @@ transfer_bytes(const wd33c93_regs regs, struct scsi_cmnd *cmd,
  * source or destination for THIS transfer.
  */
 	if (!cmd->SCp.this_residual && cmd->SCp.buffers_residual) {
-		++cmd->SCp.buffer;
+		cmd->SCp.buffer = sg_next(cmd->SCp.buffer);
 		--cmd->SCp.buffers_residual;
 		cmd->SCp.this_residual = cmd->SCp.buffer->length;
 		cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
-- 
2.20.1

[PATCH V5 16/16] NCR5380: Support chained sg lists

[Ming Lei]

From: Finn Thain <fthain@telegraphics.com.au>

My understanding is that support for chained scatterlists is to
become mandatory for LLDs.

Use the scatterlist iterators and remove direct indexing of the
scatterlist array.

This way allows us to pre-allocate one small scatterlist, which can be
chained with one runtime allocated scatterlist if the pre-allocated one
isn't enough for the whole request.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Reviewed-by: Michael Schmitz <schmitzmic@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index fe0535affc14..4ef44fafe6ca 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -149,12 +149,10 @@ static inline void initialize_SCp(struct scsi_cmnd *cmd)
 
 	if (scsi_bufflen(cmd)) {
 		cmd->SCp.buffer = scsi_sglist(cmd);
-		cmd->SCp.buffers_residual = scsi_sg_count(cmd) - 1;
 		cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
 		cmd->SCp.this_residual = cmd->SCp.buffer->length;
 	} else {
 		cmd->SCp.buffer = NULL;
-		cmd->SCp.buffers_residual = 0;
 		cmd->SCp.ptr = NULL;
 		cmd->SCp.this_residual = 0;
 	}
@@ -163,6 +161,17 @@ static inline void initialize_SCp(struct scsi_cmnd *cmd)
 	cmd->SCp.Message = 0;
 }
 
+static inline void advance_sg_buffer(struct scsi_cmnd *cmd)
+{
+	struct scatterlist *s = cmd->SCp.buffer;
+
+	if (!cmd->SCp.this_residual && s && !sg_is_last(s)) {
+		cmd->SCp.buffer = sg_next(s);
+		cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
+		cmd->SCp.this_residual = cmd->SCp.buffer->length;
+	}
+}
+
 /**
  * NCR5380_poll_politely2 - wait for two chip register values
  * @hostdata: host private data
@@ -1672,12 +1681,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 			    sun3_dma_setup_done != cmd) {
 				int count;
 
-				if (!cmd->SCp.this_residual && cmd->SCp.buffers_residual) {
-					++cmd->SCp.buffer;
-					--cmd->SCp.buffers_residual;
-					cmd->SCp.this_residual = cmd->SCp.buffer->length;
-					cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
-				}
+				advance_sg_buffer(cmd);
 
 				count = sun3scsi_dma_xfer_len(hostdata, cmd);
 
@@ -1727,15 +1731,11 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 				 * scatter-gather list, move onto the next one.
 				 */
 
-				if (!cmd->SCp.this_residual && cmd->SCp.buffers_residual) {
-					++cmd->SCp.buffer;
-					--cmd->SCp.buffers_residual;
-					cmd->SCp.this_residual = cmd->SCp.buffer->length;
-					cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
-					dsprintk(NDEBUG_INFORMATION, instance, "%d bytes and %d buffers left\n",
-					         cmd->SCp.this_residual,
-					         cmd->SCp.buffers_residual);
-				}
+				advance_sg_buffer(cmd);
+				dsprintk(NDEBUG_INFORMATION, instance,
+					"this residual %d, sg ents %d\n",
+					cmd->SCp.this_residual,
+					sg_nents(cmd->SCp.buffer));
 
 				/*
 				 * The preferred transfer method is going to be
@@ -2136,12 +2136,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 	if (sun3_dma_setup_done != tmp) {
 		int count;
 
-		if (!tmp->SCp.this_residual && tmp->SCp.buffers_residual) {
-			++tmp->SCp.buffer;
-			--tmp->SCp.buffers_residual;
-			tmp->SCp.this_residual = tmp->SCp.buffer->length;
-			tmp->SCp.ptr = sg_virt(tmp->SCp.buffer);
-		}
+		advance_sg_buffer(tmp);
 
 		count = sun3scsi_dma_xfer_len(hostdata, tmp);
 
-- 
2.20.1

Re: [PATCH V5 11/16] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

On Tue, 18 Jun 2019, Ming Lei wrote:

> From: Finn Thain <fthain@telegraphics.com.au>
> 
> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Finn added the change to replace SCp.buffers_residual with sg_is_last()
> for fixing updating it, and the similar change has been applied on
> NCR5380.c
> 
> Cc: Finn Thain <fthain@telegraphics.com.au>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

> ---
>  drivers/scsi/aha152x.c | 46 +++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index 97872838b983..f07f3fa9b58d 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
>  	   SCp.ptr              : buffer pointer
>  	   SCp.this_residual    : buffer length
>  	   SCp.buffer           : next buffer
> -	   SCp.buffers_residual : left buffers in list
>  	   SCp.phase            : current state of the command */
>  
>  	if ((phase & resetting) || !scsi_sglist(SCpnt)) {
> @@ -956,13 +955,11 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
>  		SCpnt->SCp.this_residual = 0;
>  		scsi_set_resid(SCpnt, 0);
>  		SCpnt->SCp.buffer           = NULL;
> -		SCpnt->SCp.buffers_residual = 0;
>  	} else {
>  		scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
>  		SCpnt->SCp.buffer           = scsi_sglist(SCpnt);
>  		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);
>  		SCpnt->SCp.this_residual    = SCpnt->SCp.buffer->length;
> -		SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
>  	}
>  
>  	DO_LOCK(flags);
> @@ -2030,10 +2027,9 @@ static void datai_run(struct Scsi_Host *shpnt)
>  				}
>  
>  				if (CURRENT_SC->SCp.this_residual == 0 &&
> -				    CURRENT_SC->SCp.buffers_residual > 0) {
> +				    !sg_is_last(CURRENT_SC->SCp.buffer)) {
>  					/* advance to next buffer */
> -					CURRENT_SC->SCp.buffers_residual--;
> -					CURRENT_SC->SCp.buffer++;
> +					CURRENT_SC->SCp.buffer = sg_next(CURRENT_SC->SCp.buffer);
>  					CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer);
>  					CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length;
>  				}
> @@ -2136,10 +2132,10 @@ static void datao_run(struct Scsi_Host *shpnt)
>  			CMD_INC_RESID(CURRENT_SC, -2 * data_count);
>  		}
>  
> -		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {
> +		if (CURRENT_SC->SCp.this_residual == 0 &&
> +		    !sg_is_last(CURRENT_SC->SCp.buffer)) {
>  			/* advance to next buffer */
> -			CURRENT_SC->SCp.buffers_residual--;
> -			CURRENT_SC->SCp.buffer++;
> +			CURRENT_SC->SCp.buffer = sg_next(CURRENT_SC->SCp.buffer);
>  			CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer);
>  			CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length;
>  		}
> @@ -2158,22 +2154,26 @@ static void datao_run(struct Scsi_Host *shpnt)
>  static void datao_end(struct Scsi_Host *shpnt)
>  {
>  	if(TESTLO(DMASTAT, DFIFOEMP)) {
> -		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
> -			GETSTCNT();
> +		u32 datao_cnt = GETSTCNT();
> +		int datao_out = DATA_LEN - scsi_get_resid(CURRENT_SC);
> +		int done;
> +		struct scatterlist *sg = scsi_sglist(CURRENT_SC);
>  
> -		CMD_INC_RESID(CURRENT_SC, data_count);
> +		CMD_INC_RESID(CURRENT_SC, datao_out - datao_cnt);
>  
> -		data_count -= CURRENT_SC->SCp.ptr -
> -			SG_ADDRESS(CURRENT_SC->SCp.buffer);
> -		while(data_count>0) {
> -			CURRENT_SC->SCp.buffer--;
> -			CURRENT_SC->SCp.buffers_residual++;
> -			data_count -= CURRENT_SC->SCp.buffer->length;
> +		done = scsi_bufflen(CURRENT_SC) - scsi_get_resid(CURRENT_SC);
> +		/* Locate the first SG entry not yet sent */
> +		while (done > 0 && !sg_is_last(sg)) {
> +			if (done < sg->length)
> +				break;
> +			done -= sg->length;
> +			sg = sg_next(sg);
>  		}
> -		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
> -			data_count;
> -		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
> -			data_count;
> +
> +		CURRENT_SC->SCp.buffer = sg;
> +		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) + done;
> +		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length -
> +			done;
>  	}
>  
>  	SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
> @@ -2501,7 +2501,7 @@ static void get_command(struct seq_file *m, struct scsi_cmnd * ptr)
>  
>  	seq_printf(m, "); resid=%d; residual=%d; buffers=%d; phase |",
>  		scsi_get_resid(ptr), ptr->SCp.this_residual,
> -		ptr->SCp.buffers_residual);
> +		sg_nents(ptr->SCp.buffer) - 1);
>  
>  	if (ptr->SCp.phase & not_issued)
>  		seq_puts(m, "not issued|");
> 

Re: [PATCH V5 00/16] use sg helper to operate scatterlist

[Martin K. Petersen]

Hi Ming,

> So convert drivers to use scatterlist helper.

I applied this series with a bunch of edits and clarifying comments. It
was quite the nightmare to rebase 5.3/scsi-queue to satisfy the ordering
requirements, locate the scattered fixes, tweak tags, etc. Hope I got
everything right.

It was almost at the point where I gave up and postponed everything to
5.4. We're at -rc5 -- it's not exactly a good time to rebase the entire
tree. However, I know the static mq allocations are causing headaches
for several distro vendors so I begrudgingly bit the bullet.

I would have liked to see more reviews from the official driver
maintainers but at the same time I didn't want to wait any longer giving
this some soak time in linux-next.

Thanks for your work on getting all the drivers fixed up! Just goes to
show how large the fallout can be from a relatively innocuous core
change.

Oh, and I held back the rtsx patch due to lack of reviews. But since
that driver is in staging I'm not too worried about it. Hope we can get
the fix for that reviewed and merged soon.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V5 00/16] use sg helper to operate scatterlist

[Bart Van Assche]

On 6/18/19 5:29 PM, Martin K. Petersen wrote:
> I applied this series with a bunch of edits and clarifying comments. It
> was quite the nightmare to rebase 5.3/scsi-queue to satisfy the ordering
> requirements, locate the scattered fixes, tweak tags, etc. Hope I got
> everything right.

Hi Martin,

Do you perhaps plan to push out these patches at a later time? It seems 
like that branch has not been updated recently:

$ git show --format=fuller mkp-scsi/5.3/scsi-queue | head -n7
commit f3e88ad00f58e9a05986be3028b2ed8654c601c9
Author:     Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
AuthorDate: Fri May 31 08:14:43 2019 -0400
Commit:     Martin K. Petersen <martin.petersen@oracle.com>
CommitDate: Fri Jun 7 10:17:06 2019 -0400

     scsi: mpt3sas: Update driver version to 29.100.00.00

Thanks,

Bart.

Re: [PATCH V5 00/16] use sg helper to operate scatterlist

[Martin K. Petersen]

Bart,

> Do you perhaps plan to push out these patches at a later time? It
> seems like that branch has not been updated recently:

I had a test failure on this end, that's why I didn't push. Appears to
be hardware-related, though. Still looking into it.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V5 00/16] use sg helper to operate scatterlist

[Ming Lei]

Hi Martin,

On Thu, Jun 20, 2019 at 3:57 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Bart,
>
> > Do you perhaps plan to push out these patches at a later time? It
> > seems like that branch has not been updated recently:
>
> I had a test failure on this end, that's why I didn't push. Appears to
> be hardware-related, though. Still looking into it.

Today I found the whole patchset disappears from 5.3/scsi-queue, seems
something is wrong?


Thanks,
Ming Lei

Re: [PATCH V5 00/16] use sg helper to operate scatterlist

[Martin K. Petersen]

Ming,

> Today I found the whole patchset disappears from 5.3/scsi-queue, seems
> something is wrong?

Your changes are in 5.3/scsi-sg. I put them in a separate branch to
avoid having to rebase the rest of the queue in case we find more
issues.

My for-next branch is based on 5.3/scsi-queue and 5.3/scsi-sg.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Steffen Maier]

Hi Ming,

On 6/18/19 3:37 AM, Ming Lei wrote:
> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Cc: Steffen Maier <maier@linux.ibm.com>
> Cc: Benjamin Block <bblock@linux.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Acked-by: Benjamin Block <bblock@linux.ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/s390/scsi/zfcp_fc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> index 33eddb02ee30..b018b61bd168 100644
> --- a/drivers/s390/scsi/zfcp_fc.c
> +++ b/drivers/s390/scsi/zfcp_fc.c
> @@ -620,7 +620,7 @@ static void zfcp_fc_sg_free_table(struct scatterlist *sg, int count)
>   {
>   	int i;
>   
> -	for (i = 0; i < count; i++, sg++)
> +	for (i = 0; i < count; i++, sg = sg_next(sg))
>   		if (sg)
>   			free_page((unsigned long) sg_virt(sg));
>   		else
> @@ -641,7 +641,7 @@ static int zfcp_fc_sg_setup_table(struct scatterlist *sg, int count)
>   	int i;
>   
>   	sg_init_table(sg, count);
> -	for (i = 0; i < count; i++, sg++) {
> +	for (i = 0; i < count; i++, sg = sg_next(sg)) {
>   		addr = (void *) get_zeroed_page(GFP_KERNEL);
>   		if (!addr) {
>   			zfcp_fc_sg_free_table(sg, i);
> 

I'm still catching up with emails that came during my vacation, so I'm not 
fully up-to-date on the current state of this and how to bring in potential 
fixups on top.

I think, we also have two more (not so obvious) places in the corresponding 
response/completion code path, where we might need to introduce the proper 
iterator helper:

zfcp_fsf.c:

static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
			       struct zfcp_adapter *adapter, int max_entries)
{
	struct scatterlist *sg = &fc_req->sg_rsp;
...
	/* first entry is the header */
	for (x = 1; x < max_entries && !last; x++) {
...
		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
...
		else
			acc = sg_virt(++sg);
                                       ^^^^

zfcp_dbf.c:

static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
					      struct zfcp_fsf_req *fsf,
					      u16 len)
{
	struct scatterlist *resp_entry = ct_els->resp;
...
	/* the basic CT_IU preamble is the same size as one entry in the GPN_FT
	 * response, allowing us to skip special handling for it - just skip it
	 */
	for (x = 1; x < max_entries && !last; x++) {
		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
...
		else
			acc = sg_virt(++resp_entry);
                                       ^^^^^^^^^^^^


What do you think?

-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Ming Lei]

On Mon, Jun 24, 2019 at 05:13:24PM +0200, Steffen Maier wrote:
> Hi Ming,
> 
> On 6/18/19 3:37 AM, Ming Lei wrote:
> > Use the scatterlist iterators and remove direct indexing of the
> > scatterlist array.
> > 
> > This way allows us to pre-allocate one small scatterlist, which can be
> > chained with one runtime allocated scatterlist if the pre-allocated one
> > isn't enough for the whole request.
> > 
> > Cc: Steffen Maier <maier@linux.ibm.com>
> > Cc: Benjamin Block <bblock@linux.ibm.com>
> > Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > Acked-by: Benjamin Block <bblock@linux.ibm.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   drivers/s390/scsi/zfcp_fc.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > index 33eddb02ee30..b018b61bd168 100644
> > --- a/drivers/s390/scsi/zfcp_fc.c
> > +++ b/drivers/s390/scsi/zfcp_fc.c
> > @@ -620,7 +620,7 @@ static void zfcp_fc_sg_free_table(struct scatterlist *sg, int count)
> >   {
> >   	int i;
> > -	for (i = 0; i < count; i++, sg++)
> > +	for (i = 0; i < count; i++, sg = sg_next(sg))
> >   		if (sg)
> >   			free_page((unsigned long) sg_virt(sg));
> >   		else
> > @@ -641,7 +641,7 @@ static int zfcp_fc_sg_setup_table(struct scatterlist *sg, int count)
> >   	int i;
> >   	sg_init_table(sg, count);
> > -	for (i = 0; i < count; i++, sg++) {
> > +	for (i = 0; i < count; i++, sg = sg_next(sg)) {
> >   		addr = (void *) get_zeroed_page(GFP_KERNEL);
> >   		if (!addr) {
> >   			zfcp_fc_sg_free_table(sg, i);
> > 
> 
> I'm still catching up with emails that came during my vacation, so I'm not
> fully up-to-date on the current state of this and how to bring in potential
> fixups on top.
> 
> I think, we also have two more (not so obvious) places in the corresponding
> response/completion code path, where we might need to introduce the proper
> iterator helper:
> 
> zfcp_fsf.c:
> 
> static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
> 			       struct zfcp_adapter *adapter, int max_entries)
> {
> 	struct scatterlist *sg = &fc_req->sg_rsp;
> ...
> 	/* first entry is the header */
> 	for (x = 1; x < max_entries && !last; x++) {
> ...
> 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> ...
> 		else
> 			acc = sg_virt(++sg);
>                                       ^^^^
> 
> zfcp_dbf.c:
> 
> static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
> 					      struct zfcp_fsf_req *fsf,
> 					      u16 len)
> {
> 	struct scatterlist *resp_entry = ct_els->resp;
> ...
> 	/* the basic CT_IU preamble is the same size as one entry in the GPN_FT
> 	 * response, allowing us to skip special handling for it - just skip it
> 	 */
> 	for (x = 1; x < max_entries && !last; x++) {
> 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> ...
> 		else
> 			acc = sg_virt(++resp_entry);
>                                       ^^^^^^^^^^^^
> 
> 
> What do you think?

Yeah, looks this one is missed, so we need the following patch:

From c9c368308fefbf034d670984fe9746a4181fe514 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Tue, 25 Jun 2019 09:15:34 +0800
Subject: [PATCH] s390: scsi: use sg helper to iterate over scatterlist

Unlike the legacy I/O path, scsi-mq preallocates a large array to hold
the scatterlist for each request. This static allocation can consume
substantial amounts of memory on modern controllers which support a
large number of concurrently outstanding requests.

To facilitate a switch to a smaller static allocation combined with a
dynamic allocation for requests that need it, we need to make sure all
SCSI drivers handle chained scatterlists correctly.

Convert remaining drivers that directly dereference the scatterlist
array to using the iterator functions.

Cc: Steffen Maier <maier@linux.ibm.com>
Cc: Benjamin Block <bblock@linux.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/s390/scsi/zfcp_dbf.c | 2 +-
 drivers/s390/scsi/zfcp_fc.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index dccdb41bed8c..c7129f5234f0 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
 			acc++;
 		else
-			acc = sg_virt(++resp_entry);
+			acc = sg_virt(resp_entry = sg_next(resp_entry));
 
 		last = acc->fp_flags & FC_NS_FID_LAST;
 	}
diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
index b018b61bd168..5048812ce660 100644
--- a/drivers/s390/scsi/zfcp_fc.c
+++ b/drivers/s390/scsi/zfcp_fc.c
@@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
 			acc++;
 		else
-			acc = sg_virt(++sg);
+			acc = sg_virt(sg = sg_next(sg));
 
 		last = acc->fp_flags & FC_NS_FID_LAST;
 		d_id = ntoh24(acc->fp_fid);
-- 
2.20.1


Thanks,
Ming

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Finn Thain]

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index dccdb41bed8c..c7129f5234f0 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
>  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>  			acc++;
>  		else
> -			acc = sg_virt(++resp_entry);
> +			acc = sg_virt(resp_entry = sg_next(resp_entry));

Shouldn't that be,

			acc = sg_virt(sg_next(resp_entry));

>  
>  		last = acc->fp_flags & FC_NS_FID_LAST;
>  	}
> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> index b018b61bd168..5048812ce660 100644
> --- a/drivers/s390/scsi/zfcp_fc.c
> +++ b/drivers/s390/scsi/zfcp_fc.c
> @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
>  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>  			acc++;
>  		else
> -			acc = sg_virt(++sg);
> +			acc = sg_virt(sg = sg_next(sg));

and

			acc = sg_virt(sg_next(sg));

?

-- 

>  
>  		last = acc->fp_flags & FC_NS_FID_LAST;
>  		d_id = ntoh24(acc->fp_fid);
> -- 
> 2.20.1
> 
> 
> Thanks,
> Ming
> 

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Ming Lei]

On Tue, Jun 25, 2019 at 12:01:24PM +1000, Finn Thain wrote:
> > diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> > index dccdb41bed8c..c7129f5234f0 100644
> > --- a/drivers/s390/scsi/zfcp_dbf.c
> > +++ b/drivers/s390/scsi/zfcp_dbf.c
> > @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
> >  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> >  			acc++;
> >  		else
> > -			acc = sg_virt(++resp_entry);
> > +			acc = sg_virt(resp_entry = sg_next(resp_entry));
> 
> Shouldn't that be,
> 
> 			acc = sg_virt(sg_next(resp_entry));

resp_entry needs to be updated.

> 
> >  
> >  		last = acc->fp_flags & FC_NS_FID_LAST;
> >  	}
> > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > index b018b61bd168..5048812ce660 100644
> > --- a/drivers/s390/scsi/zfcp_fc.c
> > +++ b/drivers/s390/scsi/zfcp_fc.c
> > @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
> >  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> >  			acc++;
> >  		else
> > -			acc = sg_virt(++sg);
> > +			acc = sg_virt(sg = sg_next(sg));
> 
> and
> 
> 			acc = sg_virt(sg_next(sg));
> 
> ?

Same with above.

Thanks,
Ming

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Finn Thain]

On Tue, 25 Jun 2019, Ming Lei wrote:

> On Tue, Jun 25, 2019 at 12:01:24PM +1000, Finn Thain wrote:
> > > diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> > > index dccdb41bed8c..c7129f5234f0 100644
> > > --- a/drivers/s390/scsi/zfcp_dbf.c
> > > +++ b/drivers/s390/scsi/zfcp_dbf.c
> > > @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
> > >  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > >  			acc++;
> > >  		else
> > > -			acc = sg_virt(++resp_entry);
> > > +			acc = sg_virt(resp_entry = sg_next(resp_entry));
> > 
> > Shouldn't that be,
> > 
> > 			acc = sg_virt(sg_next(resp_entry));
> 
> resp_entry needs to be updated.
> 

Right, sorry for the noise.

-- 

> > 
> > >  
> > >  		last = acc->fp_flags & FC_NS_FID_LAST;
> > >  	}
> > > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > > index b018b61bd168..5048812ce660 100644
> > > --- a/drivers/s390/scsi/zfcp_fc.c
> > > +++ b/drivers/s390/scsi/zfcp_fc.c
> > > @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
> > >  		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > >  			acc++;
> > >  		else
> > > -			acc = sg_virt(++sg);
> > > +			acc = sg_virt(sg = sg_next(sg));
> > 
> > and
> > 
> > 			acc = sg_virt(sg_next(sg));
> > 
> > ?
> 
> Same with above.
> 
> Thanks,
> Ming
> 

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Steffen Maier]

Hi Ming,

I don't mind doing this change for zfcp. However, I'm having doubts regarding 
the rationale in the commit description. If I understood your patch series 
correctly from its cover letter (would have been nice to copy the SCSI MQ 
detail part of its core statement in (one of) the patches so it would make its 
way into git log), you plan to make a change to scatterlist allocations *in 
SCSI MQ*.

All zfcp changes in this patch set only refer to zfcp-internal remote port 
discovery, i.e. they neither come from SCSI (internally) nor from block(mq).

zfcp_fc.h:
/**
  * struct zfcp_fc_req - Container for FC ELS and CT requests sent from zfcp
  * @ct_els: data required for issuing fsf command
  * @sg_req: scatterlist entry for request data, refers to embedded @u submember
  * @sg_rsp: scatterlist entry for response data, refers to embedded @u submember
  * @u: request and response specific data

  * @u.gpn_ft: GPN_FT specific data
  * @u.gpn_ft.sg_rsp2: GPN_FT response, not embedded here, allocated elsewhere
  * @u.gpn_ft.req: GPN_FT request

  */
struct zfcp_fc_req {
	struct zfcp_fsf_ct_els				ct_els;
	struct scatterlist				sg_req;
	struct scatterlist				sg_rsp;
	union {

		struct {
			struct scatterlist sg_rsp2[ZFCP_FC_GPN_FT_NUM_BUFS - 1];
			struct zfcp_fc_gpn_ft_req	req;
		} gpn_ft;

	} u;
};

So this should be guaranteed to be a linear unchained scatterlist, 
independently of your SCSI (or block) changes.
Note: Only remote port discovery also uses u.gpn_ft.sg_rsp2 instead of just sg_rsp.
  zfcp_fc_scan_ports(work)
   zfcp_fc_alloc_sg_env(buf_num)
    zfcp_fc_sg_setup_table(&fc_req->sg_rsp, buf_num)
(In fact it's somewhat intricate, because it actually uses sg_rsp and seems to 
rely on the fact that the subsequent sg_rsp2[] gives enough contiguous memory 
to hold buf_num linear scatterlist entries starting with field offset sg_rsp.)
The other cases use single element and thus linear unchained scatterlist with 
sg_rsp (and all cases use sg_req):
Finding symbol: sg_init_one
Database directory: /home/maier/docs/zfcp/tuxmaker/linux/drivers/s390/scsi/
-------------------------------------------------------------------------------
*** zfcp_dbf.c:
zfcp_dbf_san_in_els[601]       sg_init_one(&sg, srb->payload.data, length);
// above tracing part is unrelated to all of scsi/block/zfcp-internal-ct/els
*** zfcp_fc.c:
zfcp_fc_ns_gid_pn_request[388] sg_init_one(&fc_req->sg_req, gid_pn_req, 
sizeof(*gid_pn_req));
zfcp_fc_ns_gid_pn_request[389] sg_init_one(&fc_req->sg_rsp, gid_pn_rsp, 
sizeof(*gid_pn_rsp));
zfcp_fc_adisc[546]             sg_init_one(&fc_req->sg_req, &fc_req->u.adisc.req,
zfcp_fc_adisc[548]             sg_init_one(&fc_req->sg_rsp, &fc_req->u.adisc.rsp,
zfcp_fc_alloc_sg_env[668]      sg_init_one(&fc_req->sg_req, &fc_req->u.gpn_ft.req,
zfcp_fc_gspn[841]              sg_init_one(&fc_req->sg_req, gspn_req, 
sizeof(*gspn_req));
zfcp_fc_gspn[842]              sg_init_one(&fc_req->sg_rsp, gspn_rsp, 
sizeof(*gspn_rsp));
zfcp_fc_rspn[889]              sg_init_one(&fc_req->sg_req, rspn_req, 
sizeof(*rspn_req));
zfcp_fc_rspn[890]              sg_init_one(&fc_req->sg_rsp, rspn_rsp, 
sizeof(*rspn_rsp));
-------------------------------------------------------------------------------

I/O requests from SCSI (MQ) coming through queuecommand have already been safe 
for non-linear chained scatterlists in zfcp:

  zfcp_scsi_queuecommand()
   zfcp_fsf_fcp_cmnd()
    zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, scsi_sglist(scsi_cmnd))
	for (; sg; sg = sg_next(sg)) {

I/O requests from the block layer coming through BSG should also have already 
been safe for non-linear chained scatterlists in zfcp:

  zfcp_fc_exec_bsg_job()
   zfcp_fc_exec_els_job()
    zfcp_fsf_send_els()
     zfcp_fsf_setup_ct_els()
      zfcp_fsf_setup_ct_els_sbals(req, sg_req, sg_resp)
	//depending on hardware features, translate sg into HW control blocks
	if (zfcp_adapter_multi_buffer_active())
         	zfcp_qdio_sbals_from_sg() //for req&resp, see above
         	return 0
	/* use single, unchained SBAL if it can hold the request */
	if (zfcp_qdio_sg_one_sbale(sg_req) && zfcp_qdio_sg_one_sbale(sg_resp))
         	zfcp_fsf_setup_ct_els_unchained() //single element for req&resp
         	return 0
	if (!(feat & FSF_FEATURE_ELS_CT_CHAINED_SBALS))
		return -EOPNOTSUPP;
        zfcp_qdio_sbals_from_sg() for req&resp, see above
   OR
   zfcp_fc_exec_ct_job()
    zfcp_fsf_send_ct()
     zfcp_fsf_setup_ct_els() //see above

If I was not mistaken above, the following could be more descriptive parts of a 
patch/commit description, with hopefully less confusion for anyone having to 
look at zfcp git history a few weeks/months/years from now:

"While not required for this SCSI MQ change regarding scatterlist allocation, 
change all other scatterlist iterators in zfcp to the safe sg_next() even if 
not necessary as these changed zfcp-internal scatterlists are linear and 
unchained. This may avoid confusion about a potential need for conversions in 
the future."


On 6/25/19 3:19 AM, Ming Lei wrote:
> On Mon, Jun 24, 2019 at 05:13:24PM +0200, Steffen Maier wrote:
>> On 6/18/19 3:37 AM, Ming Lei wrote:
>>> Use the scatterlist iterators and remove direct indexing of the
>>> scatterlist array.
>>>
>>> This way allows us to pre-allocate one small scatterlist, which can be
>>> chained with one runtime allocated scatterlist if the pre-allocated one
>>> isn't enough for the whole request.

>>> Acked-by: Benjamin Block <bblock@linux.ibm.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>    drivers/s390/scsi/zfcp_fc.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
>>> index 33eddb02ee30..b018b61bd168 100644
>>> --- a/drivers/s390/scsi/zfcp_fc.c
>>> +++ b/drivers/s390/scsi/zfcp_fc.c
>>> @@ -620,7 +620,7 @@ static void zfcp_fc_sg_free_table(struct scatterlist *sg, int count)
>>>    {
>>>    	int i;
>>> -	for (i = 0; i < count; i++, sg++)
>>> +	for (i = 0; i < count; i++, sg = sg_next(sg))
>>>    		if (sg)
>>>    			free_page((unsigned long) sg_virt(sg));
>>>    		else
>>> @@ -641,7 +641,7 @@ static int zfcp_fc_sg_setup_table(struct scatterlist *sg, int count)
>>>    	int i;
>>>    	sg_init_table(sg, count);
>>> -	for (i = 0; i < count; i++, sg++) {
>>> +	for (i = 0; i < count; i++, sg = sg_next(sg)) {
>>>    		addr = (void *) get_zeroed_page(GFP_KERNEL);
>>>    		if (!addr) {
>>>    			zfcp_fc_sg_free_table(sg, i);
>>>
>>
>> I'm still catching up with emails that came during my vacation, so I'm not
>> fully up-to-date on the current state of this and how to bring in potential
>> fixups on top.
>>
>> I think, we also have two more (not so obvious) places in the corresponding
>> response/completion code path, where we might need to introduce the proper
>> iterator helper:
>>
>> zfcp_fsf.c:
>>
>> static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
>> 			       struct zfcp_adapter *adapter, int max_entries)
>> {
>> 	struct scatterlist *sg = &fc_req->sg_rsp;
>> ...
>> 	/* first entry is the header */
>> 	for (x = 1; x < max_entries && !last; x++) {
>> ...
>> 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>> ...
>> 		else
>> 			acc = sg_virt(++sg);
>>                                        ^^^^
>>
>> zfcp_dbf.c:
>>
>> static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
>> 					      struct zfcp_fsf_req *fsf,
>> 					      u16 len)
>> {
>> 	struct scatterlist *resp_entry = ct_els->resp;
>> ...
>> 	/* the basic CT_IU preamble is the same size as one entry in the GPN_FT
>> 	 * response, allowing us to skip special handling for it - just skip it
>> 	 */
>> 	for (x = 1; x < max_entries && !last; x++) {
>> 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>> ...
>> 		else
>> 			acc = sg_virt(++resp_entry);
>>                                        ^^^^^^^^^^^^
>>
>>
>> What do you think?
> 
> Yeah, looks this one is missed, so we need the following patch:
> 
>  From c9c368308fefbf034d670984fe9746a4181fe514 Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Tue, 25 Jun 2019 09:15:34 +0800
> Subject: [PATCH] s390: scsi: use sg helper to iterate over scatterlist
> 
> Unlike the legacy I/O path, scsi-mq preallocates a large array to hold
> the scatterlist for each request. This static allocation can consume
> substantial amounts of memory on modern controllers which support a
> large number of concurrently outstanding requests.

Very nice, as it disambiguates which scatterlist allocation this patch set is 
about.

> To facilitate a switch to a smaller static allocation combined with a
> dynamic allocation for requests that need it, we need to make sure all
> SCSI drivers handle chained scatterlists correctly.
> 
> Convert remaining drivers that directly dereference the scatterlist
> array to using the iterator functions.
> 
> Cc: Steffen Maier <maier@linux.ibm.com>

> Cc: Benjamin Block <bblock@linux.ibm.com>

> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: linux-s390@vger.kernel.org

> Cc: Benjamin Block <bblock@linux.ibm.com>

Minor: duplicate Cc ?

> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/s390/scsi/zfcp_dbf.c | 2 +-
>   drivers/s390/scsi/zfcp_fc.c  | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index dccdb41bed8c..c7129f5234f0 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
>   		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>   			acc++;
>   		else
> -			acc = sg_virt(++resp_entry);
> +			acc = sg_virt(resp_entry = sg_next(resp_entry));
>   
>   		last = acc->fp_flags & FC_NS_FID_LAST;
>   	}
> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> index b018b61bd168..5048812ce660 100644
> --- a/drivers/s390/scsi/zfcp_fc.c
> +++ b/drivers/s390/scsi/zfcp_fc.c
> @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
>   		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>   			acc++;
>   		else
> -			acc = sg_virt(++sg);
> +			acc = sg_virt(sg = sg_next(sg));
>   
>   		last = acc->fp_flags & FC_NS_FID_LAST;
>   		d_id = ntoh24(acc->fp_fid);
> 

Apart from above rationale discussion, the code change looks good.

-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Ming Lei]

Hi Steffen,

On Tue, Jun 25, 2019 at 12:51:04PM +0200, Steffen Maier wrote:
> Hi Ming,
> 
> I don't mind doing this change for zfcp. However, I'm having doubts
> regarding the rationale in the commit description. If I understood your
> patch series correctly from its cover letter (would have been nice to copy
> the SCSI MQ detail part of its core statement in (one of) the patches so it
> would make its way into git log), you plan to make a change to scatterlist
> allocations *in SCSI MQ*.
> 
> All zfcp changes in this patch set only refer to zfcp-internal remote port
> discovery, i.e. they neither come from SCSI (internally) nor from block(mq).

If you are sure about that, it is fine to not change zfcp.

However, I still suggest to do it because it will make us to audit SCSI chained
sg uses much easier. And the change shouldn't have performance effect.

> 
> zfcp_fc.h:
> /**
>  * struct zfcp_fc_req - Container for FC ELS and CT requests sent from zfcp
>  * @ct_els: data required for issuing fsf command
>  * @sg_req: scatterlist entry for request data, refers to embedded @u submember
>  * @sg_rsp: scatterlist entry for response data, refers to embedded @u submember
>  * @u: request and response specific data
> 
>  * @u.gpn_ft: GPN_FT specific data
>  * @u.gpn_ft.sg_rsp2: GPN_FT response, not embedded here, allocated elsewhere
>  * @u.gpn_ft.req: GPN_FT request
> 
>  */
> struct zfcp_fc_req {
> 	struct zfcp_fsf_ct_els				ct_els;
> 	struct scatterlist				sg_req;
> 	struct scatterlist				sg_rsp;
> 	union {
> 
> 		struct {
> 			struct scatterlist sg_rsp2[ZFCP_FC_GPN_FT_NUM_BUFS - 1];
> 			struct zfcp_fc_gpn_ft_req	req;
> 		} gpn_ft;
> 
> 	} u;
> };
> 
> So this should be guaranteed to be a linear unchained scatterlist,
> independently of your SCSI (or block) changes.
> Note: Only remote port discovery also uses u.gpn_ft.sg_rsp2 instead of just sg_rsp.
>  zfcp_fc_scan_ports(work)
>   zfcp_fc_alloc_sg_env(buf_num)
>    zfcp_fc_sg_setup_table(&fc_req->sg_rsp, buf_num)
> (In fact it's somewhat intricate, because it actually uses sg_rsp and seems
> to rely on the fact that the subsequent sg_rsp2[] gives enough contiguous
> memory to hold buf_num linear scatterlist entries starting with field offset
> sg_rsp.)
> The other cases use single element and thus linear unchained scatterlist
> with sg_rsp (and all cases use sg_req):
> Finding symbol: sg_init_one
> Database directory: /home/maier/docs/zfcp/tuxmaker/linux/drivers/s390/scsi/
> -------------------------------------------------------------------------------
> *** zfcp_dbf.c:
> zfcp_dbf_san_in_els[601]       sg_init_one(&sg, srb->payload.data, length);
> // above tracing part is unrelated to all of scsi/block/zfcp-internal-ct/els
> *** zfcp_fc.c:
> zfcp_fc_ns_gid_pn_request[388] sg_init_one(&fc_req->sg_req, gid_pn_req,
> sizeof(*gid_pn_req));
> zfcp_fc_ns_gid_pn_request[389] sg_init_one(&fc_req->sg_rsp, gid_pn_rsp,
> sizeof(*gid_pn_rsp));
> zfcp_fc_adisc[546]             sg_init_one(&fc_req->sg_req, &fc_req->u.adisc.req,
> zfcp_fc_adisc[548]             sg_init_one(&fc_req->sg_rsp, &fc_req->u.adisc.rsp,
> zfcp_fc_alloc_sg_env[668]      sg_init_one(&fc_req->sg_req, &fc_req->u.gpn_ft.req,
> zfcp_fc_gspn[841]              sg_init_one(&fc_req->sg_req, gspn_req,
> sizeof(*gspn_req));
> zfcp_fc_gspn[842]              sg_init_one(&fc_req->sg_rsp, gspn_rsp,
> sizeof(*gspn_rsp));
> zfcp_fc_rspn[889]              sg_init_one(&fc_req->sg_req, rspn_req,
> sizeof(*rspn_req));
> zfcp_fc_rspn[890]              sg_init_one(&fc_req->sg_rsp, rspn_rsp,
> sizeof(*rspn_rsp));
> -------------------------------------------------------------------------------
> 
> I/O requests from SCSI (MQ) coming through queuecommand have already been
> safe for non-linear chained scatterlists in zfcp:
> 
>  zfcp_scsi_queuecommand()
>   zfcp_fsf_fcp_cmnd()
>    zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, scsi_sglist(scsi_cmnd))
> 	for (; sg; sg = sg_next(sg)) {
> 
> I/O requests from the block layer coming through BSG should also have
> already been safe for non-linear chained scatterlists in zfcp:
> 
>  zfcp_fc_exec_bsg_job()
>   zfcp_fc_exec_els_job()
>    zfcp_fsf_send_els()
>     zfcp_fsf_setup_ct_els()
>      zfcp_fsf_setup_ct_els_sbals(req, sg_req, sg_resp)
> 	//depending on hardware features, translate sg into HW control blocks
> 	if (zfcp_adapter_multi_buffer_active())
>         	zfcp_qdio_sbals_from_sg() //for req&resp, see above
>         	return 0
> 	/* use single, unchained SBAL if it can hold the request */
> 	if (zfcp_qdio_sg_one_sbale(sg_req) && zfcp_qdio_sg_one_sbale(sg_resp))
>         	zfcp_fsf_setup_ct_els_unchained() //single element for req&resp
>         	return 0
> 	if (!(feat & FSF_FEATURE_ELS_CT_CHAINED_SBALS))
> 		return -EOPNOTSUPP;
>        zfcp_qdio_sbals_from_sg() for req&resp, see above
>   OR
>   zfcp_fc_exec_ct_job()
>    zfcp_fsf_send_ct()
>     zfcp_fsf_setup_ct_els() //see above
> 
> If I was not mistaken above, the following could be more descriptive parts
> of a patch/commit description, with hopefully less confusion for anyone
> having to look at zfcp git history a few weeks/months/years from now:
> 
> "While not required for this SCSI MQ change regarding scatterlist
> allocation, change all other scatterlist iterators in zfcp to the safe
> sg_next() even if not necessary as these changed zfcp-internal scatterlists
> are linear and unchained. This may avoid confusion about a potential need
> for conversions in the future."
> 
> 
> On 6/25/19 3:19 AM, Ming Lei wrote:
> > On Mon, Jun 24, 2019 at 05:13:24PM +0200, Steffen Maier wrote:
> > > On 6/18/19 3:37 AM, Ming Lei wrote:
> > > > Use the scatterlist iterators and remove direct indexing of the
> > > > scatterlist array.
> > > > 
> > > > This way allows us to pre-allocate one small scatterlist, which can be
> > > > chained with one runtime allocated scatterlist if the pre-allocated one
> > > > isn't enough for the whole request.
> 
> > > > Acked-by: Benjamin Block <bblock@linux.ibm.com>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >    drivers/s390/scsi/zfcp_fc.c | 4 ++--
> > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > > > index 33eddb02ee30..b018b61bd168 100644
> > > > --- a/drivers/s390/scsi/zfcp_fc.c
> > > > +++ b/drivers/s390/scsi/zfcp_fc.c
> > > > @@ -620,7 +620,7 @@ static void zfcp_fc_sg_free_table(struct scatterlist *sg, int count)
> > > >    {
> > > >    	int i;
> > > > -	for (i = 0; i < count; i++, sg++)
> > > > +	for (i = 0; i < count; i++, sg = sg_next(sg))
> > > >    		if (sg)
> > > >    			free_page((unsigned long) sg_virt(sg));
> > > >    		else
> > > > @@ -641,7 +641,7 @@ static int zfcp_fc_sg_setup_table(struct scatterlist *sg, int count)
> > > >    	int i;
> > > >    	sg_init_table(sg, count);
> > > > -	for (i = 0; i < count; i++, sg++) {
> > > > +	for (i = 0; i < count; i++, sg = sg_next(sg)) {
> > > >    		addr = (void *) get_zeroed_page(GFP_KERNEL);
> > > >    		if (!addr) {
> > > >    			zfcp_fc_sg_free_table(sg, i);
> > > > 
> > > 
> > > I'm still catching up with emails that came during my vacation, so I'm not
> > > fully up-to-date on the current state of this and how to bring in potential
> > > fixups on top.
> > > 
> > > I think, we also have two more (not so obvious) places in the corresponding
> > > response/completion code path, where we might need to introduce the proper
> > > iterator helper:
> > > 
> > > zfcp_fsf.c:
> > > 
> > > static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
> > > 			       struct zfcp_adapter *adapter, int max_entries)
> > > {
> > > 	struct scatterlist *sg = &fc_req->sg_rsp;
> > > ...
> > > 	/* first entry is the header */
> > > 	for (x = 1; x < max_entries && !last; x++) {
> > > ...
> > > 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > > ...
> > > 		else
> > > 			acc = sg_virt(++sg);
> > >                                        ^^^^
> > > 
> > > zfcp_dbf.c:
> > > 
> > > static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
> > > 					      struct zfcp_fsf_req *fsf,
> > > 					      u16 len)
> > > {
> > > 	struct scatterlist *resp_entry = ct_els->resp;
> > > ...
> > > 	/* the basic CT_IU preamble is the same size as one entry in the GPN_FT
> > > 	 * response, allowing us to skip special handling for it - just skip it
> > > 	 */
> > > 	for (x = 1; x < max_entries && !last; x++) {
> > > 		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > > ...
> > > 		else
> > > 			acc = sg_virt(++resp_entry);
> > >                                        ^^^^^^^^^^^^
> > > 
> > > 
> > > What do you think?
> > 
> > Yeah, looks this one is missed, so we need the following patch:
> > 
> >  From c9c368308fefbf034d670984fe9746a4181fe514 Mon Sep 17 00:00:00 2001
> > From: Ming Lei <ming.lei@redhat.com>
> > Date: Tue, 25 Jun 2019 09:15:34 +0800
> > Subject: [PATCH] s390: scsi: use sg helper to iterate over scatterlist
> > 
> > Unlike the legacy I/O path, scsi-mq preallocates a large array to hold
> > the scatterlist for each request. This static allocation can consume
> > substantial amounts of memory on modern controllers which support a
> > large number of concurrently outstanding requests.
> 
> Very nice, as it disambiguates which scatterlist allocation this patch set
> is about.
> 
> > To facilitate a switch to a smaller static allocation combined with a
> > dynamic allocation for requests that need it, we need to make sure all
> > SCSI drivers handle chained scatterlists correctly.
> > 
> > Convert remaining drivers that directly dereference the scatterlist
> > array to using the iterator functions.
> > 
> > Cc: Steffen Maier <maier@linux.ibm.com>
> 
> > Cc: Benjamin Block <bblock@linux.ibm.com>
> 
> > Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> 
> > Cc: Benjamin Block <bblock@linux.ibm.com>
> 
> Minor: duplicate Cc ?
> 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   drivers/s390/scsi/zfcp_dbf.c | 2 +-
> >   drivers/s390/scsi/zfcp_fc.c  | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> > index dccdb41bed8c..c7129f5234f0 100644
> > --- a/drivers/s390/scsi/zfcp_dbf.c
> > +++ b/drivers/s390/scsi/zfcp_dbf.c
> > @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
> >   		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> >   			acc++;
> >   		else
> > -			acc = sg_virt(++resp_entry);
> > +			acc = sg_virt(resp_entry = sg_next(resp_entry));
> >   		last = acc->fp_flags & FC_NS_FID_LAST;
> >   	}
> > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > index b018b61bd168..5048812ce660 100644
> > --- a/drivers/s390/scsi/zfcp_fc.c
> > +++ b/drivers/s390/scsi/zfcp_fc.c
> > @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
> >   		if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> >   			acc++;
> >   		else
> > -			acc = sg_virt(++sg);
> > +			acc = sg_virt(sg = sg_next(sg));
> >   		last = acc->fp_flags & FC_NS_FID_LAST;
> >   		d_id = ntoh24(acc->fp_fid);
> > 
> 
> Apart from above rationale discussion, the code change looks good.

OK, I still suggest to apply the patch for the mentioned reason if you
are fine.

Thanks,
Ming

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Steffen Maier]

Hi Ming,

On 6/26/19 5:07 AM, Ming Lei wrote:
> On Tue, Jun 25, 2019 at 12:51:04PM +0200, Steffen Maier wrote:
>> I don't mind doing this change for zfcp. However, I'm having doubts
>> regarding the rationale in the commit description.

> However, I still suggest to do it because it will make us to audit SCSI chained
> sg uses much easier. And the change shouldn't have performance effect.

>> If I was not mistaken above, the following could be more descriptive parts
>> of a patch/commit description, with hopefully less confusion for anyone
>> having to look at zfcp git history a few weeks/months/years from now:
>>
>> "While not required for this SCSI MQ change regarding scatterlist
>> allocation, change all other scatterlist iterators in zfcp to the safe
>> sg_next() even if not necessary as these changed zfcp-internal scatterlists
>> are linear and unchained. This may avoid confusion about a potential need
>> for conversions in the future."

Sure, let's change the code, but could you please update the description of 
your below new patch to something like I drafted above regarding rationale?

If you would like to send a V2, I'll be happy to give a Reviewed-by.

>>>   From c9c368308fefbf034d670984fe9746a4181fe514 Mon Sep 17 00:00:00 2001
>>> From: Ming Lei <ming.lei@redhat.com>
>>> Date: Tue, 25 Jun 2019 09:15:34 +0800
>>> Subject: [PATCH] s390: scsi: use sg helper to iterate over scatterlist

>>> Unlike the legacy I/O path, scsi-mq preallocates a large array to hold
>>> the scatterlist for each request. This static allocation can consume
>>> substantial amounts of memory on modern controllers which support a
>>> large number of concurrently outstanding requests.

>>> To facilitate a switch to a smaller static allocation combined with a
>>> dynamic allocation for requests that need it, we need to make sure all
>>> SCSI drivers handle chained scatterlists correctly.

>>> Convert remaining drivers that directly dereference the scatterlist
>>> array to using the iterator functions.

> OK, I still suggest to apply the patch for the mentioned reason if you
> are fine.



-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294