- Linaro-mm-sig - lists.linaro.org

[PATCH 3/4] drm/virtio: move mapping teardown to virtio_gpu_cleanup_object()

by Gerd Hoffmann

Stop sending DETACH_BACKING commands, that will happening anyway when releasing resources via UNREF. Handle guest-side cleanup in virtio_gpu_cleanup_object(), called when the host finished processing the UNREF command. Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- drivers/gpu/drm/virtio/virtgpu_drv.h | 2 -- drivers/gpu/drm/virtio/virtgpu_object.c | 14 ++++++-- drivers/gpu/drm/virtio/virtgpu_vq.c | 46 ------------------------- 3 files changed, 12 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 372dd248cf02..15fb3c12f22f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -280,8 +280,6 @@ void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, struct virtio_gpu_object *obj, struct virtio_gpu_fence *fence); -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj); int virtio_gpu_attach_status_page(struct virtio_gpu_device *vgdev); int virtio_gpu_detach_status_page(struct virtio_gpu_device *vgdev); void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 28a161af7503..bce2b3d843fe 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -23,6 +23,7 @@ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/dma-mapping.h> #include <linux/moduleparam.h> #include "virtgpu_drv.h" @@ -65,6 +66,17 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo) { struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; + if (bo->pages) { + if (bo->mapped) { + dma_unmap_sg(vgdev->vdev->dev.parent, + bo->pages->sgl, bo->mapped, + DMA_TO_DEVICE); + bo->mapped = 0; + } + sg_free_table(bo->pages); + bo->pages = NULL; + drm_gem_shmem_unpin(&bo->base.base); + } virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle); drm_gem_shmem_free_object(&bo->base.base); } @@ -74,8 +86,6 @@ static void virtio_gpu_free_object(struct drm_gem_object *obj) struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; - if (bo->pages) - virtio_gpu_object_detach(vgdev, bo); if (bo->created) { virtio_gpu_cmd_unref_resource(vgdev, bo); /* completion handler calls virtio_gpu_cleanup_object() */ diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index 6e8097e4c214..e258186bedb2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -538,22 +538,6 @@ void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev, virtio_gpu_queue_ctrl_buffer(vgdev, vbuf); } -static void virtio_gpu_cmd_resource_inval_backing(struct virtio_gpu_device *vgdev, - uint32_t resource_id, - struct virtio_gpu_fence *fence) -{ - struct virtio_gpu_resource_detach_backing *cmd_p; - struct virtio_gpu_vbuffer *vbuf; - - cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p)); - memset(cmd_p, 0, sizeof(*cmd_p)); - - cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING); - cmd_p->resource_id = cpu_to_le32(resource_id); - - virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence); -} - void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, uint32_t scanout_id, uint32_t resource_id, uint32_t width, uint32_t height, @@ -1148,36 +1132,6 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, return 0; } -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj) -{ - bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev); - - if (WARN_ON_ONCE(!obj->pages)) - return; - - if (use_dma_api && obj->mapped) { - struct virtio_gpu_fence *fence = virtio_gpu_fence_alloc(vgdev); - /* detach backing and wait for the host process it ... */ - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, fence); - dma_fence_wait(&fence->f, true); - dma_fence_put(&fence->f); - - /* ... then tear down iommu mappings */ - dma_unmap_sg(vgdev->vdev->dev.parent, - obj->pages->sgl, obj->mapped, - DMA_TO_DEVICE); - obj->mapped = 0; - } else { - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, NULL); - } - - sg_free_table(obj->pages); - obj->pages = NULL; - - drm_gem_shmem_unpin(&obj->base.base); -} - void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, struct virtio_gpu_output *output) { -- 2.18.1

5 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH -next] drm/panfrost: Remove set but not used variable 'bo'

by Steven Price

On Mon, Feb 03, 2020 at 03:27:24PM +0000, YueHaibing wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/gpu/drm/panfrost/panfrost_job.c: In function 'panfrost_job_cleanup': > drivers/gpu/drm/panfrost/panfrost_job.c:278:31: warning: > variable 'bo' set but not used [-Wunused-but-set-variable] > > commit bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept") > involved this unused variable. > > Reported-by: Hulk Robot <hulkci(a)huawei.com> > Signed-off-by: YueHaibing <yuehaibing(a)huawei.com> I'm not sure how I didn't spot that before! Thanks for fixing it. Note commit bdefca2d8dc0 is actually in v5.5 and not (yet) in drm-misc-next. Reviewed-by: Steven Price <steven.price(a)arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_job.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 7c36ec675b73..ccb8546a9342 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -275,12 +275,8 @@ static void panfrost_job_cleanup(struct kref *ref) > } > > if (job->bos) { > - struct panfrost_gem_object *bo; > - > - for (i = 0; i < job->bo_count; i++) { > - bo = to_panfrost_bo(job->bos[i]); > + for (i = 0; i < job->bo_count; i++) > drm_gem_object_put_unlocked(job->bos[i]); > - } > > kvfree(job->bos); > } > > >

5 years, 10 months

1
0
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Daniel Vetter

