> From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, January 13, 2011 3:13 PM > > Paul, I think I see the bug you're trying to fix. Can you confirm my > analysis is correct? > > This is the original code in xhci-ring.c: > > static unsigned int count_sg_trbs_needed(struct xhci_hcd *xhci, struct urb *urb) > { > int num_sgs, num_trbs, running_total, temp, i; > struct scatterlist *sg; > > sg = NULL; > num_sgs = urb->num_sgs; > temp = urb->transfer_buffer_length; > > xhci_dbg(xhci, "count sg list trbs: \n"); > num_trbs = 0; > for_each_sg(urb->sg, sg, num_sgs, i) { > unsigned int previous_total_trbs = num_trbs; > unsigned int len = sg_dma_len(sg); > > /* Scatter gather list entries may cross 64KB boundaries */ > running_total = TRB_MAX_BUFF_SIZE - > (sg_dma_address(sg) & ((1 << TRB_MAX_BUFF_SHIFT) - 1)); > if (running_total != 0) > num_trbs++; > > /* How many more 64KB chunks to transfer, how many more TRBs? */ > while (running_total < sg_dma_len(sg)) { > num_trbs++; > running_total += TRB_MAX_BUFF_SIZE; > } > ... > len = min_t(int, len, temp); > temp -= len; > if (temp == 0) > break; > } > ... > return num_trbs; > } > > I see that you've changed the while loop condition to this: > > + while (running_total < sg_dma_len(sg) && running_total < temp) { > > I think that's the only bug fix in this patch; all the rest of the > changes look like clean up. That is one of the bugs I'm trying to fix. But there are other fixes too. There are a few places where I added the line running_total &= TRB_MAX_BUFF_SIZE - 1; For example, for this original code: /* Scatter gather list entries may cross 64KB boundaries */ running_total = TRB_MAX_BUFF_SIZE - (sg_dma_address(sg) & ((1 << TRB_MAX_BUFF_SHIFT) - 1)); if you rewrite the expression as the equivalent: running_total = TRB_MAX_BUFF_SIZE - (sg_dma_address(sg) & (TRB_MAX_BUFF_SIZE - 1)); then you can see the problem better. If sg_dma_address(sg) is 64K aligned (0xbfff0000 for example), then: running_total = TRB_MAX_BUFF_SIZE - (0xbfff0000 & (TRB_MAX_BUFF_SIZE - 1)); running_total = 0x10000 - (0xbfff0000 & 0xffff); running_total = 0x10000; which is wrong. The answer should be 0 if the buffer is aligned. Adding running_total &= TRB_MAX_BUFF_SIZE - 1; makes the answer correct. -- Paul > So the original code would fail if the driver submitted a scatter gather > list where the total for all the entries was greater than > urb->transfer_buffer_length, and at least one of the entries in the list > was greater than 64KB. Is that correct? > > I think something like this scatter gather list might fail, if > urb->transfer_buffer_length = 5K. > > ----------------------- > | entry 1 | > | 4K | > ----------------------- > ----------------------- > | entry 2 | > | 3K | > | | > | -- 64KB boundary -- | > | | > | 1K | > ----------------------- > > The original code would say there needs to be 3 TRBs for this list, when > there only needs to be two TRBs. Is that the bug you're trying to fix? > > Sarah Sharp -- 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
