Re: [PATCH V3 07/15] usb: image: microtek: use sg helper to operate scatterlist

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

On Fri, Jun 14, 2019 at 03:39:10PM +1000, Finn Thain wrote:
> 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.
> > 
> > Cc: Oliver Neukum <oliver@neukum.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-usb@vger.kernel.org
> > 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;
> >  	}
> >  
> 
> Would it not be better to initialize desc->context.curr_sg in both 
> branches of this conditional?

I think either way is fine given desc->context.curr_sg is used only
if 'context->data' isn't NULL, see mts_command_done().

So I'd keep the patch as it is.

Thanks,
Ming