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.
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 }
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
regards,
dan carpenter
--
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