On Fri, Jan 31, 2020 at 11:28 PM syzbot <syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com> wrote: > > Hello, > > syzbot found the following crash on: > > HEAD commit: 39bed42d Merge tag 'for-linus-hmm' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=179465bee00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=2646535f8818ae25 > dashboard link: https://syzkaller.appspot.com/bug?extid=0dc4444774d419e916c8 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16251279e00000 > > The bug was bisected to: > > commit 7611750784664db46d0db95631e322aeb263dde7 > Author: Alex Deucher <alexander.deucher(a)amd.com> > Date: Wed Jun 21 16:31:41 2017 +0000 > > drm/amdgpu: use kernel is_power_of_2 rather than local version > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11628df1e00000 > final crash: https://syzkaller.appspot.com/x/report.txt?x=13628df1e00000 > console output: https://syzkaller.appspot.com/x/log.txt?x=15628df1e00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com > Fixes: 761175078466 ("drm/amdgpu: use kernel is_power_of_2 rather than local version") Aside: This bisect line is complete nonsense ... I'm kinda at the point where I'm assuming that syzbot bisect results are garbage, which is maybe not what we want. I guess much stricter filtering for noise is needed, dunno. -Danile > > ================================================================== > BUG: KASAN: use-after-free in vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > Read of size 8 at addr ffff88809fa67908 by task syz-executor.0/14871 > > CPU: 0 PID: 14871 Comm: syz-executor.0 Not tainted 5.5.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x197/0x210 lib/dump_stack.c:118 > print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374 > __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506 > kasan_report+0x12/0x20 mm/kasan/common.c:639 > __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 > vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x45b349 > Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f871af46c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007f871af476d4 RCX: 000000000045b349 > RDX: 0000000020000180 RSI: 00000000c02064b2 RDI: 0000000000000003 > RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff > R13: 0000000000000285 R14: 00000000004d14d0 R15: 000000000075bf2c > > Allocated by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > __kasan_kmalloc mm/kasan/common.c:513 [inline] > __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:486 > kasan_kmalloc+0x9/0x10 mm/kasan/common.c:527 > kmem_cache_alloc_trace+0x158/0x790 mm/slab.c:3551 > kmalloc include/linux/slab.h:556 [inline] > kzalloc include/linux/slab.h:670 [inline] > __vgem_gem_create+0x49/0x100 drivers/gpu/drm/vgem/vgem_drv.c:165 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:194 [inline] > vgem_gem_dumb_create+0xd7/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Freed by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > kasan_set_free_info mm/kasan/common.c:335 [inline] > __kasan_slab_free+0x102/0x150 mm/kasan/common.c:474 > kasan_slab_free+0xe/0x10 mm/kasan/common.c:483 > __cache_free mm/slab.c:3426 [inline] > kfree+0x10a/0x2c0 mm/slab.c:3757 > vgem_gem_free_object+0xbe/0xe0 drivers/gpu/drm/vgem/vgem_drv.c:68 > drm_gem_object_free+0x100/0x220 drivers/gpu/drm/drm_gem.c:983 > kref_put include/linux/kref.h:65 [inline] > drm_gem_object_put_unlocked drivers/gpu/drm/drm_gem.c:1017 [inline] > drm_gem_object_put_unlocked+0x196/0x1c0 drivers/gpu/drm/drm_gem.c:1002 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:199 [inline] > vgem_gem_dumb_create+0x115/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > The buggy address belongs to the object at ffff88809fa67800 > which belongs to the cache kmalloc-1k of size 1024 > The buggy address is located 264 bytes inside of > 1024-byte region [ffff88809fa67800, ffff88809fa67c00) > The buggy address belongs to the page: > page:ffffea00027e99c0 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 > raw: 00fffe0000000200 ffffea0002293548 ffffea00023e1f08 ffff8880aa400c40 > raw: 0000000000000000 ffff88809fa67000 0000000100000002 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff88809fa67800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > >ffff88809fa67900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff88809fa67980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== > > > --- > This bug is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller(a)googlegroups.com. > > syzbot will keep track of this bug report. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > For information about bisection process see: https://goo.gl/tpsmEJ#bisection > syzbot can test patches for this bug, for details see: > https://goo.gl/tpsmEJ#testing-patches -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years, 10 months

3
2
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Hillf Danton

Fri, 31 Jan 2020 14:28:10 -0800 (PST) > syzbot found the following crash on: > > HEAD commit: 39bed42d Merge tag 'for-linus-hmm' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=179465bee00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=2646535f8818ae25 > dashboard link: https://syzkaller.appspot.com/bug?extid=0dc4444774d419e916c8 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16251279e00000 > > The bug was bisected to: > > commit 7611750784664db46d0db95631e322aeb263dde7 > Author: Alex Deucher <alexander.deucher(a)amd.com> > Date: Wed Jun 21 16:31:41 2017 +0000 > > drm/amdgpu: use kernel is_power_of_2 rather than local version > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11628df1e00000 > final crash: https://syzkaller.appspot.com/x/report.txt?x=13628df1e00000 > console output: https://syzkaller.appspot.com/x/log.txt?x=15628df1e00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com > Fixes: 761175078466 ("drm/amdgpu: use kernel is_power_of_2 rather than local version") > > ================================================================== > BUG: KASAN: use-after-free in vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > Read of size 8 at addr ffff88809fa67908 by task syz-executor.0/14871 > > CPU: 0 PID: 14871 Comm: syz-executor.0 Not tainted 5.5.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x197/0x210 lib/dump_stack.c:118 > print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374 > __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506 > kasan_report+0x12/0x20 mm/kasan/common.c:639 > __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 > vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x45b349 > Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f871af46c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007f871af476d4 RCX: 000000000045b349 > RDX: 0000000020000180 RSI: 00000000c02064b2 RDI: 0000000000000003 > RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff > R13: 0000000000000285 R14: 00000000004d14d0 R15: 000000000075bf2c > > Allocated by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > __kasan_kmalloc mm/kasan/common.c:513 [inline] > __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:486 > kasan_kmalloc+0x9/0x10 mm/kasan/common.c:527 > kmem_cache_alloc_trace+0x158/0x790 mm/slab.c:3551 > kmalloc include/linux/slab.h:556 [inline] > kzalloc include/linux/slab.h:670 [inline] > __vgem_gem_create+0x49/0x100 drivers/gpu/drm/vgem/vgem_drv.c:165 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:194 [inline] > vgem_gem_dumb_create+0xd7/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Freed by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > kasan_set_free_info mm/kasan/common.c:335 [inline] > __kasan_slab_free+0x102/0x150 mm/kasan/common.c:474 > kasan_slab_free+0xe/0x10 mm/kasan/common.c:483 > __cache_free mm/slab.c:3426 [inline] > kfree+0x10a/0x2c0 mm/slab.c:3757 > vgem_gem_free_object+0xbe/0xe0 drivers/gpu/drm/vgem/vgem_drv.c:68 > drm_gem_object_free+0x100/0x220 drivers/gpu/drm/drm_gem.c:983 > kref_put include/linux/kref.h:65 [inline] > drm_gem_object_put_unlocked drivers/gpu/drm/drm_gem.c:1017 [inline] > drm_gem_object_put_unlocked+0x196/0x1c0 drivers/gpu/drm/drm_gem.c:1002 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:199 [inline] > vgem_gem_dumb_create+0x115/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > The buggy address belongs to the object at ffff88809fa67800 > which belongs to the cache kmalloc-1k of size 1024 > The buggy address is located 264 bytes inside of > 1024-byte region [ffff88809fa67800, ffff88809fa67c00) > The buggy address belongs to the page: > page:ffffea00027e99c0 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 > raw: 00fffe0000000200 ffffea0002293548 ffffea00023e1f08 ffff8880aa400c40 > raw: 0000000000000000 ffff88809fa67000 0000000100000002 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff88809fa67800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > >ffff88809fa67900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff88809fa67980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== Release obj in error path. --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -196,10 +196,10 @@ static struct drm_gem_object *vgem_gem_c return ERR_CAST(obj); ret = drm_gem_handle_create(file, &obj->base, handle); - drm_gem_object_put_unlocked(&obj->base); - if (ret) + if (ret) { + drm_gem_object_put_unlocked(&obj->base); return ERR_PTR(ret); - + } return &obj->base; }

