On Wed, 12 Mar 2008, FUJITA Tomonori wrote:
> I fixed some minor issues (comments, warning, etc) and I've uploaded
> the patchset:
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git
> sg-copy
Sorry, I don't see it there yet in the gitweb interface. Here are my
comments on the parts you posted earlier.
> How about this?
>
> 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.
> +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.
> + 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.
> + 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.
> + }
> +
> + *sgl = sg;
> +
> + return buf_off;
> +}
> +
> +/**
> + * sg_copy_from_buffer - Copy from liner buffer to an SG table
s/liner/linear/
> + * @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/
> + *
> + **/
> +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.
> +}
> +EXPORT_SYMBOL(sg_copy_from_buffer);
> +
> +/**
> + * sg_copy_to_buffer - Copy from an SG table to liner buffer
s/liner/linear/
> + * @sgl: The SG table
> + * @nents: Number of SG entries
> + * @buf: Where to copy to
> + * @buflen: The number of bytes to copy
> + *
> + * Returns the number of copied byte.
s/byte/bytes/
> + *
> + **/
> +int sg_copy_to_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, 1);
Same thing here.
> +}
> +EXPORT_SYMBOL(sg_copy_to_buffer);
On the whole it looks good.
Alan Stern
--
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
