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.
> 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.
Thanks,
Valentina
--
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