5 years, 10 months

3
4
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Dan Carpenter

I don't totally understand the stack trace but I do see a double free bug. drivers/gpu/drm/vgem/vgem_drv.c 186 static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, 187 struct drm_file *file, 188 unsigned int *handle, 189 unsigned long size) 190 { 191 struct drm_vgem_gem_object *obj; 192 int ret; 193 194 obj = __vgem_gem_create(dev, size); obj->base.handle_count is zero. 195 if (IS_ERR(obj)) 196 return ERR_CAST(obj); 197 198 ret = drm_gem_handle_create(file, &obj->base, handle); We bump it +1 and then the error handling calls drm_gem_object_handle_put_unlocked(obj); which calls drm_gem_object_put_unlocked(); which frees obj. 199 drm_gem_object_put_unlocked(&obj->base); So this is a double free. Could someone check my thinking and send a patch? It's just a one liner. Otherwise I can send it on Monday. 200 if (ret) 201 return ERR_PTR(ret); 202 203 return &obj->base; 204 } regards, dan carpenter

5 years, 10 months

1
0
0 0

[PATCH] dma-buf: fix locking in sync_print_obj()

by Dan Carpenter

This is always called with IRQs disabled and we don't actually want to enable IRQs at the end. Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com> --- drivers/dma-buf/sync_debug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 101394f16930..952331344b1c 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) { struct list_head *pos; + unsigned long flags; seq_printf(s, "%s: %d\n", obj->name, obj->value); - spin_lock_irq(&obj->lock); + spin_lock_irqsave(&obj->lock, flags); list_for_each(pos, &obj->pt_list) { struct sync_pt *pt = container_of(pos, struct sync_pt, link); sync_print_fence(s, &pt->base, false); } - spin_unlock_irq(&obj->lock); + spin_unlock_irqrestore(&obj->lock, flags); } static void sync_print_sync_file(struct seq_file *s, -- 2.11.0

5 years, 10 months

2
3
0 0

[PATCH] dma-heap: Make the symbol 'dma_heap_ioctl_cmds' static

by Sumit Semwal

From: zhong jiang <zhongjiang(a)huawei.com> Fix the following sparse warning. drivers/dma-buf/dma-heap.c:109:14: warning: symbol 'dma_heap_ioctl_cmds' was not declared. Should it be static? Acked-by: Andrew F. Davis <afd(a)ti.com> Acked-by: John Stultz <john.stultz(a)linaro.org> Signed-off-by: zhong jiang <zhongjiang(a)huawei.com> Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org> [sumits: rebased over IOCTL rename patches] --- drivers/dma-buf/dma-heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 1886aee46131..afd22c9dbdcf 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -106,7 +106,7 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data) return 0; } -unsigned int dma_heap_ioctl_cmds[] = { +static unsigned int dma_heap_ioctl_cmds[] = { DMA_HEAP_IOCTL_ALLOC, }; -- 2.18.0

5 years, 12 months

1
0
0 0

[PATCH][next] dma-buf: fix resource leak on -ENOTTY error return path

by Colin King

From: Colin Ian King <colin.king(a)canonical.com> The -ENOTTY error return path does not free the allocated kdata as it returns directly. Fix this by returning via the error handling label err. Addresses-Coverity: ("Resource leak") Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- drivers/dma-buf/dma-heap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 4f04d104ae61..80f2f5eac1e4 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -157,7 +157,8 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, ret = dma_heap_ioctl_allocate(file, kdata); break; default: - return -ENOTTY; + ret = -ENOTTY; + goto err; } if (copy_to_user((void __user *)arg, kdata, out_size) != 0) -- 2.24.0

6 years

2
1
0 0

[PATCH] MAINTAINERS: Match on dma_buf|fence|resv anywhere

by Daniel Vetter

I've spent a bit too much time reviewing all kinds of users all over the kernel for this buffer sharing infrastructure. And some of it is at least questionable. Make sure we at least see when this stuff flies by. Acked-by: Dave Airlie <airlied(a)gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Mauro Carvalho Chehab <mchehab+samsung(a)kernel.org> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: Rob Herring <robh(a)kernel.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 375dbea8bc88..c1e3da2c1947 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4941,6 +4941,7 @@ F: include/linux/dma-buf* F: include/linux/reservation.h F: include/linux/*fence.h F: Documentation/driver-api/dma-buf.rst +K: dma_(buf|fence|resv) T: git git://anongit.freedesktop.org/drm/drm-misc DMA GENERIC OFFLOAD ENGINE SUBSYSTEM -- 2.24.0

6 years

4
3
0 0

[PATCH] udmabuf: Remove deleted map/unmap handlers.

by Maarten Lankhorst

Commit 7f0de8d80816 ("dma-buf: Drop dma_buf_k(un)map") removed map/unmap handlers, but they still existed in udmabuf. Remove them there as well Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Fixes: 7f0de8d80816 ("dma-buf: Drop dma_buf_k(un)map") Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Daniel Vetter <daniel.vetter(a)intel.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: dri-devel(a)lists.freedesktop.org --- drivers/dma-buf/udmabuf.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 9635897458a0..9de539c1def4 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -93,26 +93,10 @@ static void release_udmabuf(struct dma_buf *buf) kfree(ubuf); } -static void *kmap_udmabuf(struct dma_buf *buf, unsigned long page_num) -{ - struct udmabuf *ubuf = buf->priv; - struct page *page = ubuf->pages[page_num]; - - return kmap(page); -} - -static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, - void *vaddr) -{ - kunmap(vaddr); -} - static const struct dma_buf_ops udmabuf_ops = { .map_dma_buf = map_udmabuf, .unmap_dma_buf = unmap_udmabuf, .release = release_udmabuf, - .map = kmap_udmabuf, - .unmap = kunmap_udmabuf, .mmap = mmap_udmabuf, }; -- 2.24.0

6 years

2
2
0 0

[PATCH 15/15] dma-buf: Remove kernel map/unmap hooks

by Daniel Vetter

All implementations are gone now. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- include/linux/dma-buf.h | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 7feb9c3805ae..abf5459a5b9d 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -249,31 +249,6 @@ struct dma_buf_ops { */ int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); - /** - * @map: - * - * Maps a page from the buffer into kernel address space. The page is - * specified by offset into the buffer in PAGE_SIZE units. - * - * This callback is optional. - * - * Returns: - * - * Virtual address pointer where requested page can be accessed. NULL - * on error or when this function is unimplemented by the exporter. - */ - void *(*map)(struct dma_buf *, unsigned long); - - /** - * @unmap: - * - * Unmaps a page from the buffer. Page offset and address pointer should - * be the same as the one passed to and returned by matching call to map. - * - * This callback is optional. - */ - void (*unmap)(struct dma_buf *, unsigned long, void *); - void *(*vmap)(struct dma_buf *); void (*vunmap)(struct dma_buf *, void *vaddr); }; -- 2.24.0

6 years

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix memory leak in sync_file_merge()

by Daniel Vetter

On Fri, Nov 22, 2019 at 04:09:55PM -0600, Navid Emamdoost wrote: > In the implementation of sync_file_merge() the allocated sync_file is > leaked if number of fences overflows. Release sync_file by goto err. > > Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") > Signed-off-by: Navid Emamdoost <navid.emamdoost(a)gmail.com> Thanks for your patch, applied with a cc:stable. -Daniel > --- > drivers/dma-buf/sync_file.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 25c5c071645b..91185db9a952 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -221,7 +221,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > a_fences = get_fences(a, &a_num_fences); > b_fences = get_fences(b, &b_num_fences); > if (a_num_fences > INT_MAX - b_num_fences) > - return NULL; > + goto err; > > num_fences = a_num_fences + b_num_fences; > > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

6 years

1
0
0 0

Re: [Linaro-mm-sig] 2989f64510 ("dma-buf: Add selftests for dma-fence"): WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:524 __debug_object_init

by Chris Wilson

Quoting kernel test robot (2019-11-21 07:19:43) > Greetings, > > 0day kernel testing robot got the below dmesg and the first bad commit is > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > commit 2989f6451084aed3f8cc9992477f7a9bf57a3716 > Author: Chris Wilson <chris(a)chris-wilson.co.uk> > AuthorDate: Mon Aug 19 10:59:27 2019 +0100 > Commit: Chris Wilson <chris(a)chris-wilson.co.uk> > CommitDate: Mon Aug 19 18:09:46 2019 +0100 That's a belated report, fixed by commit 6ac3a0ebfcc2f0c75ca0ca6974389ce421aa5cbd Author: Chris Wilson <chris(a)chris-wilson.co.uk> Date: Tue Aug 20 13:21:18 2019 +0100 dmabuf: Mark up onstack timer for selftests No? -Chris

6 years

1
0
0 0

[PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep

by Daniel Vetter

Semnatically it really doesn't matter where we grab the ticket. But since the ticket is a fake lockdep lock, it matters for lockdep validation purposes. This means stuff like grabbing a ticket and then doing copy_from/to_user isn't allowed anymore. This is a changed compared to the current ttm fault handler, which doesn't bother with having a full reservation. Since I'm looking into fixing the TODO entry in ttm_mem_evict_wait_busy() I think that'll have to change sooner or later anyway, better get started. A bit more context on why I'm looking into this: For backwards compat with existing i915 gem code I think we'll have to do full slowpath locking in the i915 equivalent of the eviction code. And with dynamic dma-buf that will leak across drivers, so another thing we need to standardize and make sure it's done the same way everyway. Unfortunately this means another full audit of all drivers: - gem helpers: acquire_init is done right before taking locks, so no problem. Same for acquire_fini and unlocking, which means nothing that's not already covered by the dma_resv_lock rules will be caught with this extension here to the acquire_ctx. - etnaviv: An absolute massive amount of code is run between the acquire_init and the first lock acquisition in submit_lock_objects. But nothing that would touch user memory and could cause a fault. Furthermore nothing that uses the ticket, so even if I missed something, it would be easy to fix by pushing the acquire_init right before the first use. Similar on the unlock/acquire_fini side. - i915: Right now (and this will likely change a lot rsn) the acquire ctx and actual locks are right next to each another. No problem. - msm has a problem: submit_create calls acquire_init, but then submit_lookup_objects() has a bunch of copy_from_user to do the object lookups. That's the only thing before submit_lock_objects call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv does not have this issue since it copies all the userspace structs earlier. submit_cleanup does not have any such issues. With the prep patch to pull out the acquire_ctx and reorder it msm is going to be safe too. - nouveau: acquire_init is right next to ttm_bo_reserve, so all good. Similar on the acquire_fini/ttm_bo_unreserve side. - ttm execbuf utils: acquire context and locking are even in the same functions here (one function to reserve everything, the other to unreserve), so all good. - vc4: Another case where acquire context and locking are handled in the same functions (one function to lock everything, the other to unlock). Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: Huang Rui <ray.huang(a)amd.com> Cc: Eric Anholt <eric(a)anholt.net> Cc: Ben Skeggs <bskeggs(a)redhat.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Rob Herring <robh(a)kernel.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Sean Paul <sean(a)poorly.run> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/dma-buf/dma-resv.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index d3c760e19991..079e38fde33a 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list) static void __init dma_resv_lockdep(void) { struct mm_struct *mm = mm_alloc(); + struct ww_acquire_ctx ctx; struct dma_resv obj; + int ret; if (!mm) return; @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void) dma_resv_init(&obj); down_read(&mm->mmap_sem); - ww_mutex_lock(&obj.lock, NULL); + ww_acquire_init(&ctx, &reservation_ww_class); + ret = dma_resv_lock(&obj, &ctx); + if (ret == -EDEADLK) + dma_resv_lock_slow(&obj, &ctx); fs_reclaim_acquire(GFP_KERNEL); fs_reclaim_release(GFP_KERNEL); ww_mutex_unlock(&obj.lock); + ww_acquire_fini(&ctx); up_read(&mm->mmap_sem); mmput(mm); -- 2.24.0

