Hi,
Thanks for your comments.
2010/9/29 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> 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.
OK.
>
>> + 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.
OK.
>
>> + 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.
Seems "break" can be used inside "do{...}while(...)", right?
thanks,
--
Lei Ming
--
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
