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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