6 years

3
2
0 0

Re: [Linaro-mm-sig] [PATCH 15/15] dma-buf: Remove kernel map/unmap hooks

by Daniel Vetter

On Mon, Nov 18, 2019 at 4:23 PM kbuild test robot <lkp(a)intel.com> wrote: > > Hi Daniel, > > I love your patch! Yet something to improve: > > [auto build test ERROR on drm-intel/for-linux-next] > [also build test ERROR on v5.4-rc8 next-20191115] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] Too old tree, on latest drm-tip this compiles since udl has lots its own dma-buf implementation. Also, the patch set will start to compile once linux-next is open for 5.6. Cheers, Daniel > > url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/Retire-dma_buf_k-un-… > base: git://anongit.freedesktop.org/drm-intel for-linux-next > config: m68k-allmodconfig (attached as .config) > compiler: m68k-linux-gcc (GCC) 7.4.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.4.0 make.cross ARCH=m68k > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp(a)intel.com> > > All errors (new ones prefixed by >>): > > >> drivers/dma-buf/udmabuf.c:114:3: error: 'const struct dma_buf_ops' has no member named 'map'; did you mean 'mmap'? > .map = kmap_udmabuf, > ^~~ > mmap > >> drivers/dma-buf/udmabuf.c:114:12: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] > .map = kmap_udmabuf, > ^~~~~~~~~~~~ > drivers/dma-buf/udmabuf.c:114:12: note: (near initialization for 'udmabuf_ops.begin_cpu_access') > >> drivers/dma-buf/udmabuf.c:115:3: error: 'const struct dma_buf_ops' has no member named 'unmap'; did you mean 'vunmap'? > .unmap = kunmap_udmabuf, > ^~~~~ > vunmap > drivers/dma-buf/udmabuf.c:115:14: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] > .unmap = kunmap_udmabuf, > ^~~~~~~~~~~~~~ > drivers/dma-buf/udmabuf.c:115:14: note: (near initialization for 'udmabuf_ops.end_cpu_access') > cc1: some warnings being treated as errors > -- > >> drivers/gpu/drm/udl/udl_dmabuf.c:169:3: error: 'const struct dma_buf_ops' has no member named 'map'; did you mean 'mmap'? > .map = udl_dmabuf_kmap, > ^~~ > mmap > >> drivers/gpu/drm/udl/udl_dmabuf.c:169:11: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] > .map = udl_dmabuf_kmap, > ^~~~~~~~~~~~~~~ > drivers/gpu/drm/udl/udl_dmabuf.c:169:11: note: (near initialization for 'udl_dmabuf_ops.release') > >> drivers/gpu/drm/udl/udl_dmabuf.c:170:3: error: 'const struct dma_buf_ops' has no member named 'unmap'; did you mean 'vunmap'? > .unmap = udl_dmabuf_kunmap, > ^~~~~ > vunmap > drivers/gpu/drm/udl/udl_dmabuf.c:170:13: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] > .unmap = udl_dmabuf_kunmap, > ^~~~~~~~~~~~~~~~~ > drivers/gpu/drm/udl/udl_dmabuf.c:170:13: note: (near initialization for 'udl_dmabuf_ops.begin_cpu_access') > cc1: some warnings being treated as errors > > vim +114 drivers/dma-buf/udmabuf.c > > fbb0de79507819 Gerd Hoffmann 2018-08-27 109 > a34852891ba45d Gerd Hoffmann 2018-09-11 110 static const struct dma_buf_ops udmabuf_ops = { > fbb0de79507819 Gerd Hoffmann 2018-08-27 111 .map_dma_buf = map_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 112 .unmap_dma_buf = unmap_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 113 .release = release_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 @114 .map = kmap_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 @115 .unmap = kunmap_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 116 .mmap = mmap_udmabuf, > fbb0de79507819 Gerd Hoffmann 2018-08-27 117 }; > fbb0de79507819 Gerd Hoffmann 2018-08-27 118 > > :::::: The code at line 114 was first introduced by commit > :::::: fbb0de795078190a9834b3409e4b009cfb18a6d4 Add udmabuf misc device > > :::::: TO: Gerd Hoffmann <kraxel(a)redhat.com> > :::::: CC: Gerd Hoffmann <kraxel(a)redhat.com> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

6 years

1
0
0 0

[PATCH 04/15] staging/android/ion: delete dma_buf->kmap/unmap implemenation

by Daniel Vetter

There's no callers in-tree anymore. For merging probably best to stuff this into drm-misc, since that's where the dma-buf heaps will land too. And the resulting conflict hopefully ensures that dma-buf heaps wont have a new ->kmap/unmap implemenation. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Laura Abbott <labbott(a)redhat.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: devel(a)driverdev.osuosl.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/staging/android/ion/ion.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index e6b1ca141b93..bb4ededc1150 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -274,18 +274,6 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) _ion_buffer_destroy(buffer); } -static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) -{ - struct ion_buffer *buffer = dmabuf->priv; - - return buffer->vaddr + offset * PAGE_SIZE; -} - -static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, - void *ptr) -{ -} - static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) { @@ -349,8 +337,6 @@ static const struct dma_buf_ops dma_buf_ops = { .detach = ion_dma_buf_detatch, .begin_cpu_access = ion_dma_buf_begin_cpu_access, .end_cpu_access = ion_dma_buf_end_cpu_access, - .map = ion_dma_buf_kmap, - .unmap = ion_dma_buf_kunmap, }; static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags) -- 2.24.0

6 years

2
1
0 0

[PATCH 09/15] dma-buf: Drop dma_buf_k(un)map

by Daniel Vetter

