On Wed, 12 Mar 2008 12:01:57 -0400 (EDT)
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> > index a3d567a..8951e3c 100644
> > --- a/include/linux/scatterlist.h
> > +++ b/include/linux/scatterlist.h
> > @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
> > sg_alloc_fn *);
> > int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
> >
> > +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> > + unsigned long *offset, void *buf, int buflen, int to_buffer);
> > +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> > + void *buf, int buflen);
> > +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> > + void *buf, int buflen);
>
> These routines probably should not return int. Maybe unsigned int or
> unsigned long. If you really want to be picky, it should be size_t.
>
> Same goes for the type of the buflen parameter.
OK, let's use size_t.
> > +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> > + unsigned long *offset, void *buf, int buflen, int to_buffer)
> > +{
> > + struct scatterlist *sg;
> > + unsigned long buf_off = 0;
>
> The type of buf_off should be the same as the function's return type.
Fixed.
> > + int i;
> > +
> > + WARN_ON(!irqs_disabled());
> > +
> > + for_each_sg(*sgl, sg, nents, i) {
>
> Will there be a problem in subsequent calls if *sgl has been
> incremented but nents hasn't been changed? Maybe nents needs to be a
> pointer also.
usb_stor_access_xfer_buf doesn't check scsi_sg_count (the number of sg
entries). It assumes that callers take care about the issue.
If you want nents to be a pointer, I'm fine with it.
> > + struct page *page;
> > + int n = 0;
> > + unsigned int sg_off = sg->offset;
> > + unsigned int sg_copy = sg->length;
> > +
> > + BUG_ON(*offset > sg_copy);
> > +
> > + if (!buflen)
> > + break;
> > +
> > + sg_off += *offset;
> > + n = sg_off >> PAGE_SHIFT;
> > + sg_off &= ~PAGE_MASK;
> > + sg_copy -= *offset;
> > +
> > + if (sg_copy > buflen) {
> > + sg_copy = buflen;
> > + *offset += sg_copy;
> > + } else
> > + *offset = 0;
> > +
> > + buflen -= sg_copy;
> > +
> > + while (sg_copy > 0) {
> > + unsigned int page_copy;
> > + void *p;
> > +
> > + page_copy = PAGE_SIZE - sg_off;
> > + if (page_copy > sg_copy)
> > + page_copy = sg_copy;
> > +
> > + page = nth_page(sg_page(sg), n);
> > + p = kmap_atomic(page, KM_BIO_SRC_IRQ);
> > +
> > + if (to_buffer)
> > + memcpy(buf + buf_off, p + sg_off, page_copy);
> > + else {
> > + memcpy(p + sg_off, buf + buf_off, page_copy);
> > + flush_kernel_dcache_page(page);
> > + }
> > +
> > + kunmap_atomic(p, KM_BIO_SRC_IRQ);
> > +
> > + buf_off += page_copy;
> > + sg_off += page_copy;
> > + if (sg_off == PAGE_SIZE) {
> > + sg_off = 0;
> > + n++;
> > + }
> > + sg_copy -= page_copy;
> > + }
>
> Here you need to add:
>
> if (*offset)
> break;
>
> Otherwise *sgl will be incremented wrongly if the transfer ends in the
> middle of an sg entry.
Thanks, fixed.
> > + }
> > +
> > + *sgl = sg;
> > +
> > + return buf_off;
> > +}
>
> > +
> > +/**
> > + * sg_copy_from_buffer - Copy from liner buffer to an SG table
>
> s/liner/linear/
It was fixed in the git tree.
> > + * @sgl: The SG table
> > + * @nents: Number of SG entries
> > + * @buf: Where to copy from
> > + * @buflen: The number of bytes to copy
> > + *
> > + * Returns the number of copied byte.
>
> s/byte/bytes/
Fixed, thanks.
> > + *
> > + **/
> > +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> > + void *buf, int buflen)
> > +{
> > + struct scatterlist *s = sgl;
> > + unsigned long offset = 0;
> > +
> > + return sg_copy_buffer(&s, nents, &offset, buf, buflen, 0);
>
> You don't have to copy sgl into s. Just pass &sgl directly.
Duh, fixed.
Here's an updated version.
=
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index a3d567a..2f863f3 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -213,6 +213,14 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
sg_alloc_fn *);
int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+size_t sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+ unsigned int *offset, void *buf, size_t buflen,
+ int to_buffer);
+size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+ void *buf, size_t buflen);
+size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+ void *buf, size_t buflen);
+
/*
* Maximum number of entries that will be allocated in one piece, if
* a list larger than this is required then chaining will be utilized.
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index acca490..4f0813c 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -8,6 +8,7 @@
*/
#include <linux/module.h>
#include <linux/scatterlist.h>
+#include <linux/highmem.h>
/**
* sg_next - return the next scatterlist entry in a list
@@ -292,3 +293,129 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
return ret;
}
EXPORT_SYMBOL(sg_alloc_table);
+
+/**
+ * sg_copy_buffer - Copy data between a linear buffer and an SG list
+ * @sgl: The SG list
+ * @nents: Number of SG entries
+ * @offset: start data transfer in the first SG entry at
+ * @buf: Where to copy from
+ * @buflen: The number of bytes to copy
+ * @to_buffer: transfer direction (non zero == from an sg list to a
+ * buffer, 0 == from a buffer to an sg list
+ *
+ * Returns the number of copied bytes.
+ *
+ * *sgl is updated to point a SG list that next data transfer should
+ * start with. *offset is updated to a value that next data transfer
+ * should use.
+ *
+ **/
+size_t sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+ unsigned int *offset, void *buf, size_t buflen,
+ int to_buffer)
+{
+ struct scatterlist *sg;
+ size_t buf_off = 0;
+ int i;
+
+ WARN_ON(!irqs_disabled());
+
+ for_each_sg(*sgl, sg, nents, i) {
+ struct page *page;
+ int n = 0;
+ unsigned int sg_off = sg->offset;
+ unsigned int sg_copy = sg->length;
+
+ BUG_ON(*offset > sg_copy);
+
+ if (!buflen)
+ break;
+
+ sg_off += *offset;
+ n = sg_off >> PAGE_SHIFT;
+ sg_off &= ~PAGE_MASK;
+ sg_copy -= *offset;
+
+ if (sg_copy > buflen) {
+ sg_copy = buflen;
+ *offset += sg_copy;
+ } else
+ *offset = 0;
+
+ buflen -= sg_copy;
+
+ while (sg_copy > 0) {
+ unsigned int page_copy;
+ void *p;
+
+ page_copy = PAGE_SIZE - sg_off;
+ if (page_copy > sg_copy)
+ page_copy = sg_copy;
+
+ page = nth_page(sg_page(sg), n);
+ p = kmap_atomic(page, KM_BIO_SRC_IRQ);
+
+ if (to_buffer)
+ memcpy(buf + buf_off, p + sg_off, page_copy);
+ else {
+ memcpy(p + sg_off, buf + buf_off, page_copy);
+ flush_kernel_dcache_page(page);
+ }
+
+ kunmap_atomic(p, KM_BIO_SRC_IRQ);
+
+ buf_off += page_copy;
+ sg_off += page_copy;
+ if (sg_off == PAGE_SIZE) {
+ sg_off = 0;
+ n++;
+ }
+ sg_copy -= page_copy;
+ }
+
+ if (*offset)
+ break;
+ }
+
+ *sgl = sg;
+
+ return buf_off;
+}
+EXPORT_SYMBOL(sg_copy_buffer);
+
+/**
+ * sg_copy_from_buffer - Copy from a linear buffer to an SG list
+ * @sgl: The SG list
+ * @nents: Number of SG entries
+ * @buf: Where to copy from
+ * @buflen: The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+ void *buf, size_t buflen)
+{
+ unsigned int offset = 0;
+ return sg_copy_buffer(&sgl, nents, &offset, buf, buflen, 0);
+}
+EXPORT_SYMBOL(sg_copy_from_buffer);
+
+/**
+ * sg_copy_to_buffer - Copy from an SG list to a linear buffer
+ * @sgl: The SG list
+ * @nents: Number of SG entries
+ * @buf: Where to copy to
+ * @buflen: The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+ void *buf, size_t buflen)
+{
+ unsigned int offset = 0;
+ return sg_copy_buffer(&sgl, nents, &offset, buf, buflen, 1);
+}
+EXPORT_SYMBOL(sg_copy_to_buffer);
--
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
