LinuxLists.cc - [PATCH v3 00/25] DRM: fix struct sg_table nents vs. orig

2020-05-05 08:45:58

Subject: [PATCH v3 00/25] DRM: fix struct sg_table nents vs. orig_nents misuse

Dear All,

During the Exynos DRM GEM rework and fixing the issues in the
drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most
drivers in DRM framework incorrectly use nents and orig_nents entries of
the struct sg_table.

In case of the most DMA-mapping implementations exchanging those two
entries or using nents for all loops on the scatterlist is harmless,
because they both have the same value. There exists however a DMA-mapping
implementations, for which such incorrect usage breaks things. The nents
returned by dma_map_sg() might be lower than the nents passed as its
parameter and this is perfectly fine. DMA framework or IOMMU is allowed
to join consecutive chunks while mapping if such operation is supported
by the underlying HW (bus, bridge, IOMMU, etc). Example of the case
where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages
is described here [2]

The DMA-mapping framework documentation [3] states that dma_map_sg()
returns the numer of the created entries in the DMA address space.
However the subsequent calls to dma_sync_sg_for_{device,cpu} and
dma_unmap_sg must be called with the original number of entries passed to
dma_map_sg. The common pattern in DRM drivers were to assign the
dma_map_sg() return value to sg_table->nents and use that value for
the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also
the code iterated over nents times to access the pages stored in the
processed scatterlist, while it should use orig_nents as the numer of
the page entries.

I've tried to identify all such incorrect usage of sg_table->nents and
this is a result of my research. It looks that the incorrect pattern has
been copied over the many drivers mainly in the DRM subsystem. Too bad in
most cases it even worked correctly if the system used simple, linear
DMA-mapping implementation, for which swapping nents and orig_nents
doesn't make any difference. To avoid similar issues in the future, I've
introduced a common dma-mapping wrappers, which operate directly on the
sg_table objects.

The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix
it fully. The driver creatively uses sg_table->orig_nents to store the
size of the allocate scatterlist and ignores the number of the entries
returned by dma_map_sg function. In this patchset I only fixed the
sg_table objects exported by dmabuf related functions. I hope that I
didn't break anything there.

Patches are based on top of Linux next-20200504.

Best regards,
Marek Szyprowski

References:

[1] https://lkml.org/lkml/2020/3/27/555
[2] https://lkml.org/lkml/2020/3/29/65
[3] Documentation/DMA-API-HOWTO.txt

Changelog:

v3:
- introduce dma_*_sgtable_* wrappers and use them in all patches

v2: https://lore.kernel.org/linux-iommu/[email protected]/T/
- dropped most of the changes to drm/i915
- added fixes for rcar-du, xen, media and ion
- fixed a few issues pointed by kbuild test robot
- added wide cc: list for each patch

v1: https://lore.kernel.org/linux-iommu/[email protected]/T/
- initial version

Patch summary:

Marek Szyprowski (25):
dma-mapping: add generic helpers for mapping sgtable objects
drm: core: fix common struct sg_table related issues
drm: amdgpu: fix common struct sg_table related issues
drm: armada: fix common struct sg_table related issues
drm: etnaviv: fix common struct sg_table related issues
drm: exynos: fix common struct sg_table related issues
drm: i915: fix common struct sg_table related issues
drm: lima: fix common struct sg_table related issues
drm: msm: fix common struct sg_table related issues
drm: panfrost: fix common struct sg_table related issues
drm: radeon: fix common struct sg_table related issues
drm: rockchip: fix common struct sg_table related issues
drm: tegra: fix common struct sg_table related issues
drm: virtio: fix common struct sg_table related issues
drm: vmwgfx: fix common struct sg_table related issues
xen: gntdev: fix common struct sg_table related issues
drm: host1x: fix common struct sg_table related issues
drm: rcar-du: fix common struct sg_table related issues
dmabuf: fix common struct sg_table related issues
staging: ion: fix common struct sg_table related issues
staging: tegra-vde: fix common struct sg_table related issues
misc: fastrpc: fix common struct sg_table related issues
rapidio: fix common struct sg_table related issues
samples: vfio-mdev/mbochs: fix common struct sg_table related issues
media: pci: fix common ALSA DMA-mapping related codes

