[firefly] staging: usbip: bugfix for isochronous packets and optimization

Greg Kroah-Hartman gregkh at linuxfoundation.org
Tue Aug 26 21:57:35 EEST 2014


On Tue, Aug 26, 2014 at 09:06:17AM -0700, Valentina Manea wrote:
> On Tue, Aug 26, 2014 at 6:12 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> > This was promoted out of staging and there wasn't an email to the
> > staging list.  Normally I like to go through any remaining questions
> > before the code is moved. The code looks pretty good, I just had one
> > question.
> >
> 
> I didn't know I should send to that list as well; will keep in mind
> for next time.

Yeah, my fault as well, sorry, I just posted the request on the
linux-usb mailing list.

> >         drivers/usb/usbip/usbip_common.c:712 usbip_pad_iso()
> >         warn: why is zero skipped 'i'
> >
> > drivers/usb/usbip/usbip_common.c
> >    687  void usbip_pad_iso(struct usbip_device *ud, struct urb *urb)
> >    688  {
> >    689          int np = urb->number_of_packets;
> >    690          int i;
> >    691          int actualoffset = urb->actual_length;
> >    692
> >    693          if (!usb_pipeisoc(urb->pipe))
> >    694                  return;
> >    695
> >    696          /* if no packets or length of data is 0, then nothing to unpack */
> >    697          if (np == 0 || urb->actual_length == 0)
> >    698                  return;
> >    699
> >    700          /*
> >    701           * if actual_length is transfer_buffer_length then no padding is
> >    702           * present.
> >    703           */
> >    704          if (urb->actual_length == urb->transfer_buffer_length)
> >    705                  return;
> >    706
> >    707          /*
> >    708           * loop over all packets from last to first (to prevent overwritting
> >    709           * memory when padding) and move them into the proper place
> >    710           */
> >    711          for (i = np-1; i > 0; i--) {
> >
> > I don't understand this loop.  If we really intended to skip the first
> > packet then why isn't the "if (np == 0" condition "if (np <= 1"?  That
> > would be more clear and the comment would say "if 1 packet then nothing
> > to unpack."
> >
> >    712                  actualoffset -= urb->iso_frame_desc[i].actual_length;
> >    713                  memmove(urb->transfer_buffer + urb->iso_frame_desc[i].offset,
> >    714                          urb->transfer_buffer + actualoffset,
> >    715                          urb->iso_frame_desc[i].actual_length);
> >    716          }
> >    717  }
> >
> 
> urb->actual_length is the sum of individual packets' lengths,
> urb->iso_frame_desc[i].actual_length. Since actualoffset in the loop
> keeps decreasing, it will reach 0 when the first packet is processed.
> The first packet's offset is 0 as well and this just does memmove to
> the same buffer location.
> The (np == 0) condition is *not* the same as (np <= 1) because we want
> to return only is there are any packets.
> In my opinion, the code is correct as it is. Do you think I should add
> an extra comment about the loop?
> 
> > Also there is a smatch warning:
> > drivers/usb/usbip/stub_tx.c:186 stub_send_ret_submit() warn: returning -1 instead of -ENOMEM is sloppy
> >
> 
> Noted, will fix after the patch series is upstream.

You can send the patches now, no need to wait for me as your patches are
already in my tree.

thanks,

greg k-h


More information about the firefly mailing list