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; }
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; }
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; }
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; }
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; }
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; }
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; > > } > >
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,
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; }
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,
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,
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,
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....), 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....), 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,
dri-devel@lists.freedesktop.org
-
Christian König -
guangming.cao＠mediatek.com -
Randy Dunlap -
Sumit Semwal