drivers/dma-buf/heaps/heap-helpers.c | 13 +++++-----
drivers/dma-buf/udmabuf.c | 7 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 ++---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +++----
drivers/gpu/drm/armada/armada_gem.c | 10 ++++----
drivers/gpu/drm/drm_cache.c | 2 +-
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++----
drivers/gpu/drm/drm_prime.c | 13 +++++-----
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++++-----
drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 +++----
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 ++++------
drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++---
drivers/gpu/drm/lima/lima_gem.c | 11 +++++---
drivers/gpu/drm/msm/msm_gem.c | 13 ++++------
drivers/gpu/drm/msm/msm_iommu.c | 2 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +--
drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 ++--
drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++++----
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 26 +++++++++----------
drivers/gpu/drm/tegra/gem.c | 27 ++++++++------------
drivers/gpu/drm/tegra/plane.c | 15 ++++-------
drivers/gpu/drm/virtio/virtgpu_object.c | 17 ++++++-------
drivers/gpu/drm/virtio/virtgpu_vq.c | 10 +++-----
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +++----------
drivers/gpu/host1x/job.c | 22 ++++++----------
drivers/media/pci/cx23885/cx23885-alsa.c | 2 +-
drivers/media/pci/cx25821/cx25821-alsa.c | 2 +-
drivers/media/pci/cx88/cx88-alsa.c | 2 +-
drivers/media/pci/saa7134/saa7134-alsa.c | 2 +-
drivers/media/platform/vsp1/vsp1_drm.c | 9 ++++---
drivers/misc/fastrpc.c | 4 +--
drivers/rapidio/devices/rio_mport_cdev.c | 8 +++---
drivers/staging/android/ion/ion.c | 25 ++++++++----------
drivers/staging/android/ion/ion_heap.c | 6 ++---
drivers/staging/android/ion/ion_system_heap.c | 2 +-
drivers/staging/media/tegra-vde/iommu.c | 4 +--
drivers/xen/gntdev-dmabuf.c | 7 +++---
include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++
include/linux/iommu.h | 6 +++++
samples/vfio-mdev/mbochs.c | 3 ++-
40 files changed, 202 insertions(+), 207 deletions(-)

--
1.9.1

2020-05-05 08:49:00

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects

struct sg_table is a common structure used for describing a memory
buffer. 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 pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents
entries, call 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 introduce a common wrappers operating directly
on the struct sg_table objects, which take care of the proper use of
the nents and orig_nents entries.

Signed-off-by: Marek Szyprowski <[email protected]>
---
include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++++++++++
include/linux/iommu.h | 6 ++++++
2 files changed, 38 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index b43116a..8364c20d 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -609,6 +609,36 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
return dma_sync_single_for_device(dev, addr + offset, size, dir);
}

+static inline int dma_map_sgtable_attrs(struct device *dev,
+ struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)
+{
+ int n = dma_map_sg_attrs(dev, sgt->sgl, sgt->orig_nents, dir, attrs);
+
+ if (n > 0) {
+ sgt->nents = n;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static inline void dma_unmap_sgtable_attrs(struct device *dev,
+ struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)
+{
+ dma_unmap_sg_attrs(dev, sgt->sgl, sgt->orig_nents, dir, attrs);
+}
+
+static inline void dma_sync_sgtable_for_cpu(struct device *dev,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_sync_sg_for_cpu(dev, sgt->sgl, sgt->orig_nents, dir);
+}
+
+static inline void dma_sync_sgtable_for_device(struct device *dev,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_sync_sg_for_device(dev, sgt->sgl, sgt->orig_nents, dir);
+}
+
#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
#define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0)
#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
@@ -617,6 +647,8 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
#define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
#define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s, 0)
#define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0)
+#define dma_map_sgtable(d, s, r) dma_map_sgtable_attrs(d, s, r, 0)
+#define dma_unmap_sgtable(d, s, r) dma_unmap_sgtable_attrs(d, s, r, 0)

extern int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7ef8b0b..5106b65 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -466,6 +466,12 @@ extern size_t iommu_map_sg_atomic(struct iommu_domain *domain,
extern void iommu_set_fault_handler(struct iommu_domain *domain,
iommu_fault_handler_t handler, void *token);

+static inline size_t iommu_map_sgtable(struct iommu_domain *domain,
+ unsigned long iova, struct sg_table *sgt, int prot)
+{
+ return iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, prot);
+}
+
extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
extern void generic_iommu_put_resv_regions(struct device *dev,
--
1.9.1

2020-05-05 08:49:06

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 23/25] rapidio: fix common struct sg_table related issues

2020-05-05 08:49:09

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 25/25] media: pci: fix common ALSA DMA-mapping related codes

2020-05-05 08:49:23

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 20/25] staging: ion: fix common struct sg_table related issues

