| Message ID | 20230529223935.2672495-6-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move dma-buf mmap() reservation locking down to exporters | expand |
On 5/31/23 22:58, Dmitry Osipenko wrote: > On 5/30/23 01:39, Dmitry Osipenko wrote: >> Change locking policy of mmap() callback, making exporters responsible >> for handling dma-buf reservation locking. Previous locking policy stated >> that dma-buf is locked for both importers and exporters by the dma-buf >> core, which caused a deadlock problem for DRM drivers in a case of >> self-imported dma-bufs which required to take the lock from the DRM >> exporter side. >> >> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/dma-buf/dma-buf.c | 17 +++-------------- >> 1 file changed, 3 insertions(+), 14 deletions(-) > > Christian, you acked the drm patch of this series sometime ago, perhaps > it also implies implicit ack to this patch, but I'd prefer to have the > explicit ack. I'll apply this series to drm-misc later this week if > you'll approve this dma-buf change. Thanks in advance! I'll merge the patches tomorrow. If there are any additional comments, then please don't hesitate to post them.
Am 20.06.23 um 17:58 schrieb Dmitry Osipenko: > On 5/31/23 22:58, Dmitry Osipenko wrote: >> On 5/30/23 01:39, Dmitry Osipenko wrote: >>> Change locking policy of mmap() callback, making exporters responsible >>> for handling dma-buf reservation locking. Previous locking policy stated >>> that dma-buf is locked for both importers and exporters by the dma-buf >>> core, which caused a deadlock problem for DRM drivers in a case of >>> self-imported dma-bufs which required to take the lock from the DRM >>> exporter side. >>> >>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> --- >>> drivers/dma-buf/dma-buf.c | 17 +++-------------- >>> 1 file changed, 3 insertions(+), 14 deletions(-) >> Christian, you acked the drm patch of this series sometime ago, perhaps >> it also implies implicit ack to this patch, but I'd prefer to have the >> explicit ack. I'll apply this series to drm-misc later this week if >> you'll approve this dma-buf change. Thanks in advance! > I'll merge the patches tomorrow. If there are any additional comments, > then please don't hesitate to post them. Sorry for not responding earlier, I have been moving both my office as well as my household and still catching up. I don't have time for an in-deep review, but my ack stands for the whole series. Regards, Christian.
On 6/21/23 08:42, Christian König wrote: > Am 20.06.23 um 17:58 schrieb Dmitry Osipenko: >> On 5/31/23 22:58, Dmitry Osipenko wrote: >>> On 5/30/23 01:39, Dmitry Osipenko wrote: >>>> Change locking policy of mmap() callback, making exporters responsible >>>> for handling dma-buf reservation locking. Previous locking policy >>>> stated >>>> that dma-buf is locked for both importers and exporters by the dma-buf >>>> core, which caused a deadlock problem for DRM drivers in a case of >>>> self-imported dma-bufs which required to take the lock from the DRM >>>> exporter side. >>>> >>>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> drivers/dma-buf/dma-buf.c | 17 +++-------------- >>>> 1 file changed, 3 insertions(+), 14 deletions(-) >>> Christian, you acked the drm patch of this series sometime ago, perhaps >>> it also implies implicit ack to this patch, but I'd prefer to have the >>> explicit ack. I'll apply this series to drm-misc later this week if >>> you'll approve this dma-buf change. Thanks in advance! >> I'll merge the patches tomorrow. If there are any additional comments, >> then please don't hesitate to post them. > > Sorry for not responding earlier, I have been moving both my office as > well as my household and still catching up. > > I don't have time for an in-deep review, but my ack stands for the whole > series. Thanks! I'll add yours ack and merge patches soon
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index aa4ea8530cb3..21916bba77d5 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -131,7 +131,6 @@ static struct file_system_type dma_buf_fs_type = { static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) { struct dma_buf *dmabuf; - int ret; if (!is_dma_buf_file(file)) return -EINVAL; @@ -147,11 +146,7 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) dmabuf->size >> PAGE_SHIFT) return -EINVAL; - dma_resv_lock(dmabuf->resv, NULL); - ret = dmabuf->ops->mmap(dmabuf, vma); - dma_resv_unlock(dmabuf->resv); - - return ret; + return dmabuf->ops->mmap(dmabuf, vma); } static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) @@ -850,6 +845,7 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, * - &dma_buf_ops.release() * - &dma_buf_ops.begin_cpu_access() * - &dma_buf_ops.end_cpu_access() + * - &dma_buf_ops.mmap() * * 2. These &dma_buf_ops callbacks are invoked with locked dma-buf * reservation and exporter can't take the lock: @@ -858,7 +854,6 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, * - &dma_buf_ops.unpin() * - &dma_buf_ops.map_dma_buf() * - &dma_buf_ops.unmap_dma_buf() - * - &dma_buf_ops.mmap() * - &dma_buf_ops.vmap() * - &dma_buf_ops.vunmap() * @@ -1463,8 +1458,6 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { - int ret; - if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1485,11 +1478,7 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff; - dma_resv_lock(dmabuf->resv, NULL); - ret = dmabuf->ops->mmap(dmabuf, vma); - dma_resv_unlock(dmabuf->resv); - - return ret; + return dmabuf->ops->mmap(dmabuf, vma); } EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);