Re: [PATCH V3 10/15] scsi: aha152x: use sg helper to operate scatterlist

[Ming Lei <ming.lei@redhat.com>]

On Fri, Jun 14, 2019 at 03:27:36PM +1000, Finn Thain wrote:
> Hi Ming,
> 
> On Fri, 14 Jun 2019, 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.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/scsi/aha152x.c | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> > index 97872838b983..bc9d12aa7880 100644
> > --- a/drivers/scsi/aha152x.c
> > +++ b/drivers/scsi/aha152x.c
> > @@ -2033,7 +2033,7 @@ static void datai_run(struct Scsi_Host *shpnt)
> >  				    CURRENT_SC->SCp.buffers_residual > 0) {
> >  					/* 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;
> >  				}
> > @@ -2139,7 +2139,7 @@ static void datao_run(struct Scsi_Host *shpnt)
> >  		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {
> >  			/* 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;
> >  		}
> > @@ -2160,20 +2160,29 @@ static void datao_end(struct Scsi_Host *shpnt)
> >  	if(TESTLO(DMASTAT, DFIFOEMP)) {
> >  		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
> >  			GETSTCNT();
> 
> data_count appears to be the number of bytes remaining in the FIFO. (I 
> have to infer that much from the surrounding code. I don't have 
> documentation for this controller.)

Yeah, it should be so, that is the data which need to transfer again.

> 
> Some comments would be nice.
> 
> > +		struct scatterlist *sg = scsi_sglist(CURRENT_SC);
> > +		int left, i = 0;
> >  
> >  		CMD_INC_RESID(CURRENT_SC, data_count);
> >  
> 
> Apparently the aim is to increase the residual by the number of bytes that 
> will never leave the FIFO. Above we can see that increase performed by 
> scsi_set_resid() and now the same has to be done to the SCp struct.

Agree.

> 
> >  		data_count -= CURRENT_SC->SCp.ptr -
> >  			SG_ADDRESS(CURRENT_SC->SCp.buffer);
> 
> Here, data_count effectively has SCp.this_residual subtracted from it.
> 
> > -		while(data_count>0) {
> > -			CURRENT_SC->SCp.buffer--;
> > -			CURRENT_SC->SCp.buffers_residual++;
> > -			data_count -= CURRENT_SC->SCp.buffer->length;
> > -		}
> 
> So far, so good.
> 
> > -		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
> > -			data_count;
> > -		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
> > -			data_count;
> 
> This is like saying ptr = buffer + residual, which is bogus. This must be 
> an old bug, but we never hit it because the FIFO is always empty when we 
> get a DISCONNECT message. Probably because every SG segment has a length 
> that is a multiple of 128 bytes. (Juergen?)
> 
> > +
> > +		left = CURRENT_SC->transfersize - data_count;
> 
> Are you sure about that? Perhaps you meant to write,
> 		left = scsi_bufflen(CURRENT_SC) - scsi_get_resid(CURRENT_SC);
> 
> Is there a better name for this variable? Maybe 'sent' or 'bytes_sent'?

'left' can be renamed as 'done', and

	done =  GETSTCNT - (CURRENT_SC->SCp.ptr - SG_ADDRESS(CURRENT_SC->SCp.buffer))

> 
> > +		for (i = 0; left > 0 && !sg_is_last(sg); i++, sg = sg_next(sg)) {
> > +			if (left < sg->length)
> > +				break;
> > +			left -= sg->length;
> > +		}
> > +
> > +		if (data_count > 0) {
> > +			CURRENT_SC->SCp.buffers_residual += i;
> 
> Shouldn't that be,
> 			CURRENT_SC->SCp.buffers_residual = i;

Good catch.

> 
> > +			CURRENT_SC->SCp.buffer = sg;
> > +
> > +			CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) + left;
> > +			CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length -
> > +				left;
> > +		}
> >  	}
> >  
> >  	SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
> > 
> 
> BTW, datao_run() seems to guarantee that the FIFO will never contain more 
> than min(128, SCp.this_residual) so I suspect that this code can be 
> simplified. That's just BTW. I wouldn't attempt to optimize this branch as 
> it will only run when the FIFO is not empty, if ever. So I'd prefer 
> clarity. Besides, I don't have the hardware to test it on.

I'd suggest to simply translate into code which uses current sg helper.

Follows the fixed version, could you review again?

From f03484d4bac083c39d70665cfbadb641093b63de Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Wed, 12 Jun 2019 20:37:35 +0800
Subject: [PATCH] scsi: aha152x: use sg helper to operate scatterlist

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.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/aha152x.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 97872838b983..7faecdefda56 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -2033,7 +2033,7 @@ static void datai_run(struct Scsi_Host *shpnt)
 				    CURRENT_SC->SCp.buffers_residual > 0) {
 					/* 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;
 				}
@@ -2139,7 +2139,7 @@ static void datao_run(struct Scsi_Host *shpnt)
 		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {
 			/* 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 +2158,28 @@ 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();
+		int done = GETSTCNT();
+		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) - done;
+		struct scatterlist *sg = scsi_sglist(CURRENT_SC);
+		int i;
 
 		CMD_INC_RESID(CURRENT_SC, data_count);
 
-		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;
+		/*
+		 * rewind where we have done, and we have to start from
+		 * the beginning
+		 */
+		for (i = 0; done > 0 && !sg_is_last(sg); i++, sg = sg_next(sg)) {
+			if (done < sg->length)
+				break;
+			done -= sg->length;
 		}
-		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.buffers_residual = i;
+		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);
-- 
2.20.1


thanks,
Ming