On Thu, Jan 13, 2011 at 03:38:07PM -0800, Paul Zimmerman wrote: > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > > > 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. Ok, it really sounds like this patch needs to be three patches: - one cleanup patch that changes instances of (1 << TRB_MAX_BUFF_SHIFT) to TRB_MAX_BUFF_SIZE, and changes dev_dbg to dev_err where necessary. - one bug fix patch for the scatter gather list fix for the while loop - one bug fix that makes sure to AND the running total to avoid the 64K alignment issue you described above. I think that will clarify what you're trying to accomplish. The TRB math code is fairly hairy, and I want to document any bugs in it for future people who may not search the linux-usb mailing list. The last two patches need to have detailed commit messages that describe what the problem is before the patch and how the patch fixes it. Using my example for the first commit message and your example for the second commit message would be fine. Sorry to ask you to respin this, but if I can't easily spot all the bug fixes in the patch now, I won't remember them 6 months down the road. :) 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