It's unused. 10 years ago, back when 32bit was still fairly common and trying to not exhaust vmalloc space sounded like a worthwhile goal, adding these to dma_buf made sense. Reality is that they simply never caught on, and nowadays everyone who needs plenty of buffers will run in 64bit mode anyway. Also update the docs in this area to adjust them to reality. The actual hooks in dma_buf_ops will be removed once all the implementations are gone. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-buf.c | 63 ++------------------------------------- include/linux/dma-buf.h | 2 -- 2 files changed, 3 insertions(+), 62 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d377b4ca66bf..97988ce1d2dc 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -880,29 +880,9 @@ EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); * with calls to dma_buf_begin_cpu_access() and dma_buf_end_cpu_access() * access. * - * To support dma_buf objects residing in highmem cpu access is page-based - * using an api similar to kmap. Accessing a dma_buf is done in aligned chunks - * of PAGE_SIZE size. Before accessing a chunk it needs to be mapped, which - * returns a pointer in kernel virtual address space. Afterwards the chunk - * needs to be unmapped again. There is no limit on how often a given chunk - * can be mapped and unmapped, i.e. the importer does not need to call - * begin_cpu_access again before mapping the same chunk again. - * - * Interfaces:: - * void \*dma_buf_kmap(struct dma_buf \*, unsigned long); - * void dma_buf_kunmap(struct dma_buf \*, unsigned long, void \*); - * - * Implementing the functions is optional for exporters and for importers all - * the restrictions of using kmap apply. - * - * dma_buf kmap calls outside of the range specified in begin_cpu_access are - * undefined. If the range is not PAGE_SIZE aligned, kmap needs to succeed on - * the partial chunks at the beginning and end but may return stale or bogus - * data outside of the range (in these partial chunks). - * - * For some cases the overhead of kmap can be too high, a vmap interface - * is introduced. This interface should be used very carefully, as vmalloc - * space is a limited resources on many architectures. + * Since for most kernel internal dma-buf accesses need the entire buffer, a + * vmap interface is introduced. Note that on very old 32-bit architectures + * vmalloc space might be limited and result in vmap calls failing. * * Interfaces:: * void \*dma_buf_vmap(struct dma_buf \*dmabuf) @@ -1052,43 +1032,6 @@ int dma_buf_end_cpu_access(struct dma_buf *dmabuf, } EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); -/** - * dma_buf_kmap - Map a page of the buffer object into kernel address space. The - * same restrictions as for kmap and friends apply. - * @dmabuf: [in] buffer to map page from. - * @page_num: [in] page in PAGE_SIZE units to map. - * - * This call must always succeed, any necessary preparations that might fail - * need to be done in begin_cpu_access. - */ -void *dma_buf_kmap(struct dma_buf *dmabuf, unsigned long page_num) -{ - WARN_ON(!dmabuf); - - if (!dmabuf->ops->map) - return NULL; - return dmabuf->ops->map(dmabuf, page_num); -} -EXPORT_SYMBOL_GPL(dma_buf_kmap); - -/** - * dma_buf_kunmap - Unmap a page obtained by dma_buf_kmap. - * @dmabuf: [in] buffer to unmap page from. - * @page_num: [in] page in PAGE_SIZE units to unmap. - * @vaddr: [in] kernel space pointer obtained from dma_buf_kmap. - * - * This call must always succeed. - */ -void dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long page_num, - void *vaddr) -{ - WARN_ON(!dmabuf); - - if (dmabuf->ops->unmap) - dmabuf->ops->unmap(dmabuf, page_num, vaddr); -} -EXPORT_SYMBOL_GPL(dma_buf_kunmap); - /** * dma_buf_mmap - Setup up a userspace mmap with the given vma diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index af73f835c51c..7feb9c3805ae 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -464,8 +464,6 @@ int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction dir); int dma_buf_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction dir); -void *dma_buf_kmap(struct dma_buf *, unsigned long); -void dma_buf_kunmap(struct dma_buf *, unsigned long, void *); int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, unsigned long); -- 2.24.0

6 years

1
0
0 0

[PATCH] dma-buf/sw-sync: Use unsigned type for value of sync_timeline

by Seung-Woo Kim

