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

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

[Ricardo Ribalda Delgado]

When sg_alloc_table_from_pages() does not fail it returns a sg_table
structure with nents and nents_orig initialized to the same value.

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

The output of dma_map, should be used to transverse the scatter list.

dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

sg_free_tables uses also orig_nent.

This patch fix the file to follow this paradigm.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index b1838ab..40c330f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -147,8 +147,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size,
 	 * No need to sync to the device, this will happen later when the
 	 * prepare() memop is called.
 	 */
-	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-			     buf->dma_dir, &attrs) == 0)
+	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+				      buf->dma_dir, &attrs);
+	if (!sgt->nents)
 		goto fail_map;
 
 	buf->handler.refcount = &buf->refcount;
@@ -187,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
 		dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
 		dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
 			buf->num_pages);
-		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
+		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
 				   buf->dma_dir, &attrs);
 		if (buf->vaddr)
 			vm_unmap_ram(buf->vaddr, buf->num_pages);
@@ -314,9 +315,11 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
 	 * No need to sync to the device, this will happen later when the
 	 * prepare() memop is called.
 	 */
-	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-			     buf->dma_dir, &attrs) == 0)
+	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+				      buf->dma_dir, &attrs);
+	if (!sgt->nents)
 		goto userptr_fail_map;
+
 	return buf;
 
 userptr_fail_map:
@@ -351,7 +354,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
 
 	dprintk(1, "%s: Releasing userspace buffer of %d pages\n",
 	       __func__, buf->num_pages);
-	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, &attrs);
+	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
+			   &attrs);
 	if (buf->vaddr)
 		vm_unmap_ram(buf->vaddr, buf->num_pages);
 	sg_free_table(buf->dma_sgt);
@@ -502,7 +506,6 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
 	/* stealing dmabuf mutex to serialize map/unmap operations */
 	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
-	int ret;
 
 	mutex_lock(lock);
 
@@ -521,8 +524,9 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
 	}
 
 	/* mapping to the client with new direction */
-	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-	if (ret <= 0) {
+	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+				dma_dir);
+	if (!sgt->nents) {
 		pr_err("failed to map scatterlist\n");
 		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
-- 
2.1.4

[PATCH v2 2/3] media/videobuf2-dma-contig: Save output from dma_map_sg

[Ricardo Ribalda Delgado]

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index b481d20..bfb5917 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -299,7 +299,6 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 	/* stealing dmabuf mutex to serialize map/unmap operations */
 	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
-	int ret;
 
 	mutex_lock(lock);
 
@@ -318,8 +317,9 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 	}
 
 	/* mapping to the client with new direction */
-	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-	if (ret <= 0) {
+	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+				dma_dir);
+	if (!sgt->nents) {
 		pr_err("failed to map scatterlist\n");
 		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
-- 
2.1.4

Re: [PATCH v2 2/3] media/videobuf2-dma-contig: Save output from dma_map_sg

[Marek Szyprowski]

Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:
> dma_map_sg returns the number of areas mapped by the hardware,
> which could be different than the areas given as an input.
> The output must be saved to nent.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index b481d20..bfb5917 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -299,7 +299,6 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
>   	/* stealing dmabuf mutex to serialize map/unmap operations */
>   	struct mutex *lock = &db_attach->dmabuf->lock;
>   	struct sg_table *sgt;
> -	int ret;
>   
>   	mutex_lock(lock);
>   
> @@ -318,8 +317,9 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
>   	}
>   
>   	/* mapping to the client with new direction */
> -	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
> -	if (ret <= 0) {
> +	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
> +				dma_dir);
> +	if (!sgt->nents) {
>   		pr_err("failed to map scatterlist\n");
>   		mutex_unlock(lock);
>   		return ERR_PTR(-EIO);

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

[PATCH v2 3/3] media/videobuf2-dma-vmalloc: Save output from dma_map_sg

[Ricardo Ribalda Delgado]

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
index bcde885..f92bc9e 100644
--- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
@@ -287,7 +287,6 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
 	/* stealing dmabuf mutex to serialize map/unmap operations */
 	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
-	int ret;
 
 	mutex_lock(lock);
 
@@ -306,8 +305,9 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
 	}
 
 	/* mapping to the client with new direction */
