| Message ID | 20190614025316.7360-11-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | use sg helper to operate scatterlist | expand |
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.) 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. > 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'? > + 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; > + 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. --
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);
On Fri, 14 Jun 2019, Ming Lei wrote: > > 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; I think that's better than my suggestion. > + 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 > + */ How about, "Locate the first SG entry not yet sent". We could use sg_nents_for_len() but it returns a count of sg entries not a scatterlist pointer so it's not very helpful here. > + 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; Contradicting my previous email, that's still not right. I think it would have to be, CURRENT_SC->SCp.buffers_residual = scsi_sg_count(CURRENT_SC) - i; But we could remove all references to SCp.buffers_residual, like I did in patch 15/15 for NCR5380.c. > + 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); > What do you think of the revised patch below? diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c index 97872838b983..6d0518f616cb 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,24 @@ 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); 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; + /* 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 +2499,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|"); --
On Fri, Jun 14, 2019 at 08:36:38PM +1000, Finn Thain wrote: > On Fri, 14 Jun 2019, Ming Lei wrote: > > > > > 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; > > I think that's better than my suggestion. > > > + 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 > > + */ > > How about, "Locate the first SG entry not yet sent". OK. > > We could use sg_nents_for_len() but it returns a count of sg entries not a > scatterlist pointer so it's not very helpful here. > > > + 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; > > Contradicting my previous email, that's still not right. I think it would > have to be, > > CURRENT_SC->SCp.buffers_residual = scsi_sg_count(CURRENT_SC) - i; Right, my fault. > > But we could remove all references to SCp.buffers_residual, like I did in > patch 15/15 for NCR5380.c. > > > + 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); > > > > What do you think of the revised patch below? Looks fine, I will include it in V4. Thanks, Ming
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(); + struct scatterlist *sg = scsi_sglist(CURRENT_SC); + int left, i = 0; 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; + + left = CURRENT_SC->transfersize - data_count; + 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; + 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; } - CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) - - data_count; - CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length + - data_count; } SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
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(-)