Am 26.10.21 um 10:34 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd(a)arndb.de>
>
> The new driver incorrectly unwinds after errors, as clang points out:
>
> drivers/dma-buf/st-dma-resv.c:295:7: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> if (r) {
> ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
> while (i--)
> …
[View More] ^
> drivers/dma-buf/st-dma-resv.c:295:3: note: remove the 'if' if its condition is always false
> if (r) {
> ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:288:6: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> if (r) {
> ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
> while (i--)
> ^
> drivers/dma-buf/st-dma-resv.c:288:2: note: remove the 'if' if its condition is always false
> if (r) {
> ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:280:10: note: initialize the variable 'i' to silence this warning
> int r, i;
> ^
> = 0
>
> Skip cleaning up the bits that have not been allocated at this point.
>
> Fixes: 1d51775cd3f5 ("dma-buf: add dma_resv selftest v4")
> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>
I already send out a patch to fix this up, but forgot to fix both gotos.
Going to add my rb and using that one here instead.
Thanks,
Christian.
> ---
> I'm not familiar with these interfaces, so I'm just guessing where
> we should jump after an error, please double-check and fix if necessary.
> ---
> drivers/dma-buf/st-dma-resv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c
> index 6f3ba756da3e..bc32b3eedcb6 100644
> --- a/drivers/dma-buf/st-dma-resv.c
> +++ b/drivers/dma-buf/st-dma-resv.c
> @@ -287,7 +287,7 @@ static int test_get_fences(void *arg, bool shared)
> r = dma_resv_lock(&resv, NULL);
> if (r) {
> pr_err("Resv locking failed\n");
> - goto err_free;
> + goto err_resv;
> }
>
> if (shared) {
> @@ -295,7 +295,7 @@ static int test_get_fences(void *arg, bool shared)
> if (r) {
> pr_err("Resv shared slot allocation failed\n");
> dma_resv_unlock(&resv);
> - goto err_free;
> + goto err_resv;
> }
>
> dma_resv_add_shared_fence(&resv, f);
> @@ -336,6 +336,7 @@ static int test_get_fences(void *arg, bool shared)
> while (i--)
> dma_fence_put(fences[i]);
> kfree(fences);
> +err_resv:
> dma_resv_fini(&resv);
> dma_fence_put(f);
> return r;
[View Less]
Hi,
On 10/23/21 4:14 AM, Flora Fu wrote:
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index d9bac2710494..074b0cf24c44 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -24,6 +24,24 @@ config MTK_APU_PM
> APU power domain shall be enabled before accessing the
> internal sub modules.
>
> +config MTK_APU
> + tristate "MediaTek APUSYS Support"
> + select REGMAP
> + select …
[View More]MTK_APU_PM
> + select MTK_SCP
> + help
> + Say yes here to add support for the APU tinysys. The tinsys is
tinysys runs on
> + running on a micro processor in APU.
a microprocessor in the APU.
> + Its firmware is load and boot from Kernel side. Kernel and tinysys use
is loaded and booted
> + IPI to tx/rx messages.
to send/receive messages.
> +
> +config MTK_APU_DEBUG
> + tristate "MediaTek APUSYS debug functions"
> + depends on MTK_APU
> + help
> + Say yes here to enalbe debug on APUSYS.
enable
> + Disable it if you don't need them.
--
~Randy
[View Less]
On Tue, Oct 19, 2021 at 08:23:45PM +0800, guangming.cao(a)mediatek.com wrote:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> Since there is no mandatory inspection for attachments in dma_buf_release.
> There will be a case that dma_buf already released but attachment is still
> in use, which can points to the dmabuf, and it maybe cause
> some unexpected issues.
>
> With IOMMU, when this cases occurs, there will have IOMMU address
> translation fault(…
[View More]s) followed by this warning,
> I think it's useful for dma devices to debug issue.
>
> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com>
This feels a lot like hand-rolling kobject debugging. If you want to do
this then I think adding kobject debug support to
dma_buf/dma_buf_attachment would be better than hand-rolling something
bespoke here.
Also on the patch itself: You don't need the trylock. For correctly
working code non one else can get at the dma-buf, so no locking needed to
iterate through the attachment list. For incorrect code the kernel will be
on fire pretty soon anyway, trying to do locking won't help :-) And
without the trylock we can catch more bugs (e.g. if you also forgot to
unlock and not just forgot to detach).
-Daniel
> ---
> drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 511fe0d217a0..672404857d6a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -74,6 +74,29 @@ static void dma_buf_release(struct dentry *dentry)
> */
> BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
>
> + /* attachment check */
> + if (dma_resv_trylock(dmabuf->resv) && WARN(!list_empty(&dmabuf->attachments),
> + "%s err, inode:%08lu size:%08zu name:%s exp_name:%s flags:0x%08x mode:0x%08x, %s\n",
> + __func__, file_inode(dmabuf->file)->i_ino, dmabuf->size,
> + dmabuf->name, dmabuf->exp_name,
> + dmabuf->file->f_flags, dmabuf->file->f_mode,
> + "Release dmabuf before detach all attachments, dump attach:\n")) {
> + int attach_cnt = 0;
> + dma_addr_t dma_addr;
> + struct dma_buf_attachment *attach_obj;
> + /* dump all attachment info */
> + list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
> + dma_addr = (dma_addr_t)0;
> + if (attach_obj->sgt)
> + dma_addr = sg_dma_address(attach_obj->sgt->sgl);
> + pr_err("attach[%d]: dev:%s dma_addr:0x%-12lx\n",
> + attach_cnt, dev_name(attach_obj->dev), dma_addr);
> + attach_cnt++;
> + }
> + pr_err("Total %d devices attached\n\n", attach_cnt);
> + dma_resv_unlock(dmabuf->resv);
> + }
> +
> dmabuf->ops->release(dmabuf);
>
> if (dmabuf->resv == (struct dma_resv *)&dmabuf[1])
> --
> 2.17.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[View Less]
Hello Guangming, Christian,
On Tue, 12 Oct 2021, 14:09 , <guangming.cao(a)mediatek.com> wrote:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> > Am 09.10.21 um 07:55 schrieb guangming.cao(a)mediatek.com:
> > From: Guangming Cao <Guangming.Cao(a)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,
> > > …
[View More]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(a)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;
> > > }
> > >
>
[View Less]
Am 09.10.21 um 07:55 schrieb guangming.cao(a)mediatek.com:
> From: Guangming Cao <Guangming.Cao(a)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 …
[View More]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(a)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;
> }
>
[View Less]