The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
numer of the created entries in the DMA address space. However the
subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
called with the original number of the entries passed to dma_map_sg. The
sg_table->nents in turn holds the result of the dma_map_sg call as stated
in include/linux/scatterlist.h. A common mistake was to ignore a result
of the dma_map_sg function and don't use the sg_table->orig_nents at all.

To avoid such issues, lets use common dma-mapping wrappers operating
directly on the struct sg_table objects and adjust references to the
nents and orig_nents respectively.

Signed-off-by: Marek Szyprowski <[email protected]>
---
For more information, see '[PATCH v3 00/25] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread: https://lkml.org/lkml/2020/5/5/187
---
drivers/staging/android/ion/ion.c | 25 +++++++++++--------------
drivers/staging/android/ion/ion_heap.c | 6 +++---
drivers/staging/android/ion/ion_system_heap.c | 2 +-
3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 38b51ea..9274298 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -147,14 +147,14 @@ static struct sg_table *dup_sg_table(struct sg_table *table)
if (!new_table)
return ERR_PTR(-ENOMEM);

- ret = sg_alloc_table(new_table, table->nents, GFP_KERNEL);
+ ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
if (ret) {
kfree(new_table);
return ERR_PTR(-ENOMEM);
}

new_sg = new_table->sgl;
- for_each_sg(table->sgl, sg, table->nents, i) {
+ for_each_sg(table->sgl, sg, table->orig_nents, i) {
memcpy(new_sg, sg, sizeof(*sg));
new_sg->dma_address = 0;
new_sg = sg_next(new_sg);
@@ -224,12 +224,13 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
{
struct ion_dma_buf_attachment *a = attachment->priv;
struct sg_table *table;
+ int ret;

table = a->table;

- if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
- direction))
- return ERR_PTR(-ENOMEM);
+ ret = dma_map_sgtable(attachment->dev, table, direction);
+ if (ret)
+ return ERR_PTR(ret);

return table;
}
@@ -238,7 +239,7 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
struct sg_table *table,
enum dma_data_direction direction)
{
- dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+ dma_unmap_sgtable(attachment->dev, table, direction);
}

static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
@@ -296,10 +297,8 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
}

mutex_lock(&buffer->lock);
- list_for_each_entry(a, &buffer->attachments, list) {
- dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
- direction);
- }
+ list_for_each_entry(a, &buffer->attachments, list)
+ dma_sync_sgtable_for_cpu(a->dev, a->table, direction);

unlock:
mutex_unlock(&buffer->lock);
@@ -319,10 +318,8 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
}

mutex_lock(&buffer->lock);
- list_for_each_entry(a, &buffer->attachments, list) {
- dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
- direction);
- }
+ list_for_each_entry(a, &buffer->attachments, list)
+ dma_sync_sgtable_for_device(a->dev, a->table, direction);
mutex_unlock(&buffer->lock);

return 0;
diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 0755b11..f2f7ca7 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -38,7 +38,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
else
pgprot = pgprot_writecombine(PAGE_KERNEL);

- for_each_sg(table->sgl, sg, table->nents, i) {
+ for_each_sg(table->sgl, sg, table->orig_nents, i) {
int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
struct page *page = sg_page(sg);

@@ -71,7 +71,7 @@ int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
int i;
int ret;

- for_each_sg(table->sgl, sg, table->nents, i) {
+ for_each_sg(table->sgl, sg, table->orig_nents, i) {
struct page *page = sg_page(sg);
unsigned long remainder = vma->vm_end - addr;
unsigned long len = sg->length;
@@ -142,7 +142,7 @@ int ion_heap_buffer_zero(struct ion_buffer *buffer)
else
pgprot = pgprot_writecombine(PAGE_KERNEL);

- return ion_heap_sglist_zero(table->sgl, table->nents, pgprot);
+ return ion_heap_sglist_zero(table->sgl, table->orig_nents, pgprot);
}

int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index b83a1d1..34f6e81 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -162,7 +162,7 @@ static void ion_system_heap_free(struct ion_buffer *buffer)
if (!(buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE))
ion_heap_buffer_zero(buffer);

- for_each_sg(table->sgl, sg, table->nents, i)
+ for_each_sg(table->sgl, sg, table->orig_nents, i)
free_buffer_page(sys_heap, buffer, sg_page(sg));
sg_free_table(table);
kfree(table);
--
1.9.1

2020-05-05 08:49:30

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 22/25] misc: fastrpc: fix common struct sg_table related issues

2020-05-05 08:49:33

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 24/25] samples: vfio-mdev/mbochs: fix common struct sg_table related issues

2020-05-05 08:49:37

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 19/25] dmabuf: fix common struct sg_table related issues

2020-05-05 08:49:45

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 18/25] drm: rcar-du: fix common struct sg_table related issues

2020-05-05 08:49:52

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 21/25] staging: tegra-vde: fix common struct sg_table related issues

2020-05-05 08:49:56

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 14/25] drm: virtio: fix common struct sg_table related issues

2020-05-05 08:50:23

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 15/25] drm: vmwgfx: fix common struct sg_table related issues

2020-05-05 08:50:34

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 10/25] drm: panfrost: fix common struct sg_table related issues

2020-05-05 08:50:41

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 11/25] drm: radeon: fix common struct sg_table related issues

2020-05-05 08:50:42

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 04/25] drm: armada: fix common struct sg_table related issues

2020-05-05 08:50:49

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 12/25] drm: rockchip: fix common struct sg_table related issues

2020-05-05 08:51:34

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 03/25] drm: amdgpu: fix common struct sg_table related issues

2020-05-05 08:51:51

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 16/25] xen: gntdev: fix common struct sg_table related issues

2020-05-05 08:52:44

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 09/25] drm: msm: fix common struct sg_table related issues

2020-05-05 08:52:51

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 07/25] drm: i915: fix common struct sg_table related issues

2020-05-05 08:52:57

by Marek Szyprowski

[permalink] [raw]

Subject: [PATCH v3 06/25] drm: exynos: fix common struct sg_table related issues

2020-05-05 09:26:40

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v3 18/25] drm: rcar-du: fix common struct sg_table related issues

Hi Marek,

On Tue, May 5, 2020 at 10:48 AM Marek Szyprowski
<[email protected]> wrote:
> The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
> numer of the created entries in the DMA address space. However the
> subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
> called with the original number of the entries passed to dma_map_sg. The
> sg_table->nents in turn holds the result of the dma_map_sg call as stated
> in include/linux/scatterlist.h. A common mistake was to ignore a result
> of the dma_map_sg function and don't use the sg_table->orig_nents at all.
>
> To avoid such issues, lets use common dma-mapping wrappers operating
> directly on the struct sg_table objects and adjust references to the
> nents and orig_nents respectively.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> For more information, see '[PATCH v3 00/25] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread: https://lkml.org/lkml/2020/5/5/187

For the modern lore-users:
https://lore.kernel.org/r/[email protected]/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2020-05-05 10:26:42

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects

> +static inline int dma_map_sgtable_attrs(struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)

Two tab indents for parameter continuation, please.

Can we also skip the separate _attrs version? The existing ones have the
separate _attrs variant as there were pre-existing versions without the
attrs argument and lots of users, but that doesn't really apply here as
an extra 0 argument isn't really an issue.

> +static inline size_t iommu_map_sgtable(struct iommu_domain *domain,
> + unsigned long iova, struct sg_table *sgt, int prot)
> +{
> + return iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, prot);
> +}

Should this be a separate patch due to the different subsystems?

FYI, I'll happily pick up the prep patches in an immutable branch of
the dma-mapping tree one we have settled on the details.

2020-05-05 10:46:43

by Marek Szyprowski

[permalink] [raw]

Subject: Re: [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects

Hi Christoph,

On 05.05.2020 12:22, Christoph Hellwig wrote:
>> +static inline int dma_map_sgtable_attrs(struct device *dev,
>> + struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)
> Two tab indents for parameter continuation, please.
>
> Can we also skip the separate _attrs version? The existing ones have the
> separate _attrs variant as there were pre-existing versions without the
> attrs argument and lots of users, but that doesn't really apply here as
> an extra 0 argument isn't really an issue.

Okay.

>> +static inline size_t iommu_map_sgtable(struct iommu_domain *domain,
>> + unsigned long iova, struct sg_table *sgt, int prot)
>> +{
>> + return iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, prot);
>> +}
> Should this be a separate patch due to the different subsystems?
>
> FYI, I'll happily pick up the prep patches in an immutable branch of
> the dma-mapping tree one we have settled on the details.

Okay.

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

2020-05-07 08:50:09

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v3 01/25] dma-mapping: add generic helpers for mapping sgtable objects

Hi Marek,

On 05/05/2020 10:45, Marek Szyprowski wrote:
> struct sg_table is a common structure used for describing a memory
> buffer. 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 pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, call 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 introduce a common wrappers operating directly
> on the struct sg_table objects, which take care of the proper use of
> the nents and orig_nents entries.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++++++++++
> include/linux/iommu.h | 6 ++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index b43116a..8364c20d 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -609,6 +609,36 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
> return dma_sync_single_for_device(dev, addr + offset, size, dir);
> }
>
> +static inline int dma_map_sgtable_attrs(struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)

Please document these new inline functions!

The core reason IMHO why so many drivers do this wrong is that the struct sg_table
documentation is very poor. In particular w.r.t. how the nents and orig_nents
fields should be used.

It confused me, and clearly I am not alone in that.

So while this is a very nice improvement, it doesn't address the core issue, i.e.
the documentation.

Especially some examples on how to use these new functions would be welcome.

Regards,

Hans

