From: Guangming Cao <redacted>

On Wed, 2021-10-13 at 14:20 +0200, Christian König wrote:

Am 13.10.21 um 01:56 schrieb Sumit Semwal:

quoted

Hello Guangming, Christian,



On Tue, 12 Oct 2021, 14:09 , [off-list ref] wrote:

quoted

From: Guangming Cao <redacted>

quoted

Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com:
From: Guangming Cao <redacted>

quoted

If dma-buf don't want userspace users to touch the dmabuf

buffer,

quoted

quoted

it seems we should add this restriction into

dma_buf_ops.mmap,

quoted

quoted

not in this IOCTL:DMA_BUF_SET_NAME.

With this restriction, we can only know the kernel users of

the dmabuf

quoted

quoted

by attachments.
However, for many userspace users, such as userpsace users of

dma_heap,

quoted

quoted

they also need to mark the usage of dma-buf, and they don't

care about

quoted

quoted

who attached to this dmabuf, and seems it's no meaning to be

waiting for

quoted

quoted

IOCTL:DMA_BUF_SET_NAME rather than mmap.

Sounds valid to me, but I have no idea why this restriction was

added in 

quoted

the first place.

Can you double check the git history and maybe identify when

that was 

quoted

added? Mentioning this change in the commit message then might

make 

quoted

things a bit easier to understand.

Thanks,
Christian.

It was add in this patch: 
https://patchwork.freedesktop.org/patch/310349/.
However, there is no illustration about it.
I guess it wants users to set_name when no attachments on the
dmabuf,
for case with attachments, we can find owner by device in
attachments.
But just I said in commit message, this is might not a good idea.

Do you have any idea?

For the original series, the idea was that allowing name change
mid-use could confuse the users about the dma-buf. However, the
rest of the series also makes sure each dma-buf have a unique
inode, and any accounting should probably use that, without relying
on the name as much.
So I don't have an objection to this change.

 
I suggest to add that explanation and the original commit id into the
commit message.

With that changed the patch has my rb as well.

Regards,
Christian.

updated, thanks!
Guangming.

quoted

Best,
Sumit.

quoted

quoted

quoted

Signed-off-by: Guangming Cao <redacted>
---
  drivers/dma-buf/dma-buf.c | 14 ++------------
  1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-

buf.c

quoted

quoted

index 511fe0d217a0..db2f4efdec32 100644

--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c

@@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file

*file, poll_table *poll)

quoted

quoted

  
  /**
   * dma_buf_set_name - Set a name to a specific dma_buf to

track the usage.

quoted

quoted

- * The name of the dma-buf buffer can only be set when the

dma-buf is not

quoted

quoted

- * attached to any devices. It could theoritically support

changing the

quoted

quoted

- * name of the dma-buf if the same piece of memory is used

for multiple

quoted

quoted

- * purpose between different devices.
+ * It could theoretically support changing the name of the

dma-buf if the same

quoted

quoted

+ * piece of memory is used for multiple purpose between

different devices.

quoted

quoted

   *
   * @dmabuf: [in]     dmabuf buffer that will be renamed.
   * @buf:    [in]     A piece of userspace memory that

contains the name of

quoted

quoted

@@ -346,19 +344,11 @@ static long dma_buf_set_name(struct

dma_buf *dmabuf, const char __user *buf)

quoted

quoted

    if (IS_ERR(name))
            return PTR_ERR(name);
  
-   dma_resv_lock(dmabuf->resv, NULL);
-   if (!list_empty(&dmabuf->attachments)) {
-           ret = -EBUSY;
-           kfree(name);
-           goto out_unlock;
-   }
    spin_lock(&dmabuf->name_lock);
    kfree(dmabuf->name);
    dmabuf->name = name;
    spin_unlock(&dmabuf->name_lock);
  
-out_unlock:
-   dma_resv_unlock(dmabuf->resv);
    return ret;
  }
  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel