diff mbox series

[v4,5/6] dma-buf: Change locking policy for mmap()

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

Commit Message

Dmitry Osipenko May 29, 2023, 10:39 p.m. UTC
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(-)

Comments

Dmitry Osipenko June 20, 2023, 3:58 p.m. UTC | #1
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.
Christian König June 21, 2023, 5:42 a.m. UTC | #2
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.
Dmitry Osipenko June 21, 2023, 6:17 p.m. UTC | #3
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 mbox series

Patch

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);