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@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html