| Message ID | 20230529223935.2672495-6-dmitry.osipenko@collabora.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Move dma-buf mmap() reservation locking down to exporters | expand |
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!
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);