diff mbox series

[v7,31/36] staging: tegra-vde: fix common struct sg_table related issues

Message ID 20200619103636.11974-32-m.szyprowski@samsung.com (mailing list archive)
State New, archived
Headers show
Series DRM: fix struct sg_table nents vs. orig_nents misuse | expand

Commit Message

Marek Szyprowski June 19, 2020, 10:36 a.m. UTC 
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/staging/media/tegra-vde/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

kernel test robot June 20, 2020, 9:10 p.m. UTC | #1 
Hi Marek,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20200618]
[also build test ERROR on v5.8-rc1]
[cannot apply to linuxtv-media/master staging/staging-testing drm-exynos/exynos-drm-next drm-intel/for-linux-next linus/master v5.8-rc1 v5.7 v5.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Szyprowski/DRM-fix-struct-sg_table-nents-vs-orig_nents-misuse/20200619-184302
base:    ce2cc8efd7a40cbd17841add878cb691d0ce0bba
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/tegra-vde/iommu.c: In function 'tegra_vde_iommu_map':
>> drivers/staging/media/tegra-vde/iommu.c:39:9: error: implicit declaration of function 'iommu_map_sgtable'; did you mean 'dma_map_sgtable'? [-Werror=implicit-function-declaration]
      39 |  size = iommu_map_sgtable(vde->domain, addr, sgt,
         |         ^~~~~~~~~~~~~~~~~
         |         dma_map_sgtable
   cc1: some warnings being treated as errors

vim +39 drivers/staging/media/tegra-vde/iommu.c

    18	
    19	int tegra_vde_iommu_map(struct tegra_vde *vde,
    20				struct sg_table *sgt,
    21				struct iova **iovap,
    22				size_t size)
    23	{
    24		struct iova *iova;
    25		unsigned long shift;
    26		unsigned long end;
    27		dma_addr_t addr;
    28	
    29		end = vde->domain->geometry.aperture_end;
    30		size = iova_align(&vde->iova, size);
    31		shift = iova_shift(&vde->iova);
    32	
    33		iova = alloc_iova(&vde->iova, size >> shift, end >> shift, true);
    34		if (!iova)
    35			return -ENOMEM;
    36	
    37		addr = iova_dma_addr(&vde->iova, iova);
    38	
  > 39		size = iommu_map_sgtable(vde->domain, addr, sgt,
    40					 IOMMU_READ | IOMMU_WRITE);
    41		if (!size) {
    42			__free_iova(&vde->iova, iova);
    43			return -ENXIO;
    44		}
    45	
    46		*iovap = iova;
    47	
    48		return 0;
    49	}
    50	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dmitry Osipenko June 21, 2020, 4 a.m. UTC | #2 
В Fri, 19 Jun 2020 12:36:31 +0200
Marek Szyprowski <m.szyprowski@samsung.com> пишет:

> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg()
> function returns the number of the created entries in the DMA address
> space. However the subsequent calls to the
> dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with
> the original number of the entries passed to the dma_map_sg().
> 
> struct sg_table is a common structure used for describing a
> non-contiguous memory buffer, used commonly in the DRM and graphics
> subsystems. It consists of a scatterlist with memory pages and DMA
> addresses (sgl entry), as well as the number of scatterlist entries:
> CPU pages (orig_nents entry) and DMA mapped pages (nents entry).
> 
> It turned out that it was a common mistake to misuse nents and
> orig_nents entries, calling DMA-mapping functions with a wrong number
> of entries or ignoring the number of mapped entries returned by the
> dma_map_sg() function.
> 
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/staging/media/tegra-vde/iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-vde/iommu.c
> b/drivers/staging/media/tegra-vde/iommu.c index
> 6af863d92123..adf8dc7ee25c 100644 ---
> a/drivers/staging/media/tegra-vde/iommu.c +++
> b/drivers/staging/media/tegra-vde/iommu.c @@ -36,8 +36,8 @@ int
> tegra_vde_iommu_map(struct tegra_vde *vde, 
>  	addr = iova_dma_addr(&vde->iova, iova);
>  
> -	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
> -			    IOMMU_READ | IOMMU_WRITE);
> +	size = iommu_map_sgtable(vde->domain, addr, sgt,
> +				 IOMMU_READ | IOMMU_WRITE);
>  	if (!size) {
>  		__free_iova(&vde->iova, iova);
>  		return -ENXIO;

Ahh, I saw the build failure report. You're changing the DMA API in
this series, while DMA API isn't used by this driver, it uses IOMMU
API. Hence there is no need to touch this code. Similar problem in the
host1x driver patch.
Marek Szyprowski June 30, 2020, 10:07 a.m. UTC | #3 
On 21.06.2020 06:00, Dmitry Osipenko wrote:
> В Fri, 19 Jun 2020 12:36:31 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> пишет:
>
>> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg()
>> function returns the number of the created entries in the DMA address
>> space. However the subsequent calls to the
>> dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with
>> the original number of the entries passed to the dma_map_sg().
>>
>> struct sg_table is a common structure used for describing a
>> non-contiguous memory buffer, used commonly in the DRM and graphics
>> subsystems. It consists of a scatterlist with memory pages and DMA
>> addresses (sgl entry), as well as the number of scatterlist entries:
>> CPU pages (orig_nents entry) and DMA mapped pages (nents entry).
>>
>> It turned out that it was a common mistake to misuse nents and
>> orig_nents entries, calling DMA-mapping functions with a wrong number
>> of entries or ignoring the number of mapped entries returned by the
>> dma_map_sg() function.
>>
>> To avoid such issues, lets use a common dma-mapping wrappers operating
>> directly on the struct sg_table objects and use scatterlist page
>> iterators where possible. This, almost always, hides references to the
>> nents and orig_nents entries, making the code robust, easier to follow
>> and copy/paste safe.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>   drivers/staging/media/tegra-vde/iommu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/tegra-vde/iommu.c
>> b/drivers/staging/media/tegra-vde/iommu.c index
>> 6af863d92123..adf8dc7ee25c 100644 ---
>> a/drivers/staging/media/tegra-vde/iommu.c +++
>> b/drivers/staging/media/tegra-vde/iommu.c @@ -36,8 +36,8 @@ int
>> tegra_vde_iommu_map(struct tegra_vde *vde,
>>   	addr = iova_dma_addr(&vde->iova, iova);
>>   
>> -	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
>> -			    IOMMU_READ | IOMMU_WRITE);
>> +	size = iommu_map_sgtable(vde->domain, addr, sgt,
>> +				 IOMMU_READ | IOMMU_WRITE);
>>   	if (!size) {
>>   		__free_iova(&vde->iova, iova);
>>   		return -ENXIO;
> Ahh, I saw the build failure report. You're changing the DMA API in
> this series, while DMA API isn't used by this driver, it uses IOMMU
> API. Hence there is no need to touch this code. Similar problem in the
> host1x driver patch.

The issue is caused by the lack of iommu_map_sgtable() stub when no 
IOMMU support is configured. I've posted a patch for this:

https://lore.kernel.org/lkml/20200630081756.18526-1-m.szyprowski@samsung.com/

The patch for this driver is fine, we have to wait until the above fix 
gets merged and then it can be applied during the next release cycle.

Best regards
Dmitry Osipenko July 1, 2020, 1:45 a.m. UTC | #4 
30.06.2020 13:07, Marek Szyprowski пишет:
> On 21.06.2020 06:00, Dmitry Osipenko wrote:
>> В Fri, 19 Jun 2020 12:36:31 +0200
>> Marek Szyprowski <m.szyprowski@samsung.com> пишет:
>>
>>> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg()
>>> function returns the number of the created entries in the DMA address
>>> space. However the subsequent calls to the
>>> dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with
>>> the original number of the entries passed to the dma_map_sg().
>>>
>>> struct sg_table is a common structure used for describing a
>>> non-contiguous memory buffer, used commonly in the DRM and graphics
>>> subsystems. It consists of a scatterlist with memory pages and DMA
>>> addresses (sgl entry), as well as the number of scatterlist entries:
>>> CPU pages (orig_nents entry) and DMA mapped pages (nents entry).
>>>
>>> It turned out that it was a common mistake to misuse nents and
>>> orig_nents entries, calling DMA-mapping functions with a wrong number
>>> of entries or ignoring the number of mapped entries returned by the
>>> dma_map_sg() function.
>>>
>>> To avoid such issues, lets use a common dma-mapping wrappers operating
>>> directly on the struct sg_table objects and use scatterlist page
>>> iterators where possible. This, almost always, hides references to the
>>> nents and orig_nents entries, making the code robust, easier to follow
>>> and copy/paste safe.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>   drivers/staging/media/tegra-vde/iommu.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/tegra-vde/iommu.c
>>> b/drivers/staging/media/tegra-vde/iommu.c index
>>> 6af863d92123..adf8dc7ee25c 100644 ---
>>> a/drivers/staging/media/tegra-vde/iommu.c +++
>>> b/drivers/staging/media/tegra-vde/iommu.c @@ -36,8 +36,8 @@ int
>>> tegra_vde_iommu_map(struct tegra_vde *vde,
>>>   	addr = iova_dma_addr(&vde->iova, iova);
>>>   
>>> -	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
>>> -			    IOMMU_READ | IOMMU_WRITE);
>>> +	size = iommu_map_sgtable(vde->domain, addr, sgt,
>>> +				 IOMMU_READ | IOMMU_WRITE);
>>>   	if (!size) {
>>>   		__free_iova(&vde->iova, iova);
>>>   		return -ENXIO;
>> Ahh, I saw the build failure report. You're changing the DMA API in
>> this series, while DMA API isn't used by this driver, it uses IOMMU
>> API. Hence there is no need to touch this code. Similar problem in the
>> host1x driver patch.
> 
> The issue is caused by the lack of iommu_map_sgtable() stub when no 
> IOMMU support is configured. I've posted a patch for this:
> 
> https://lore.kernel.org/lkml/20200630081756.18526-1-m.szyprowski@samsung.com/
> 
> The patch for this driver is fine, we have to wait until the above fix 
> gets merged and then it can be applied during the next release cycle.

Thank you for the clarification!
diff mbox series

Patch

diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
index 6af863d92123..adf8dc7ee25c 100644
--- a/drivers/staging/media/tegra-vde/iommu.c
+++ b/drivers/staging/media/tegra-vde/iommu.c
@@ -36,8 +36,8 @@  int tegra_vde_iommu_map(struct tegra_vde *vde,
 
 	addr = iova_dma_addr(&vde->iova, iova);
 
-	size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
-			    IOMMU_READ | IOMMU_WRITE);
+	size = iommu_map_sgtable(vde->domain, addr, sgt,
+				 IOMMU_READ | IOMMU_WRITE);
 	if (!size) {
 		__free_iova(&vde->iova, iova);
 		return -ENXIO;