-	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-	if (ret <= 0) {
+	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+				dma_dir);
+	if (!sgt->nents) {
 		pr_err("failed to map scatterlist\n");
 		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
-- 
2.1.4

Re: [PATCH v2 3/3] media/videobuf2-dma-vmalloc: Save output from dma_map_sg

[Marek Szyprowski]

Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:
> dma_map_sg returns the number of areas mapped by the hardware,
> which could be different than the areas given as an input.
> The output must be saved to nent.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>   drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> index bcde885..f92bc9e 100644
> --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> @@ -287,7 +287,6 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
>   	/* stealing dmabuf mutex to serialize map/unmap operations */
>   	struct mutex *lock = &db_attach->dmabuf->lock;
>   	struct sg_table *sgt;
> -	int ret;
>   
>   	mutex_lock(lock);
>   
> @@ -306,8 +305,9 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
>   	}
>   
>   	/* mapping to the client with new direction */
> -	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
> -	if (ret <= 0) {
> +	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
> +				dma_dir);
> +	if (!sgt->nents) {
>   		pr_err("failed to map scatterlist\n");
>   		mutex_unlock(lock);
>   		return ERR_PTR(-EIO);

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

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

[Marek Szyprowski]

Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:
> When sg_alloc_table_from_pages() does not fail it returns a sg_table
> structure with nents and nents_orig initialized to the same value.
>
> dma_map_sg returns the number of areas mapped by the hardware,
> which could be different than the areas given as an input.
> The output must be saved to nent.
>
> The output of dma_map, should be used to transverse the scatter list.
>
> dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).
>
> sg_free_tables uses also orig_nent.
>
> This patch fix the file to follow this paradigm.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>

I would also consider sending it to stable.

> ---
>   drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index b1838ab..40c330f 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -147,8 +147,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size,
>   	 * No need to sync to the device, this will happen later when the
>   	 * prepare() memop is called.
>   	 */
> -	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> -			     buf->dma_dir, &attrs) == 0)
> +	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
> +				      buf->dma_dir, &attrs);
> +	if (!sgt->nents)
>   		goto fail_map;
>   
>   	buf->handler.refcount = &buf->refcount;
> @@ -187,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
>   		dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
>   		dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
>   			buf->num_pages);
> -		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> +		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
>   				   buf->dma_dir, &attrs);
>   		if (buf->vaddr)
>   			vm_unmap_ram(buf->vaddr, buf->num_pages);
> @@ -314,9 +315,11 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
>   	 * No need to sync to the device, this will happen later when the
>   	 * prepare() memop is called.
>   	 */
> -	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> -			     buf->dma_dir, &attrs) == 0)
> +	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
> +				      buf->dma_dir, &attrs);
> +	if (!sgt->nents)
>   		goto userptr_fail_map;
> +
>   	return buf;
>   
>   userptr_fail_map:
> @@ -351,7 +354,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
>   
>   	dprintk(1, "%s: Releasing userspace buffer of %d pages\n",
>   	       __func__, buf->num_pages);
> -	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, &attrs);
> +	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
> +			   &attrs);
>   	if (buf->vaddr)
>   		vm_unmap_ram(buf->vaddr, buf->num_pages);
>   	sg_free_table(buf->dma_sgt);
> @@ -502,7 +506,6 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
>   	/* stealing dmabuf mutex to serialize map/unmap operations */
>   	struct mutex *lock = &db_attach->dmabuf->lock;
>   	struct sg_table *sgt;
> -	int ret;
>   
>   	mutex_lock(lock);
>   
> @@ -521,8 +524,9 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
>   	}
>   
>   	/* mapping to the client with new direction */
> -	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
> -	if (ret <= 0) {
> +	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
> +				dma_dir);
> +	if (!sgt->nents) {
>   		pr_err("failed to map scatterlist\n");
>   		mutex_unlock(lock);
>   		return ERR_PTR(-EIO);

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

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

[Hans Verkuil]

Hi Ricardo, Marek,

I have a few questions, mostly to improve my own understanding.

First of all, is this solving an actual bug for you, or did you just find
it while reviewing code? And if it solves a bug, then which architecture
are you using? ARM? Intel?

On 02/11/2015 11:33 AM, Ricardo Ribalda Delgado wrote:
> When sg_alloc_table_from_pages() does not fail it returns a sg_table
> structure with nents and nents_orig initialized to the same value.
> 
> dma_map_sg returns the number of areas mapped by the hardware,
> which could be different than the areas given as an input.
> The output must be saved to nent.
> 
> The output of dma_map, should be used to transverse the scatter list.
> 
> dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

I noticed that few dma_unmap_sg calls actually use orig_nents. It makes
me wonder if the dma_unmap_sg documentation is actually correct. It does
clearly state that orig_nents should be used, and it might well be that
the only reason this hasn't led to problems is that very few architectures
actually seem to return nents < orig_nents.

> 
> sg_free_tables uses also orig_nent.
> 
> This patch fix the file to follow this paradigm.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index b1838ab..40c330f 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -147,8 +147,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size,
>  	 * No need to sync to the device, this will happen later when the
>  	 * prepare() memop is called.
>  	 */
> -	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> -			     buf->dma_dir, &attrs) == 0)
> +	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
> +				      buf->dma_dir, &attrs);