The value of sync_timeline is only incremented and all reference usage of it is unsigned. Use unsigned type for value of synctimeline. Signed-off-by: Seung-Woo Kim <sw0312.kim(a)samsung.com> --- drivers/dma-buf/sync_debug.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h index 6176e52..ff07f0b 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -36,7 +36,7 @@ struct sync_timeline { /* protected by lock */ u64 context; - int value; + unsigned int value; struct rb_root pt_tree; struct list_head pt_list; -- 1.7.4.1

6 years, 1 month

1
0
0 0

[bug report] dma-buf: heaps: Add heap helpers

by Dan Carpenter

Hello John Stultz, The patch 7b87ea704fd9: "dma-buf: heaps: Add heap helpers" from Oct 21, 2019, leads to the following static checker warning: drivers/dma-buf/heaps/heap-helpers.c:165 dma_heap_vm_fault() warn: uncapped user index 'buffer->pages[vmf->pgoff]' drivers/dma-buf/heaps/heap-helpers.c 160 static vm_fault_t dma_heap_vm_fault(struct vm_fault *vmf) 161 { 162 struct vm_area_struct *vma = vmf->vma; 163 struct heap_helper_buffer *buffer = vma->vm_private_data; 164 165 vmf->page = buffer->pages[vmf->pgoff]; ^^^^^^^^^^ Smatch for some reason thinks this needs to be checked. Smatch also gets confused by these fault handlers and thinks there is some recursion involved... 166 get_page(vmf->page); 167 168 return 0; 169 } 170 171 static const struct vm_operations_struct dma_heap_vm_ops = { 172 .fault = dma_heap_vm_fault, 173 }; 174 regards, dan carpenter

6 years, 1 month

2
2
0 0

Re: [Linaro-mm-sig] [PATCH][next] dma-buf: heaps: remove redundant assignment to variable ret

by John Stultz

On Wed, Oct 30, 2019 at 8:45 AM Andrew F. Davis <afd(a)ti.com> wrote: > > On 10/30/19 11:02 AM, Colin King wrote: > > From: Colin Ian King <colin.king(a)canonical.com> > > > > The variable ret is being assigned with a value that is never > > read, it is being re-assigned the same value on the err0 exit > > path. The assignment is redundant and hence can be removed. > > > > Addresses-Coverity: ("Unused value") > > Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") > > Signed-off-by: Colin Ian King <colin.king(a)canonical.com> > > --- > > > The root of the issue is that ret is not used in the error path, it > should be, I suggest this fix: > > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -98,7 +98,7 @@ static int system_heap_allocate(struct dma_heap *heap, > > err0: > > kfree(helper_buffer); > > > > - return -ENOMEM; > > + return ret; > > } Sounds good! If its ok I'll generate a commit crediting Colin for reporting the issue and Andrew for the fix and submit it to Sumit. Many thanks to you both! -john

6 years, 1 month

2
1
0 0

[PATCH] dma-buf: heaps: Fix redundant assignment to variable ret in system_heap.c

by John Stultz

Colin King reported a coverity error: The variable ret is being assigned with a value that is never read, it is being re-assigned the same value on the err0 exit path. The assignment is redundant and hence can be removed. He had a fix, but Andrew Davis suggested a better solution (actually returning ret), so this patch implements that fix. Cc: Colin King <colin.king(a)canonical.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Benjamin Gaignard <benjamin.gaignard(a)linaro.org> Cc: Liam Mark <lmark(a)codeaurora.org> Cc: Laura Abbott <labbott(a)redhat.com> Cc: Brian Starkey <brian.starkey(a)arm.com> Cc: Andrew F. Davis <afd(a)ti.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: kernel-janitors(a)vger.kernel.org Addresses-Coverity: ("Unused value") Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") Reported-by: Colin Ian King <colin.king(a)canonical.com> Suggested-by: Andrew F. Davis <afd(a)ti.com> Signed-off-by: John Stultz <john.stultz(a)linaro.org> --- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 455782efbb32..9a56393e40b4 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -98,7 +98,7 @@ static int system_heap_allocate(struct dma_heap *heap, err0: kfree(helper_buffer); - return -ENOMEM; + return ret; } static const struct dma_heap_ops system_heap_ops = { -- 2.17.1

6 years, 1 month

1
0
0 0

[PATCH] dma-buf: Fix a warning message in dma_heap_buffer_destroy()

by Dan Carpenter

The first argument of WARN() is a condition so this will just print the function name instead of the whole warning message. Fixes: 7b87ea704fd9 ("dma-buf: heaps: Add heap helpers") Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com> --- drivers/dma-buf/heaps/heap-helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/heaps/heap-helpers.c b/drivers/dma-buf/heaps/heap-helpers.c index 750bef4e902d..a31684c0d5b2 100644 --- a/drivers/dma-buf/heaps/heap-helpers.c +++ b/drivers/dma-buf/heaps/heap-helpers.c @@ -52,7 +52,7 @@ static void *dma_heap_map_kernel(struct heap_helper_buffer *buffer) static void dma_heap_buffer_destroy(struct heap_helper_buffer *buffer) { if (buffer->vmap_cnt > 0) { - WARN("%s: buffer still mapped in the kernel\n", __func__); + WARN(1, "%s: buffer still mapped in the kernel\n", __func__); vunmap(buffer->vaddr); } -- 2.20.1

6 years, 1 month

2
1
0 0

[PATCH][next] dma-buf: heaps: remove redundant assignment to variable ret

by Colin King

From: Colin Ian King <colin.king(a)canonical.com> The variable ret is being assigned with a value that is never read, it is being re-assigned the same value on the err0 exit path. The assignment is redundant and hence can be removed. Addresses-Coverity: ("Unused value") Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- drivers/dma-buf/heaps/system_heap.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 455782efbb32..817a1667bd57 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -55,10 +55,8 @@ static int system_heap_allocate(struct dma_heap *heap, helper_buffer->pages = kmalloc_array(helper_buffer->pagecount, sizeof(*helper_buffer->pages), GFP_KERNEL); - if (!helper_buffer->pages) { - ret = -ENOMEM; + if (!helper_buffer->pages) goto err0; - } for (pg = 0; pg < helper_buffer->pagecount; pg++) { /* -- 2.20.1

6 years, 1 month

1
0
0 0

[PATCH 1/4] dma-buf: change DMA-buf locking convention v2

by Christian König

This patch is a stripped down version of the locking changes necessary to support dynamic DMA-buf handling. It adds a dynamic flag for both importers as well as exporters so that drivers can choose if they want the reservation object locked or unlocked during mapping of attachments. For compatibility between drivers we cache the DMA-buf mapping during attaching an importer as soon as exporter/importer disagree on the dynamic handling. This change has gone through a lengthy discussion on dri-devel and other mailing lists with at least 3-4 different attempts and dead-ends until we settled on this solution. Please refer to the mailing lists archives for full background on the history of this change. v2: cleanup set_name merge, improve kerneldoc Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-buf.c | 102 +++++++++++++++++++++++++++++++++----- include/linux/dma-buf.h | 57 +++++++++++++++++++-- 2 files changed, 143 insertions(+), 16 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 433d91d710e4..753be84b5fd6 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) size_t ret = 0; dmabuf = dentry->d_fsdata; - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (dmabuf->name) ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); return dynamic_dname(dentry, buffer, buflen, "/%s:%s", dentry->d_name.name, ret > 0 ? name : ""); @@ -334,7 +334,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) if (IS_ERR(name)) return PTR_ERR(name); - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (!list_empty(&dmabuf->attachments)) { ret = -EBUSY; kfree(name); @@ -344,7 +344,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) dmabuf->name = name; out_unlock: - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); return ret; } @@ -403,10 +403,10 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) /* Don't count the temporary reference taken inside procfs seq_show */ seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1); seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name); - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (dmabuf->name) seq_printf(m, "name:\t%s\n", dmabuf->name); - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); } static const struct file_operations dma_buf_fops = { @@ -525,6 +525,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) return ERR_PTR(-EINVAL); } + if (WARN_ON(exp_info->ops->cache_sgt_mapping && + exp_info->ops->dynamic_mapping)) + return ERR_PTR(-EINVAL); + if (!try_module_get(exp_info->owner)) return ERR_PTR(-ENOENT); @@ -645,10 +649,11 @@ void dma_buf_put(struct dma_buf *dmabuf) EXPORT_SYMBOL_GPL(dma_buf_put); /** - * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality - * @dmabuf: [in] buffer to attach device to. - * @dev: [in] device to be attached. + * @dmabuf: [in] buffer to attach device to. + * @dev: [in] device to be attached. + * @dynamic_mapping: [in] calling convention for map/unmap * * Returns struct dma_buf_attachment pointer for this attachment. Attachments * must be cleaned up by calling dma_buf_detach(). @@ -662,8 +667,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev) +struct dma_buf_attachment * +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, + bool dynamic_mapping) { struct dma_buf_attachment *attach; int ret; @@ -677,6 +683,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, attach->dev = dev; attach->dmabuf = dmabuf; + attach->dynamic_mapping = dynamic_mapping; mutex_lock(&dmabuf->lock); @@ -685,16 +692,64 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, if (ret) goto err_attach; } + dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); + dma_resv_unlock(dmabuf->resv); mutex_unlock(&dmabuf->lock); + /* When either the importer or the exporter can't handle dynamic + * mappings we cache the mapping here to avoid issues with the + * reservation object lock. + */ + if (dma_buf_attachment_is_dynamic(attach) != + dma_buf_is_dynamic(dmabuf)) { + struct sg_table *sgt; + + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_lock(attach->dmabuf->resv, NULL); + + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + if (!sgt) + sgt = ERR_PTR(-ENOMEM); + if (IS_ERR(sgt)) { + ret = PTR_ERR(sgt); + goto err_unlock; + } + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + attach->sgt = sgt; + attach->dir = DMA_BIDIRECTIONAL; + } + return attach; err_attach: kfree(attach); mutex_unlock(&dmabuf->lock); return ERR_PTR(ret); + +err_unlock: + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + + dma_buf_detach(dmabuf, attach); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach); + +/** + * dma_buf_attach - Wrapper for dma_buf_dynamic_attach + * @dmabuf: [in] buffer to attach device to. + * @dev: [in] device to be attached. + * + * Wrapper to call dma_buf_dynamic_attach() for drivers which still use a static + * mapping. + */ +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, + struct device *dev) +{ + return dma_buf_dynamic_attach(dmabuf, dev, false); } EXPORT_SYMBOL_GPL(dma_buf_attach); @@ -711,11 +766,20 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (WARN_ON(!dmabuf || !attach)) return; - if (attach->sgt) + if (attach->sgt) { + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_lock(attach->dmabuf->resv, NULL); + dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + } + mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); list_del(&attach->node); + dma_resv_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); @@ -749,6 +813,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL); + if (dma_buf_attachment_is_dynamic(attach)) + dma_resv_assert_held(attach->dmabuf->resv); + if (attach->sgt) { /* * Two mappings with different directions for the same @@ -761,6 +828,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, return attach->sgt; } + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_assert_held(attach->dmabuf->resv); + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -793,9 +863,15 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf || !sg_table)) return; + if (dma_buf_attachment_is_dynamic(attach)) + dma_resv_assert_held(attach->dmabuf->resv); + if (attach->sgt == sg_table) return; + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_assert_held(attach->dmabuf->resv); + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); @@ -1219,10 +1295,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0; + dma_resv_lock(buf_obj->resv, NULL); list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; } + dma_resv_unlock(buf_obj->resv); seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index ec212cb27fdc..bcc0f4d0b678 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -42,6 +42,18 @@ struct dma_buf_ops { */ bool cache_sgt_mapping; + /** + * @dynamic_mapping: + * + * If true the framework makes sure that the map/unmap_dma_buf + * callbacks are always called with the dma_resv object locked. + * + * If false the framework makes ure that the map/unmap_dma_buf + * callbacks are always called without the dma_resv object locked. + * Mutual exclusive with @cache_sgt_mapping. + */ + bool dynamic_mapping; + /** * @attach: * @@ -109,6 +121,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. * + * This is always called with the dmabuf->resv object locked when + * the dynamic_mapping flag is true. + * * Returns: * * A &sg_table scatter list of or the backing storage of the DMA buffer, @@ -267,7 +282,8 @@ struct dma_buf_ops { * struct dma_buf - shared buffer object * @size: size of the buffer * @file: file pointer used for sharing buffers across, and for refcounting. - * @attachments: list of dma_buf_attachment that denotes all devices attached. + * @attachments: list of dma_buf_attachment that denotes all devices attached, + * protected by dma_resv lock. * @ops: dma_buf_ops associated with this buffer object. * @lock: used internally to serialize list manipulation, attach/detach and * vmap/unmap, and accesses to name @@ -323,10 +339,12 @@ struct dma_buf { * struct dma_buf_attachment - holds device-buffer attachment data * @dmabuf: buffer for this attachment. * @dev: device attached to the buffer. - * @node: list of dma_buf_attachment. + * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf. * @sgt: cached mapping. * @dir: direction of cached mapping. * @priv: exporter specific attachment data. + * @dynamic_mapping: true if dma_buf_map/unmap_attachment() is called with the + * dma_resv lock held. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -343,6 +361,7 @@ struct dma_buf_attachment { struct list_head node; struct sg_table *sgt; enum dma_data_direction dir; + bool dynamic_mapping; void *priv; }; @@ -394,10 +413,39 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } +/** + * dma_buf_is_dynamic - check if a DMA-buf uses dynamic mappings. + * @dmabuf: the DMA-buf to check + * + * Returns true if a DMA-buf exporter wants to be called with the dma_resv + * locked, false if it doesn't wants to be called with the lock held. + */ +static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) +{ + return dmabuf->ops->dynamic_mapping; +} + +/** + * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic + * mappinsg + * @attach: the DMA-buf attachment to check + * + * Returns true if a DMA-buf importer wants to call the map/unmap functions with + * the dma_resv lock held. + */ +static inline bool +dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach) +{ + return attach->dynamic_mapping; +} + struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev); + struct device *dev); +struct dma_buf_attachment * +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, + bool dynamic_mapping); void dma_buf_detach(struct dma_buf *dmabuf, - struct dma_buf_attachment *dmabuf_attach); + struct dma_buf_attachment *attach); struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info); @@ -409,6 +457,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *, enum dma_data_direction); +void dma_buf_move_notify(struct dma_buf *dma_buf); int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction dir); int dma_buf_end_cpu_access(struct dma_buf *dma_buf, -- 2.17.1

6 years, 1 month

2
6
0 0

Dynamic DMA-buf locking changes

by Christian König

Hi everyone, since upstreaming the full dynamic DMA-buf changes turned out more problematic than previously thought I've reverted back to individual patches and separated out only the locking changes. So this patch does NOT contain any new callbacks for pinning/unpinning and move notification, but only the locking changes necessary. As previously discussed when the framework detects that the locking semantics between exporter and importer are different it just falls back to using a cached sgt created during attach time. While separating the patch set I've most likely stumbled over the problem why this previously raised some lockdep warning with i915, it turned out to be just a might_lock() at the wrong place. Please review and/or comment, Christian.

6 years, 2 months

3
24
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig