Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori <tomof@acm.org>]

On Sun, 16 Mar 2008 13:18:07 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Sun, 16 Mar 2008, FUJITA Tomonori wrote:
> 
> > On Fri, 14 Mar 2008 10:46:52 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> 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;