* [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME
2021-10-14 10:25 ` [PATCH v3] " guangming.cao
@ 2021-10-14 10:35 ` guangming.cao
2021-10-26 8:04 ` guangming.cao
2021-10-26 11:18 ` Christian König
2 siblings, 0 replies; 11+ messages in thread
From: guangming.cao @ 2021-10-14 10:35 UTC (permalink / raw)
To: christian.koenig, sumit.semwal
Cc: guangming.cao, dri-devel, linaro-mm-sig, linux-arm-kernel,
linux-kernel, linux-media, linux-mediatek, matthias.bgg, rdunlap,
wsd_upstream, 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;
> > > > > }
> > > > >
> > >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME
2021-10-14 10:25 ` [PATCH v3] " guangming.cao
2021-10-14 10:35 ` guangming.cao
@ 2021-10-26 8:04 ` guangming.cao
2021-10-26 11:18 ` Christian König
2 siblings, 0 replies; 11+ messages in thread
From: guangming.cao @ 2021-10-26 8:04 UTC (permalink / raw)
To: christian.koenig, sumit.semwal
Cc: dri-devel, guangming.cao, linaro-mm-sig, linux-arm-kernel,
linux-kernel, linux-media, linux-mediatek, matthias.bgg, rdunlap,
wsd_upstream, 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,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME
2021-10-14 10:25 ` [PATCH v3] " guangming.cao
2021-10-14 10:35 ` guangming.cao
2021-10-26 8:04 ` guangming.cao
@ 2021-10-26 11:18 ` Christian König
2021-10-26 11:52 ` guangming.cao
2 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2021-10-26 11:18 UTC (permalink / raw)
To: guangming.cao, sumit.semwal
Cc: dri-devel, linaro-mm-sig, linux-arm-kernel, linux-kernel,
linux-media, linux-mediatek, matthias.bgg, rdunlap, wsd_upstream
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,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME
2021-10-26 11:18 ` Christian König
@ 2021-10-26 11:52 ` guangming.cao
2021-10-28 11:35 ` Christian König
0 siblings, 1 reply; 11+ messages in thread
From: guangming.cao @ 2021-10-26 11:52 UTC (permalink / raw)
To: christian.koenig
Cc: dri-devel, guangming.cao, linaro-mm-sig, linux-arm-kernel,
linux-kernel, linux-media, linux-mediatek, matthias.bgg, rdunlap,
sumit.semwal, wsd_upstream, 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,
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME
2021-10-26 11:52 ` guangming.cao
@ 2021-10-28 11:35 ` Christian König
0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2021-10-28 11:35 UTC (permalink / raw)
To: guangming.cao
Cc: dri-devel, linaro-mm-sig, linux-arm-kernel, linux-kernel,
linux-media, linux-mediatek, matthias.bgg, rdunlap, sumit.semwal,
wsd_upstream
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&data=04%7C01%7Cchristian.koenig%40amd.com%7C6652f423b96547b8321308d998772121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637708459727236977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4EFg60sCxQzNuMKB0VjhbpvoeGlcAbofErMtcjPtIfw%3D&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&data=04%7C01%7Cchristian.koenig%40amd.com%7C6652f423b96547b8321308d998772121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637708459727236977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=c%2Bbso%2BSxiPjwlv%2B9tAzA4x0gf5UktREhSiAsODFkihk%3D&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,
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread