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@xxxxxxxxxxx> >> --- >> 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@xxxxxxxxxxx/T/ >> --- >> 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@xxxxxx> > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
