Hi Christoph,

On 12.05.2020 14:18, Christoph Hellwig wrote:

On Tue, May 12, 2020 at 11:00:21AM +0200, 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 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 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 m.szyprowski@samsung.com

For more information, see '[PATCH v4 00/38] DRM: fix struct sg_table nents vs. orig_nents misuse' thread: https://lore.kernel.org/dri-devel/20200512085710.14688-1-m.szyprowski@samsun...

---

include/linux/dma-mapping.h | 79 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)

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

+/**

• • dma_map_sgtable - Map the given buffer for the DMA operations
• • @dev: The device to perform a DMA operation
• • @sgt: The sg_table object describing the buffer
• • @dir: DMA direction
• • @attrs: Optional DMA attributes for the map operation
• • Maps a buffer described by a scatterlist stored in the given sg_table
• • object for the @dir DMA operation by the @dev device. After success
• • the ownership for the buffer is transferred to the DMA domain. One has
• • to call dma_sync_sgtable_for_cpu() or dma_unmap_sgtable() to move the
• • ownership of the buffer back to the CPU domain before touching the
• • buffer by the CPU.
• • Returns 0 on success or -EINVAL on error during mapping the buffer.
• */

+static inline int dma_map_sgtable(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;

Nit: code tend to be a tad easier to read if the the exceptional condition is inside the branch block, so:

if (n <= 0) return -EINVAL; sgt->nents = n; return 0;

Indeed this version looks much better. I will resend it in a few minutes.

Otherwise this looks good to me:

Reviewed-by: Christoph Hellwig hch@lst.de

Best regards