| Message ID | 20200826063316.23486-30-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM: fix struct sg_table nents vs. orig_nents misuse | expand |
On 2020-08-26 07:33, Marek Szyprowski wrote: > 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. Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/rapidio/devices/rio_mport_cdev.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c > index a30342942e26..89eb3d212652 100644 > --- a/drivers/rapidio/devices/rio_mport_cdev.c > +++ b/drivers/rapidio/devices/rio_mport_cdev.c > @@ -573,8 +573,7 @@ static void dma_req_free(struct kref *ref) > refcount); > struct mport_cdev_priv *priv = req->priv; > > - dma_unmap_sg(req->dmach->device->dev, > - req->sgt.sgl, req->sgt.nents, req->dir); > + dma_unmap_sgtable(req->dmach->device->dev, &req->sgt, req->dir, 0); > sg_free_table(&req->sgt); > if (req->page_list) { > unpin_user_pages(req->page_list, req->nr_pages); > @@ -814,7 +813,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, > struct mport_dev *md = priv->md; > struct dma_chan *chan; > int ret; > - int nents; > > if (xfer->length == 0) > return -EINVAL; > @@ -930,15 +928,14 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, > xfer->offset, xfer->length); > } > > - nents = dma_map_sg(chan->device->dev, > - req->sgt.sgl, req->sgt.nents, dir); > - if (nents == 0) { > + ret = dma_map_sgtable(chan->device->dev, &req->sgt, dir, 0); > + if (ret) { > rmcd_error("Failed to map SG list"); > ret = -EFAULT; > goto err_pg; > } > > - ret = do_dma_request(req, xfer, sync, nents); > + ret = do_dma_request(req, xfer, sync, req->sgt.nents); > > if (ret >= 0) { > if (sync == RIO_TRANSFER_ASYNC) >
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c index a30342942e26..89eb3d212652 100644 --- a/drivers/rapidio/devices/rio_mport_cdev.c +++ b/drivers/rapidio/devices/rio_mport_cdev.c @@ -573,8 +573,7 @@ static void dma_req_free(struct kref *ref) refcount); struct mport_cdev_priv *priv = req->priv; - dma_unmap_sg(req->dmach->device->dev, - req->sgt.sgl, req->sgt.nents, req->dir); + dma_unmap_sgtable(req->dmach->device->dev, &req->sgt, req->dir, 0); sg_free_table(&req->sgt); if (req->page_list) { unpin_user_pages(req->page_list, req->nr_pages); @@ -814,7 +813,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, struct mport_dev *md = priv->md; struct dma_chan *chan; int ret; - int nents; if (xfer->length == 0) return -EINVAL; @@ -930,15 +928,14 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, xfer->offset, xfer->length); } - nents = dma_map_sg(chan->device->dev, - req->sgt.sgl, req->sgt.nents, dir); - if (nents == 0) { + ret = dma_map_sgtable(chan->device->dev, &req->sgt, dir, 0); + if (ret) { rmcd_error("Failed to map SG list"); ret = -EFAULT; goto err_pg; } - ret = do_dma_request(req, xfer, sync, nents); + ret = do_dma_request(req, xfer, sync, req->sgt.nents); if (ret >= 0) { if (sync == RIO_TRANSFER_ASYNC)
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> --- drivers/rapidio/devices/rio_mport_cdev.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)