> +{
> + int n = dma_map_sg_attrs(dev, sgt->sgl, sgt->orig_nents, dir, attrs);
> +
> + if (n > 0) {
> + sgt->nents = n;
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> +static inline void dma_unmap_sgtable_attrs(struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir, unsigned long attrs)
> +{
> + dma_unmap_sg_attrs(dev, sgt->sgl, sgt->orig_nents, dir, attrs);
> +}
> +
> +static inline void dma_sync_sgtable_for_cpu(struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir)
> +{
> + dma_sync_sg_for_cpu(dev, sgt->sgl, sgt->orig_nents, dir);
> +}
> +
> +static inline void dma_sync_sgtable_for_device(struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir)
> +{
> + dma_sync_sg_for_device(dev, sgt->sgl, sgt->orig_nents, dir);
> +}
> +
> #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
> #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0)
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> @@ -617,6 +647,8 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
> #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
> #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s, 0)
> #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0)
> +#define dma_map_sgtable(d, s, r) dma_map_sgtable_attrs(d, s, r, 0)
> +#define dma_unmap_sgtable(d, s, r) dma_unmap_sgtable_attrs(d, s, r, 0)
>
> extern int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
> void *cpu_addr, dma_addr_t dma_addr, size_t size,
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 7ef8b0b..5106b65 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -466,6 +466,12 @@ extern size_t iommu_map_sg_atomic(struct iommu_domain *domain,
> extern void iommu_set_fault_handler(struct iommu_domain *domain,
> iommu_fault_handler_t handler, void *token);
>
> +static inline size_t iommu_map_sgtable(struct iommu_domain *domain,
> + unsigned long iova, struct sg_table *sgt, int prot)
> +{
> + return iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, prot);
> +}
> +
> extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
> extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
> extern void generic_iommu_put_resv_regions(struct device *dev,
>

2020-05-11 15:56:44

by Steven Price

[permalink] [raw]

Subject: Re: [PATCH v3 10/25] drm: panfrost: fix common struct sg_table related issues

On 05/05/2020 09:45, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
> numer of the created entries in the DMA address space. However the
> subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
> called with the original number of the entries passed to dma_map_sg. The
> sg_table->nents in turn holds the result of the dma_map_sg call as stated
> in include/linux/scatterlist.h. A common mistake was to ignore a result
> of the dma_map_sg function and don't use the sg_table->orig_nents at all.
>
> To avoid such issues, lets use common dma-mapping wrappers operating
> directly on the struct sg_table objects and adjust references to the
> nents and orig_nents respectively.
>
> Signed-off-by: Marek Szyprowski <[email protected]>

The change looks good to me:

Reviewed-by: Steven Price <[email protected]>

Although I would have appreciated the commit message being modified to
match the specifics of Panfrost - the return of dma_mpa_sg() wasn't
being ignored, but the use of orig_nents/nents was indeed wrong.

Steve

> ---
> For more information, see '[PATCH v3 00/25] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread: https://lkml.org/lkml/2020/5/5/187
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 4 ++--
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 ++---
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 17b654e..95d7e80 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -41,8 +41,8 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>
> for (i = 0; i < n_sgt; i++) {
> if (bo->sgts[i].sgl) {
> - dma_unmap_sg(pfdev->dev, bo->sgts[i].sgl,
> - bo->sgts[i].nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(pfdev->dev, &bo->sgts[i],
> + DMA_BIDIRECTIONAL);
> sg_free_table(&bo->sgts[i]);
> }
> }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ed28aeb..9926111 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -517,10 +517,9 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> if (ret)
> goto err_pages;
>
> - if (!dma_map_sg(pfdev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL)) {
> - ret = -EINVAL;
> + ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL);
> + if (ret)
> goto err_map;
> - }
>
> mmu_map_sg(pfdev, bomapping->mmu, addr,
> IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
>

2020-05-12 00:22:55

by Roland Scheidegger

[permalink] [raw]

Subject: Re: [Linux-graphics-maintainer] [PATCH v3 15/25] drm: vmwgfx: fix common struct sg_table related issues

I'm not exactly an expert on this, but looks alright to me.
Acked-by: Roland Scheidegger <[email protected]>

Am 05.05.20 um 10:46 schrieb Marek Szyprowski:
> The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
> numer of the created entries in the DMA address space. However the
> subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
> called with the original number of the entries passed to dma_map_sg. The
> sg_table->nents in turn holds the result of the dma_map_sg call as stated
> in include/linux/scatterlist.h. A common mistake was to ignore a result
> of the dma_map_sg function and don't use the sg_table->orig_nents at all.
>
> To avoid such issues, lets use common dma-mapping wrappers operating
> directly on the struct sg_table objects and adjust references to the
> nents and orig_nents respectively.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> For more information, see '[PATCH v3 00/25] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread: https://lkml.org/lkml/2020/5/5/187
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index bf0bc46..e50ae8b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -362,8 +362,7 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt)
> {
> struct device *dev = vmw_tt->dev_priv->dev->dev;
>
> - dma_unmap_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.nents,
> - DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL);
> vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents;
> }
>
> @@ -383,16 +382,8 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt)
> static int vmw_ttm_map_for_dma(struct vmw_ttm_tt *vmw_tt)
> {
> struct device *dev = vmw_tt->dev_priv->dev->dev;
> - int ret;
> -
> - ret = dma_map_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.orig_nents,
> - DMA_BIDIRECTIONAL);
> - if (unlikely(ret == 0))
> - return -ENOMEM;
>
> - vmw_tt->sgt.nents = ret;
> -
> - return 0;
> + return dma_map_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL);
> }
>
> /**
> @@ -449,10 +440,10 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
> if (unlikely(ret != 0))
> goto out_sg_alloc_fail;
>
> - if (vsgt->num_pages > vmw_tt->sgt.nents) {
> + if (vsgt->num_pages > vmw_tt->sgt.orig_nents) {
> uint64_t over_alloc =
> sgl_size * (vsgt->num_pages -
> - vmw_tt->sgt.nents);
> + vmw_tt->sgt.orig_nents);
>
> ttm_mem_global_free(glob, over_alloc);
> vmw_tt->sg_alloc_size -= over_alloc;
>