[PATCH] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[PATCH] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

If dma-buf don't want userspace users to touch the dmabuf buffer,
it seems we should add this restriction into dma_buf_ops.mmap,
not in this IOCTL:DMA_BUF_SET_NAME.

With this restriction, we can only know the kernel users of the dmabuf
by attachments.
However, for many userspace users, such as userpsace users of dma_heap,
they also need to mark the usage of dma-buf, and they don't care about
who attached to this dmabuf, and seems it's no meaning to waitting for
IOCTL:DMA_BUF_SET_NAME rather than mmap.

Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
---
 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
index 511fe0d217a0..afbd0a226639 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)
 
 /**
  * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
- * The name of the dma-buf buffer can only be set when the dma-buf is not
- * attached to any devices. It could theoritically support changing the
- * name of the dma-buf if the same piece of memory is used for multiple
- * purpose between different devices.
+ * It could theoritically support changing the name of the dma-buf if the same
+ * piece of memory is used for multiple purpose between different devices.
  *
  * @dmabuf: [in]     dmabuf buffer that will be renamed.
  * @buf:    [in]     A piece of userspace memory that contains the name of
@@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
 	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;
 }
 
-- 
2.17.1

Re: [PATCH] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Randy Dunlap]

On 10/8/21 7:47 PM, guangming.cao@mediatek.com wrote:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
> 
> If dma-buf don't want userspace users to touch the dmabuf buffer,
> it seems we should add this restriction into dma_buf_ops.mmap,
> not in this IOCTL:DMA_BUF_SET_NAME.
> 
> With this restriction, we can only know the kernel users of the dmabuf
> by attachments.
> However, for many userspace users, such as userpsace users of dma_heap,
> they also need to mark the usage of dma-buf, and they don't care about
> who attached to this dmabuf, and seems it's no meaning to waitting for

                                                          to be waiting for

> IOCTL:DMA_BUF_SET_NAME rather than mmap.
> 
> Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> ---
>   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
> index 511fe0d217a0..afbd0a226639 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)
>   
>   /**
>    * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
> - * The name of the dma-buf buffer can only be set when the dma-buf is not
> - * attached to any devices. It could theoritically support changing the
> - * name of the dma-buf if the same piece of memory is used for multiple
> - * purpose between different devices.
> + * It could theoritically support changing the name of the dma-buf if the same

                theoretically

(yes, it was incorrect before this change.)

> + * piece of memory is used for multiple purpose between different devices.
>    *
>    * @dmabuf: [in]     dmabuf buffer that will be renamed.
>    * @buf:    [in]     A piece of userspace memory that contains the name of
> @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
>   	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;
>   }
>   
> 


-- 
~Randy

[PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

If dma-buf don't want userspace users to touch the dmabuf buffer,
it seems we should add this restriction into dma_buf_ops.mmap,
not in this IOCTL:DMA_BUF_SET_NAME.

With this restriction, we can only know the kernel users of the dmabuf
by attachments.
However, for many userspace users, such as userpsace users of dma_heap,
they also need to mark the usage of dma-buf, and they don't care about
who attached to this dmabuf, and seems it's no meaning to be waiting for
IOCTL:DMA_BUF_SET_NAME rather than mmap.

Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
---
 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
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)
 
 /**
  * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
- * The name of the dma-buf buffer can only be set when the dma-buf is not
- * attached to any devices. It could theoritically support changing the
- * name of the dma-buf if the same piece of memory is used for multiple
- * purpose between different devices.
+ * It could theoretically support changing the name of the dma-buf if the same
+ * piece of memory is used for multiple purpose between different devices.
  *
  * @dmabuf: [in]     dmabuf buffer that will be renamed.
  * @buf:    [in]     A piece of userspace memory that contains the name of
@@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
 	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;
 }
 
-- 
2.17.1

Re: [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Christian König]

Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
>
> If dma-buf don't want userspace users to touch the dmabuf buffer,
> it seems we should add this restriction into dma_buf_ops.mmap,
> not in this IOCTL:DMA_BUF_SET_NAME.
>
> With this restriction, we can only know the kernel users of the dmabuf
> by attachments.
> However, for many userspace users, such as userpsace users of dma_heap,
> they also need to mark the usage of dma-buf, and they don't care about
> who attached to this dmabuf, and seems it's no meaning to be waiting for
> IOCTL:DMA_BUF_SET_NAME rather than mmap.

Sounds valid to me, but I have no idea why this restriction was added in 
the first place.

Can you double check the git history and maybe identify when that was 
added? Mentioning this change in the commit message then might make 
things a bit easier to understand.

Thanks,
Christian.

>
> Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> ---
>   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
> 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)
>   
>   /**
>    * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
> - * The name of the dma-buf buffer can only be set when the dma-buf is not
> - * attached to any devices. It could theoritically support changing the
> - * name of the dma-buf if the same piece of memory is used for multiple
> - * purpose between different devices.
> + * It could theoretically support changing the name of the dma-buf if the same
> + * piece of memory is used for multiple purpose between different devices.
>    *
>    * @dmabuf: [in]     dmabuf buffer that will be renamed.
>    * @buf:    [in]     A piece of userspace memory that contains the name of
> @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
>   	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;
>   }
>   

Re: [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

> Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
> >
> > If dma-buf don't want userspace users to touch the dmabuf buffer,
> > it seems we should add this restriction into dma_buf_ops.mmap,
> > not in this IOCTL:DMA_BUF_SET_NAME.
> >
> > With this restriction, we can only know the kernel users of the dmabuf
> > by attachments.
> > However, for many userspace users, such as userpsace users of dma_heap,
> > they also need to mark the usage of dma-buf, and they don't care about
> > who attached to this dmabuf, and seems it's no meaning to be waiting for
> > IOCTL:DMA_BUF_SET_NAME rather than mmap.
> 
> Sounds valid to me, but I have no idea why this restriction was added in 
> the first place.
> 
> Can you double check the git history and maybe identify when that was 
> added? Mentioning this change in the commit message then might make 
> 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?
> 
> >
> > Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> > ---
> >   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
> > 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)
> >   
> >   /**
> >    * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
> > - * The name of the dma-buf buffer can only be set when the dma-buf is not
> > - * attached to any devices. It could theoritically support changing the
> > - * name of the dma-buf if the same piece of memory is used for multiple
> > - * purpose between different devices.
> > + * It could theoretically support changing the name of the dma-buf if the same
> > + * piece of memory is used for multiple purpose between different devices.
> >    *
> >    * @dmabuf: [in]     dmabuf buffer that will be renamed.
> >    * @buf:    [in]     A piece of userspace memory that contains the name of
> > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
> >   	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;
> >   }
> >   

Re: [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Sumit Semwal]

[-- Attachment #1: Type: text/plain, Size: 3723 bytes --]

Hello Guangming, Christian,



On Tue, 12 Oct 2021, 14:09 , <guangming.cao@mediatek.com> wrote:

> From: Guangming Cao <Guangming.Cao@mediatek.com>
>
> > Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com:
> > From: Guangming Cao <Guangming.Cao@mediatek.com>
> > >
> > > If dma-buf don't want userspace users to touch the dmabuf buffer,
> > > it seems we should add this restriction into dma_buf_ops.mmap,
> > > not in this IOCTL:DMA_BUF_SET_NAME.
> > >
> > > With this restriction, we can only know the kernel users of the dmabuf
> > > by attachments.
> > > However, for many userspace users, such as userpsace users of dma_heap,
> > > they also need to mark the usage of dma-buf, and they don't care about
> > > who attached to this dmabuf, and seems it's no meaning to be waiting
> for
> > > IOCTL:DMA_BUF_SET_NAME rather than mmap.
> >
> > Sounds valid to me, but I have no idea why this restriction was added in
> > the first place.
> >
> > Can you double check the git history and maybe identify when that was
> > added? Mentioning this change in the commit message then might make
> > 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.

Best,
Sumit.

> >
> > >
> > > Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> > > ---
> > >   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
> > > 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)
> > >
> > >   /**
> > >    * dma_buf_set_name - Set a name to a specific dma_buf to track the
> usage.
> > > - * The name of the dma-buf buffer can only be set when the dma-buf is
> not
> > > - * attached to any devices. It could theoritically support changing
> the
> > > - * name of the dma-buf if the same piece of memory is used for
> multiple
> > > - * purpose between different devices.
> > > + * It could theoretically support changing the name of the dma-buf if
> the same
> > > + * piece of memory is used for multiple purpose between different
> devices.
> > >    *
> > >    * @dmabuf: [in]     dmabuf buffer that will be renamed.
> > >    * @buf:    [in]     A piece of userspace memory that contains the
> name of
> > > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf
> *dmabuf, const char __user *buf)
> > >     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;
> > >   }
> > >
>

[-- Attachment #2: Type: text/html, Size: 5719 bytes --]

Re: [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Christian König]

[-- Attachment #1: Type: text/plain, Size: 5037 bytes --]

Am 13.10.21 um 01:56 schrieb Sumit Semwal:
> Hello Guangming, Christian,
>
>
>
> On Tue, 12 Oct 2021, 14:09 , <guangming.cao@mediatek.com 
> <mailto:guangming.cao@mediatek.com>> wrote:
>
>     From: Guangming Cao <Guangming.Cao@mediatek.com
>     <mailto:Guangming.Cao@mediatek.com>>
>
>     > Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com
>     <mailto:guangming.cao@mediatek.com>:
>     > From: Guangming Cao <Guangming.Cao@mediatek.com
>     <mailto:Guangming.Cao@mediatek.com>>
>     > >
>     > > If dma-buf don't want userspace users to touch the dmabuf buffer,
>     > > it seems we should add this restriction into dma_buf_ops.mmap,
>     > > not in this IOCTL:DMA_BUF_SET_NAME.
>     > >
>     > > With this restriction, we can only know the kernel users of
>     the dmabuf
>     > > by attachments.
>     > > However, for many userspace users, such as userpsace users of
>     dma_heap,
>     > > they also need to mark the usage of dma-buf, and they don't
>     care about
>     > > who attached to this dmabuf, and seems it's no meaning to be
>     waiting for
>     > > IOCTL:DMA_BUF_SET_NAME rather than mmap.
>     >
>     > Sounds valid to me, but I have no idea why this restriction was
>     added in
>     > the first place.
>     >
>     > Can you double check the git history and maybe identify when
>     that was
>     > added? Mentioning this change in the commit message then might make
>     > things a bit easier to understand.
>     >
>     > Thanks,
>     > Christian.
>     It was add in this patch:
>     https://patchwork.freedesktop.org/patch/310349/
>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F310349%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C4149923e2b0646de82ce08d98ddbf2c2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637696798278342557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N49RVF4s%2BGQ4D%2Ft1MOwRsCnslFnwobSB3G86pvP9m7A%3D&reserved=0>.
>     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.

>
> Best,
> Sumit.
>
>     >
>     > >
>     > > Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com
>     <mailto:Guangming.Cao@mediatek.com>>
>     > > ---
>     > >   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
>     > > 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)
>     > >
>     > >   /**
>     > >    * dma_buf_set_name - Set a name to a specific dma_buf to
>     track the usage.
>     > > - * The name of the dma-buf buffer can only be set when the
>     dma-buf is not
>     > > - * attached to any devices. It could theoritically support
>     changing the
>     > > - * name of the dma-buf if the same piece of memory is used
>     for multiple
>     > > - * purpose between different devices.
>     > > + * It could theoretically support changing the name of the
>     dma-buf if the same
>     > > + * piece of memory is used for multiple purpose between
>     different devices.
>     > >    *
>     > >    * @dmabuf: [in]     dmabuf buffer that will be renamed.
>     > >    * @buf:    [in]     A piece of userspace memory that
>     contains the name of
>     > > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct
>     dma_buf *dmabuf, const char __user *buf)
>     > >     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;
>     > >   }
>     > >
>


[-- Attachment #2: Type: text/html, Size: 9308 bytes --]

[PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

In this patch(https://patchwork.freedesktop.org/patch/310349),
it add a new IOCTL to support dma-buf user to set debug name.

But it also added a limitation of this IOCTL, it needs the
attachments of dmabuf should be empty, otherwise it will fail.

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(https://patchwork.freedesktop.org/patch/310387/), and any accounting
should probably use that, without relying on the name as much.

So, removing this restriction will let dma-buf userspace users to use it
more comfortably and without any side effect.

Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
---
 drivers/dma-buf/dma-buf.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 511fe0d217a0..5fbb3a2068a3 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)
 
 /**
  * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
- * The name of the dma-buf buffer can only be set when the dma-buf is not
- * attached to any devices. It could theoritically support changing the
- * name of the dma-buf if the same piece of memory is used for multiple
- * purpose between different devices.
+ * It could support changing the name of the dma-buf if the same
+ * piece of memory is used for multiple purpose between different devices.
  *
  * @dmabuf: [in]     dmabuf buffer that will be renamed.
  * @buf:    [in]     A piece of userspace memory that contains the name of
@@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
 static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
 {
 	char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
-	long ret = 0;
 
 	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;
+	return 0;
 }
 
 static long dma_buf_ioctl(struct file *file,
-- 
2.17.1

[PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

On Wed, 2021-10-13 at 14:20 +0200, Christian König wrote:
> Am 13.10.21 um 01:56 schrieb Sumit Semwal:
> > Hello Guangming, Christian,
> > 
> > 
> > 
> > On Tue, 12 Oct 2021, 14:09 , <guangming.cao@mediatek.com> wrote:
> > > From: Guangming Cao <Guangming.Cao@mediatek.com>
> > > 
> > > > Am 09.10.21 um 07:55 schrieb guangming.cao@mediatek.com:
> > > > From: Guangming Cao <Guangming.Cao@mediatek.com>
> > > > >
> > > > > If dma-buf don't want userspace users to touch the dmabuf
> > > buffer,
> > > > > it seems we should add this restriction into
> > > dma_buf_ops.mmap,
> > > > > not in this IOCTL:DMA_BUF_SET_NAME.
> > > > >
> > > > > With this restriction, we can only know the kernel users of
> > > the dmabuf
> > > > > by attachments.
> > > > > However, for many userspace users, such as userpsace users of
> > > dma_heap,
> > > > > they also need to mark the usage of dma-buf, and they don't
> > > care about
> > > > > who attached to this dmabuf, and seems it's no meaning to be
> > > waiting for
> > > > > IOCTL:DMA_BUF_SET_NAME rather than mmap.
> > > > 
> > > > Sounds valid to me, but I have no idea why this restriction was
> > > added in 
> > > > the first place.
> > > > 
> > > > Can you double check the git history and maybe identify when
> > > that was 
> > > > added? Mentioning this change in the commit message then might
> > > make 
> > > > 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.

> > Best,
> > Sumit.
> > > > 
> > > > >
> > > > > Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> > > > > ---
> > > > >   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
> > > > > 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)
> > > > >   
> > > > >   /**
> > > > >    * dma_buf_set_name - Set a name to a specific dma_buf to
> > > track the usage.
> > > > > - * The name of the dma-buf buffer can only be set when the
> > > dma-buf is not
> > > > > - * attached to any devices. It could theoritically support
> > > changing the
> > > > > - * name of the dma-buf if the same piece of memory is used
> > > for multiple
> > > > > - * purpose between different devices.
> > > > > + * It could theoretically support changing the name of the
> > > dma-buf if the same
> > > > > + * piece of memory is used for multiple purpose between
> > > different devices.
> > > > >    *
> > > > >    * @dmabuf: [in]     dmabuf buffer that will be renamed.
> > > > >    * @buf:    [in]     A piece of userspace memory that
> > > contains the name of
> > > > > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct
> > > dma_buf *dmabuf, const char __user *buf)
> > > > >     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;
> > > > >   }
> > > > >   
> > > 
>  

[PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

On Thu, 2021-10-14 at 18:25 +0800, guangming.cao@mediatek.com wrote:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
> 
> In this patch(https://patchwork.freedesktop.org/patch/310349),
> it add a new IOCTL to support dma-buf user to set debug name.
> 
> But it also added a limitation of this IOCTL, it needs the
> attachments of dmabuf should be empty, otherwise it will fail.
> 
> 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(https://patchwork.freedesktop.org/patch/310387/), and any
> accounting
> should probably use that, without relying on the name as much.
> 
> So, removing this restriction will let dma-buf userspace users to use
> it
> more comfortably and without any side effect.
> 
Hi christian, sumit,

Just a gentle ping for this patch, please kindly let me know your comments about this patch.
Thanks!

Guangming

> Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> ---
>  drivers/dma-buf/dma-buf.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 511fe0d217a0..5fbb3a2068a3 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)
>  
>  /**
>   * dma_buf_set_name - Set a name to a specific dma_buf to track the
> usage.
> - * The name of the dma-buf buffer can only be set when the dma-buf
> is not
> - * attached to any devices. It could theoritically support changing
> the
> - * name of the dma-buf if the same piece of memory is used for
> multiple
> - * purpose between different devices.
> + * It could support changing the name of the dma-buf if the same
> + * piece of memory is used for multiple purpose between different
> devices.
>   *
>   * @dmabuf: [in]     dmabuf buffer that will be renamed.
>   * @buf:    [in]     A piece of userspace memory that contains the
> name of
> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file,
> poll_table *poll)
>  static long dma_buf_set_name(struct dma_buf *dmabuf, const char
> __user *buf)
>  {
>  	char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
> -	long ret = 0;
>  
>  	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;
> +	return 0;
>  }
>  
>  static long dma_buf_ioctl(struct file *file,

Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Christian König]

Am 14.10.21 um 12:25 schrieb guangming.cao@mediatek.com:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
>
> In this patch(https://patchwork.freedesktop.org/patch/310349),
> it add a new IOCTL to support dma-buf user to set debug name.
>
> But it also added a limitation of this IOCTL, it needs the
> attachments of dmabuf should be empty, otherwise it will fail.
>
> 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(https://patchwork.freedesktop.org/patch/310387/), and any accounting
> should probably use that, without relying on the name as much.
>
> So, removing this restriction will let dma-buf userspace users to use it
> more comfortably and without any side effect.
>
> Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>

We could now cleanup the return value from dma_buf_set_name() into a 
void since that function can't fail any more as far as I can see.

But that isn't mandatory I think, patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

> ---
>   drivers/dma-buf/dma-buf.c | 17 +++--------------
>   1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 511fe0d217a0..5fbb3a2068a3 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)
>   
>   /**
>    * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
> - * The name of the dma-buf buffer can only be set when the dma-buf is not
> - * attached to any devices. It could theoritically support changing the
> - * name of the dma-buf if the same piece of memory is used for multiple
> - * purpose between different devices.
> + * It could support changing the name of the dma-buf if the same
> + * piece of memory is used for multiple purpose between different devices.
>    *
>    * @dmabuf: [in]     dmabuf buffer that will be renamed.
>    * @buf:    [in]     A piece of userspace memory that contains the name of
> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
>   {
>   	char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
> -	long ret = 0;
>   
>   	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;
> +	return 0;
>   }
>   
>   static long dma_buf_ioctl(struct file *file,

Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[guangming.cao]

From: Guangming Cao <Guangming.Cao@mediatek.com>

On Tue, 2021-10-26 at 13:18 +0200, Christian König wrote:
> Am 14.10.21 um 12:25 schrieb guangming.cao@mediatek.com:
> > From: Guangming Cao <Guangming.Cao@mediatek.com>
> > 
> > In this patch(https://patchwork.freedesktop.org/patch/310349),
> > it add a new IOCTL to support dma-buf user to set debug name.
> > 
> > But it also added a limitation of this IOCTL, it needs the
> > attachments of dmabuf should be empty, otherwise it will fail.
> > 
> > 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(https://patchwork.freedesktop.org/patch/310387/), and any
> > accounting
> > should probably use that, without relying on the name as much.
> > 
> > So, removing this restriction will let dma-buf userspace users to
> > use it
> > more comfortably and without any side effect.
> > 
> > Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
> 
> We could now cleanup the return value from dma_buf_set_name() into a 
> void since that function can't fail any more as far as I can see.
> 
> But that isn't mandatory I think, patch is Reviewed-by: Christian
> König 
> <christian.koenig@amd.com>
>

So, here is no need to check return value of 'strndup_user',
just return without error code if the almost impossible error occurs?

Guangming.

> Regards,
> Christian.
> 
> > ---
> >   drivers/dma-buf/dma-buf.c | 17 +++--------------
> >   1 file changed, 3 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 511fe0d217a0..5fbb3a2068a3 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)
> >   
> >   /**
> >    * dma_buf_set_name - Set a name to a specific dma_buf to track
> > the usage.
> > - * The name of the dma-buf buffer can only be set when the dma-buf 
> > is not
> > - * attached to any devices. It could theoritically support
> > changing the
> > - * name of the dma-buf if the same piece of memory is used for
> > multiple
> > - * purpose between different devices.
> > + * It could support changing the name of the dma-buf if the same
> > + * piece of memory is used for multiple purpose between different
> > devices.
> >    *
> >    * @dmabuf: [in]     dmabuf buffer that will be renamed.
> >    * @buf:    [in]     A piece of userspace memory that contains
> > the name of
> > @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file
> > *file, poll_table *poll)
> >   static long dma_buf_set_name(struct dma_buf *dmabuf, const char
> > __user *buf)
> >   {
> >   	char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
> > -	long ret = 0;
> >   
> >   	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;
> > +	return 0;
> >   }
> >   
> >   static long dma_buf_ioctl(struct file *file,
> 
> 

Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

[Christian König]

Am 26.10.21 um 13:52 schrieb guangming.cao@mediatek.com:
> From: Guangming Cao <Guangming.Cao@mediatek.com>
>
> On Tue, 2021-10-26 at 13:18 +0200, Christian König wrote:
>> Am 14.10.21 um 12:25 schrieb guangming.cao@mediatek.com:
>>> From: Guangming Cao <Guangming.Cao@mediatek.com>
>>>
>>> In this patch(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F310349&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C6652f423b96547b8321308d998772121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637708459727236977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4EFg60sCxQzNuMKB0VjhbpvoeGlcAbofErMtcjPtIfw%3D&amp;reserved=0),
>>> it add a new IOCTL to support dma-buf user to set debug name.
>>>
>>> But it also added a limitation of this IOCTL, it needs the
>>> attachments of dmabuf should be empty, otherwise it will fail.
>>>
>>> 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(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F310387%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C6652f423b96547b8321308d998772121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637708459727236977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=c%2Bbso%2BSxiPjwlv%2B9tAzA4x0gf5UktREhSiAsODFkihk%3D&amp;reserved=0), and any
>>> accounting
>>> should probably use that, without relying on the name as much.
>>>
>>> So, removing this restriction will let dma-buf userspace users to
>>> use it
>>> more comfortably and without any side effect.
>>>
>>> Signed-off-by: Guangming Cao <Guangming.Cao@mediatek.com>
>> We could now cleanup the return value from dma_buf_set_name() into a
>> void since that function can't fail any more as far as I can see.
>>
>> But that isn't mandatory I think, patch is Reviewed-by: Christian
>> König
>> <christian.koenig@amd.com>
>>
> So, here is no need to check return value of 'strndup_user',
> just return without error code if the almost impossible error occurs?

Good point, totally missed that one.

In that case I'm going to push the patch to drm-misc-next as is.

Regards,
Christian.

>
> Guangming.
>
>> Regards,
>> Christian.
>>
>>> ---
>>>    drivers/dma-buf/dma-buf.c | 17 +++--------------
>>>    1 file changed, 3 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 511fe0d217a0..5fbb3a2068a3 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)
>>>    
>>>    /**
>>>     * dma_buf_set_name - Set a name to a specific dma_buf to track
>>> the usage.
>>> - * The name of the dma-buf buffer can only be set when the dma-buf
>>> is not
>>> - * attached to any devices. It could theoritically support
>>> changing the
>>> - * name of the dma-buf if the same piece of memory is used for
>>> multiple
>>> - * purpose between different devices.
>>> + * It could support changing the name of the dma-buf if the same
>>> + * piece of memory is used for multiple purpose between different
>>> devices.
>>>     *
>>>     * @dmabuf: [in]     dmabuf buffer that will be renamed.
>>>     * @buf:    [in]     A piece of userspace memory that contains
>>> the name of
>>> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file
>>> *file, poll_table *poll)
>>>    static long dma_buf_set_name(struct dma_buf *dmabuf, const char
>>> __user *buf)
>>>    {
>>>    	char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
>>> -	long ret = 0;
>>>    
>>>    	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;
>>> +	return 0;
>>>    }
>>>    
>>>    static long dma_buf_ioctl(struct file *file,
>>