On Tue, 8 Apr 2025 14:38:44 +0100
Adrián Larumbe <adrian.laru...@collabora.com> wrote:
> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c
> > > b/drivers/gpu/drm/panthor/panthor_gem.c
> > > index 44d027e6d664..2fc87be9b700 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > > @@ -2,6 +2,7 @@
> > > /* Copyright 2019 Linaro, Ltd, Rob Herring <r...@kernel.org> */
> > > /* Copyright 2023 Collabora ltd. */
> > >
> > > +#include <linux/cleanup.h>
> > > #include <linux/dma-buf.h>
> > > #include <linux/dma-mapping.h>
> > > #include <linux/err.h>
> > > @@ -10,14 +11,65 @@
> > > #include <drm/panthor_drm.h>
> > >
> > > #include "panthor_device.h"
> > > +#include "panthor_fw.h"
> > > #include "panthor_gem.h"
> > > #include "panthor_mmu.h"
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo,
> > > u32 type_mask)
> > > +{
> > > + INIT_LIST_HEAD(&bo->debugfs.node);
> >
> > This should be called when the GEM object is created, otherwise the
> > list_empty() test done in panthor_gem_debugfs_bo_rm() will only work if
> > panthor_gem_debugfs_bo_add() is called, and depending on when this
> > happens, or whether it happens at all, the error path will do a NULL
> > deref.
>
> I'll be moving panthor_gem_debugfs_bo_add() back into
> panthor_gem_create_object() and
> inline panthor_gem_debugfs_bo_init() into it.
You mean moving the panthor_gem_debugfs_bo_add() call to
panthor_gem_create_object(), not inlining its content, right?
> > > + } else {
> > > + bo->debugfs.creator.tgid = 0;
> > > + snprintf(bo->debugfs.creator.process_name,
> > > + sizeof(bo->debugfs.creator.process_name),
> > > + "kernel");
> > > + }
> > > +
> > > + bo->debugfs.bo_mask = type_mask;
> >
> > Why not do that directly in panthor_gem_debugfs_bo_add()? The only bits
> > that might be useful to do early is the INIT_LIST_HEAD(), and I think
> > it can be inlined in panthor_gem_create_object().
>
> I'll be doing in this in the next revision, but because I've no access to the
> BO
> type mask from inside Panthor's drm_driver::gem_create_object() binding, then
> I'll have to assign the mask right after the object has been created.
>
> I think this means there might be a short window after the object's been
> added to
> the DebugFS GEMs list in which it could be shown with the kernel mask field
> still
> set to 0, but I guess that's not too important either.
I think it's okay, as long as you don't crash when printing partially
initialized objects. Another solution would be to have a flag encoding
when the obj is initialized, so you can skip objects that don't have
this flag set yet.
