Re: [PATCH 1/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

From: Marek Szyprowski
Date: Wed Feb 11 2015 - 04:00:54 EST


On 2015-02-11 09:37, Ricardo Ribalda Delgado wrote:
Hello Marek
On Wed, Feb 11, 2015 at 9:06 AM, Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
Unfortunately nent differs in sign to the output of dma_map_sg, so an
intermediate value must be used.

I don't get this part. dma_map_sg() returns the number of scatter list
entries mapped
to the hardware or zero if anything fails. What is the problem of assigning
it directly
to nents?
Are you sure about that?

The prototype of the function is (from dma-mapping-common.h)
static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction dir,
struct dma_attrs *attrs)

which calls map_sg at the struct dma_map_ops (dma-mapping.h)

int (*map_sg)(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction dir,
struct dma_attrs *attrs);

Both return int instead of unsigned int....

Well, this int return value seems to be misleading, but according to
Documentation/DMA-API.txt, the only error value is zero:

"As with the other mapping interfaces, dma_map_sg() can fail. When it
does, 0 is returned and a driver must take appropriate action. It is
critical that the driver do something, in the case of a block driver
aborting the request or even oopsing is better than doing nothing and
corrupting the filesystem."

I've also checked various dma-mapping implementation for different
architectures and they follow this convention.

Maybe one should add some comments to include/linux/dma_mapping.h to
clarify this and avoid further confusion.

dma_map_sg_attrs() return 0 in case of error, so the check can be
there is no need for temporary variable.
Check last comment

vm_unmap_ram(buf->vaddr, buf->num_pages);
@@ -463,7 +470,7 @@ static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf
*dbuf, struct device *dev
rd = buf->dma_sgt->sgl;
wr = sgt->sgl;
- for (i = 0; i < sgt->orig_nents; ++i) {
+ for (i = 0; i < sgt->nents; ++i) {

Here the code iterates over every memory page in the scatter list (to create
a copy of it), not the device mapped chunks, so it must use orig_nents
like it was already there.
At that point both have the same value, but you are right, it is more
clear to use orig_nents

I will resend a version using orig_nents in dmabug_ops attach, but
please take a look the map_sg, I think it can return <0

Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at