On Wed, 29 Sep 2010 tom.leiming@xxxxxxxxx wrote:
> From: Ming Lei <tom.leiming@xxxxxxxxx>
>
> This patch adds native scatter-gather support to uhci-hcd.
This isn't a big deal, because UHCI only runs at full speed. But it
doesn't hurt to have it.
> @@ -945,8 +962,8 @@ static int uhci_submit_common(struct uhci_hcd *uhci, struct urb *urb,
> do { /* Allow zero length packets */
> int pktsze = maxsze;
>
> - if (len <= pktsze) { /* The last packet */
> - pktsze = len;
> + if (this_sg_len <= pktsze) { /* The last packet */
I would prefer to keep the original "if" statement. It's true that the
two tests should be equivalent, but the first test more obviously
agrees with the comment.
> + pktsze = this_sg_len;
> if (!(urb->transfer_flags & URB_SHORT_NOT_OK))
> status &= ~TD_CTRL_SPD;
> }
> @@ -965,9 +982,17 @@ static int uhci_submit_common(struct uhci_hcd *uhci, struct urb *urb,
> plink = &td->link;
> status |= TD_CTRL_ACTIVE;
>
> + toggle ^= 1;
> data += pktsze;
> + this_sg_len -= pktsze;
> len -= maxsze;
> - toggle ^= 1;
> + if (likely(this_sg_len <= 0)) {
Please remove the "likely()". In fact this condition is _unlikely_,
because this_sg_len should never start out smaller than 512 whereas
pktsze will never be larger than 64.
> + if (--i <= 0 || len <= 0)
> + break;
> + sg = sg_next(sg);
> + data = sg_dma_address(sg);
> + this_sg_len = min_t(int, sg_dma_len(sg), len);
> + }
> } while (len > 0);
With the new "break" statement above, this doesn't need to be a "do ...
while" loop. Change it to a "for (;;) {" loop.
Alan Stern
--
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