Is a driver free to change sgt->nents? It's unclear from the documentation
or code that that is actually the purpose of sgt->nents. Most drivers seem
to store the result of dma_map_sg into a driver-specific struct.

Regards,

	Hans

> +	if (!sgt->nents)
>  		goto fail_map;
>  
>  	buf->handler.refcount = &buf->refcount;
> @@ -187,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
>  		dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
>  		dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
>  			buf->num_pages);
> -		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> +		dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
>  				   buf->dma_dir, &attrs);
>  		if (buf->vaddr)
>  			vm_unmap_ram(buf->vaddr, buf->num_pages);
> @@ -314,9 +315,11 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
>  	 * No need to sync to the device, this will happen later when the
>  	 * prepare() memop is called.
>  	 */
> -	if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
> -			     buf->dma_dir, &attrs) == 0)
> +	sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
> +				      buf->dma_dir, &attrs);
> +	if (!sgt->nents)
>  		goto userptr_fail_map;
> +
>  	return buf;
>  
>  userptr_fail_map:
> @@ -351,7 +354,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
>  
>  	dprintk(1, "%s: Releasing userspace buffer of %d pages\n",
>  	       __func__, buf->num_pages);
> -	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, &attrs);
> +	dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
> +			   &attrs);
>  	if (buf->vaddr)
>  		vm_unmap_ram(buf->vaddr, buf->num_pages);
>  	sg_free_table(buf->dma_sgt);
> @@ -502,7 +506,6 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
>  	/* stealing dmabuf mutex to serialize map/unmap operations */
>  	struct mutex *lock = &db_attach->dmabuf->lock;
>  	struct sg_table *sgt;
> -	int ret;
>  
>  	mutex_lock(lock);
>  
> @@ -521,8 +524,9 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
>  	}
>  
>  	/* mapping to the client with new direction */
> -	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
> -	if (ret <= 0) {
> +	sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
> +				dma_dir);
> +	if (!sgt->nents) {
>  		pr_err("failed to map scatterlist\n");
>  		mutex_unlock(lock);
>  		return ERR_PTR(-EIO);
> 

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

[Ricardo Ribalda Delgado]

Hello Hans

On Fri, Feb 13, 2015 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Ricardo, Marek,
>
> I have a few questions, mostly to improve my own understanding.
>
> First of all, is this solving an actual bug for you, or did you just find
> it while reviewing code? And if it solves a bug, then which architecture
> are you using? ARM? Intel?
>

My arch is intel based (AMD APU). I found it while doing review. While
updating our kernel to 3.19 I had to patch some of my out of tree
drivers, and then I gave a look to the file.

>> dma_map_sg returns the number of areas mapped by the hardware,
>> which could be different than the areas given as an input.
>> The output must be saved to nent.
>>
>> The output of dma_map, should be used to transverse the scatter list.
>>
>> dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).
>
> I noticed that few dma_unmap_sg calls actually use orig_nents. It makes
> me wonder if the dma_unmap_sg documentation is actually correct. It does
> clearly state that orig_nents should be used, and it might well be that
> the only reason this hasn't led to problems is that very few architectures
> actually seem to return nents < orig_nents.

