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@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > 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
