On Sun, 16 Mar 2008 13:18:07 -0400 (EDT) Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Sun, 16 Mar 2008, FUJITA Tomonori wrote: > > > On Fri, 14 Mar 2008 10:46:52 -0400 (EDT) > > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > On Fri, 14 Mar 2008, FUJITA Tomonori wrote: > > > > > > > > If nents doesn't change then for_each_sg() won't work right. There > > > > > could be an alternative macro: > > > > > > > > Oops, I thought that for_each_sg is defined like: > > > > > > > > #define for_each_sg(sglist, sg, nr, __i) \ > > > > for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg)) > > > > > > > > > > > > > /* > > > > > * Loop over each sg element, stopping at the end of the chain > > > > > */ > > > > > #define for_each_sg_all(sglist, sg, __i) \ > > > > > for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg)) > > > > > > > > > > If you added this macro to include/linux/scatterlist.h and used it > > > > > instead of for_each_sg() then you can get rid of nents entirely. > > > > > However I'm not sure whether this would be safe. Do people sometimes > > > > > use a subset of the entries in a scatterlist? > > > > > > > > IIRC, some drivers do that (though they might use sg_next). > > > > > > But will usb-storage ever receive a scatterlist like that? For > > > example, if there are three 4096-byte entries in the list, but the > > > transfer length is 8192 and nents is 2, then there could be a problem. > > > > If LLDs don't use the padding or drain buffer feature (USB uses > > neither), scsi midlayer doesn't send such (that is, the block layer > > doesn't create such). > > Then it should be okay to open-code that loop and test whether sg is > NULL. > > > As I explained above, it should be safe with USB. But in general, LLDs > > should not rely on a scatterlist about how much data they transfer. > > True. Let's add a comment explaining the situation. For example: > > /* If this routine is called multiple times for a single > * scatterlist, the nents value will not be updated properly. > * Transfers will stop when the end of the list is reached, > * which might not be correct in cases where nents is less than > * the actual number of entries in the list. However, if > * drivers are careful not to use multiple calls to transfer > * more data than was requested then this will be safe. > */ > for (sg = *sgl; nents > 0 && sg; --nents, sg = sg_next(sg)) { > > When this change is made, we will be able to convert usb-storage over > to using your library routine. For me, it would be much better to fix USB drivers since LLDs should not rely on a scatterlist about how much data they transfer, as I said. If LLDs do that, they are broken. It's not good to add such a workaround to the common API for broken LLDs. USB drivers should do something like this if they definitely need to call this API multiple times: done = sg_copy_buffer(buffer, scsi_bufflen(sc) - us->done_length, us->done_length += done; -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html