It is not the most clear API to use :(. Some of the prototypes do not
make a lot of sense, and it is documented outside the code.

I have sent these two patches:

https://lkml.org/lkml/2015/2/11/231
https://lkml.org/lkml/2015/2/11/232

>> +     sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
>> +                                   buf->dma_dir, &attrs);
>
> Is a driver free to change sgt->nents? It's unclear from the documentation
> or code that that is actually the purpose of sgt->nents. Most drivers seem
> to store the result of dma_map_sg into a driver-specific struct.

As I understand it, this is the purpose of the struct scatter list,
have at hand the three values that you need,
the sgl, nents and orig_ents.

But it would be great if the maintaner of the dma-api speaks up :)

I am putting  get_maintainer.pl in cc

Thanks Hans!

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

[Hans Verkuil]

On 02/13/2015 04:20 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
> 
> On Fri, Feb 13, 2015 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Hi Ricardo, Marek,
>>
>> I have a few questions, mostly to improve my own understanding.
>>
>> First of all, is this solving an actual bug for you, or did you just find
>> it while reviewing code? And if it solves a bug, then which architecture
>> are you using? ARM? Intel?
>>
> 
> My arch is intel based (AMD APU). I found it while doing review. While
> updating our kernel to 3.19 I had to patch some of my out of tree
> drivers, and then I gave a look to the file.
> 
>>> dma_map_sg returns the number of areas mapped by the hardware,
>>> which could be different than the areas given as an input.
>>> The output must be saved to nent.
>>>
>>> The output of dma_map, should be used to transverse the scatter list.
>>>
>>> dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).
>>
>> I noticed that few dma_unmap_sg calls actually use orig_nents. It makes
>> me wonder if the dma_unmap_sg documentation is actually correct. It does
>> clearly state that orig_nents should be used, and it might well be that
>> the only reason this hasn't led to problems is that very few architectures
>> actually seem to return nents < orig_nents.

Actually, I think I should pay more attention how often I actually write
'actually'. I went a bit too far with that... :-)

> 
> It is not the most clear API to use :(. Some of the prototypes do not
> make a lot of sense, and it is documented outside the code.
> 
> I have sent these two patches:
> 
> https://lkml.org/lkml/2015/2/11/231
> https://lkml.org/lkml/2015/2/11/232
> 
>>> +     sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
>>> +                                   buf->dma_dir, &attrs);
>>
>> Is a driver free to change sgt->nents? It's unclear from the documentation
>> or code that that is actually the purpose of sgt->nents. Most drivers seem
>> to store the result of dma_map_sg into a driver-specific struct.
> 
> As I understand it, this is the purpose of the struct scatter list,
> have at hand the three values that you need,
> the sgl, nents and orig_ents.
> 
> But it would be great if the maintaner of the dma-api speaks up :)

Yes please. And if Ricardo is correct, then someone (janitor job?) should do
a review of dma_unmap_sg in particular.

Regards,

	Hans

> 
> I am putting  get_maintainer.pl in cc
> 
> Thanks Hans!
> 

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

[Ricardo Ribalda Delgado]

Hello all

On Fri, Feb 13, 2015 at 4:32 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Yes please. And if Ricardo is correct, then someone (janitor job?) should do
> a review of dma_unmap_sg in particular.

Perhaps a code snippet inside scatterlist.h will clarify even more.

Would any of the maintainers accept a patch to include a comment like:

struct sg_table *sgt;

sgt = kzalloc(sizeof(*sgt);
if (!sgt){
return -ENOMEM;
}

ret = sg_alloc_table(sgt, N_NENTS, GPF_KERNEL);
if (ret){
  kfree(sgt);
  return ret;
}

//Fill sgt using orig_nents or nents  as index

sgt->nents = dma_map_sg(dev, sgt->sgl, sgt->orig_nents, DIR);
if (!sgt->nents){
  sg_free_table(sgt);
  kfree(sgt);
  return -EIO;
}

//Use nent  as index

dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, DIR);
sg_free_table(sgt);
kfree(sgt);
return 0



Thanks!


-- 
Ricardo Ribalda