Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Device memory mappings are leaked when drm_gpusvm_dma_map_pages() fails midway. -- commit 9ac7ddc26000d29a77c97e82c04194e8d027c3ec Author: Honglei Huang <[email protected]> drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages This patch updates drm_gpusvm_get_pages() to take a contiguous array of drm_gpusvm_pages plus a count. It performs the HMM fault once for the CPU range and then DMA maps each instance per owning drm_device. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c [ ... ] > @@ -1727,15 +1765,30 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > goto retry; > } > > - err = drm_gpusvm_dma_map_pages(gpusvm, svm_pages, pfns, npages, ctx, > - dma_dir); > - drm_gpusvm_notifier_unlock(gpusvm); > - if (err) > - goto err_free; > + for (p = 0; p < num_pages; ++p) { > + if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p])) > + continue; > + > + err = drm_gpusvm_dma_map_pages(gpusvm, &svm_pages[p], pfns, > + npages, ctx, dma_dir); [Severity: High] This is a pre-existing issue, but I noticed a potential leak in how drm_gpusvm_dma_map_pages() handles its error path. If drm_gpusvm_dma_map_pages() maps one or more device pages successfully but a subsequent page fails (for example, yielding -EOPNOTSUPP due to mixed memory, or -EFAULT from dma_mapping_error), it jumps to err_unmap: drm_gpusvm_dma_map_pages() { ... err_unmap: svm_pages->flags.has_dma_mapping = true; __drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped); return err; } The rollback calls __drm_gpusvm_unmap_pages(), which relies on svm_pages->dpagemap to invoke device_unmap: __drm_gpusvm_unmap_pages() { ... else if (dpagemap && dpagemap->ops->device_unmap) dpagemap->ops->device_unmap(dpagemap, dev, addr); ... } However, svm_pages->dpagemap is only updated at the very end of drm_gpusvm_dma_map_pages() upon success. During the error path, it is still NULL. Can this result in __drm_gpusvm_unmap_pages() silently skipping the unmapping of all previously mapped device pages, thereby leaking device mapping resources? > + if (err) { > + /* > + * drm_gpusvm_dma_map_pages() already cleaned up the > + * instance that failed. Leave the earlier ones mapped: > + * on -EAGAIN the retry reuses them, on other errors the > + * driver frees them with the range. They may also be > + * used by other drm_devices, so do not unmap them here. > + */ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
