[PATCH 2/2] [v8] drm/nouveau: expose GSP-RM logging buffers via debugfs

[Timur Tabi]

On Thu, 2024-10-03 at 23:48 +0200, Danilo Krummrich wrote:
> > +#ifdef CONFIG_DEBUG_FS
> > +	/*
> > +	 * Logging buffers in debugfs. The wrapper objects need to remain
> > +	 * in memory until the dentry is deleted.
> > +	 */
> > +	struct dentry *dir;		/* Parent dentry */
> > +	struct dentry *dir_init;
> > +	struct dentry *dir_rm;
> > +	struct dentry *dir_intr;
> > +	struct dentry *dir_pmu;
> 
> I think `dir` is confusing, maybe just `dent_*`? Or maybe just wrap all those in
> a `struct { ... } debugfs;` and just name them `init`, `rm`, etc.?

Ok, but I'm pretty sure this is like the fifth time I've moved these fields
around because you keep telling me to put them somewhere else.

> > +/*
> > + * The debugfs root for all devices.  Normally we'd use /sys/kernel/debug/dri.,
> > + * but that path may not exist when we need it.  So create a new root
> > + * /sys/kernel/debug/nouveau/.
> > + *
> > + * We take a reference every time a dentry under the root is created.  When
> > + * the last reference is released, the root is deleted.
> > + */
> > +static struct {
> > +	struct mutex mutex; /* Protects dentry */
> > +	struct dentry *dentry;
> > +	struct kref kref;
> > +} root = {
> > +	.mutex = __MUTEX_INITIALIZER(root.mutex),
> > +	.kref = KREF_INIT(0),
> > +	.dentry = NULL,
> > +};
> > +
> > +static void release_root_dentry(struct kref *ref)
> > +{
> > +	mutex_lock(&root.mutex);
> > +	debugfs_remove(root.dentry);
> > +	root.dentry = NULL;
> > +	mutex_unlock(&root.mutex);
> > +}
> 
> I think all this should go into module_init() and module_exit(), then you don't
> need the mutex and all the reference counts.

Sorry, I don't see how I can just move "all this" to module_init and exit. 
The point is to keep the parent dentry around until the last GPU has shut
down in r535_debugfs_shutdown().  

Please tell me what you think module_init() and module_exit() will look
like.

> > +/**
> > + * create_debufgs - create a blob debugfs entry
> > + * @name: filename
> > + * @parent: parent
> > + * @blob: blob
> > + *
> > + * Creates a debugfs entry for a logging buffer with the name 'name'.
> > + */
> > +static struct dentry *create_debugfs(struct nvkm_gsp *gsp, const char *name,
> > +				     struct debugfs_blob_wrapper *blob)
> > +{
> > +	struct dentry *dir;
> 
> I think `dir` is confusing, what about `dent` or `entry`?

Here's a count of the most popular names for this type:

     10 	struct dentry *ddir;
     18 	struct dentry *d;
     21 	struct dentry *debugfs_root;
     31 	struct dentry *dbgfs_dir;
     39 	struct dentry *debugfs_dir;
     50 	struct dentry *dentry;
     55 	struct dentry *debugfs;
     55 	struct dentry *dir;
     64 	struct dentry *root;

As you can see, 'dir' is the second most popular name.  So I think it's
fine.

Besides, couldn't you have make this suggestion during one of the previous 7
versions of this patch?  I'm never going to get this patch finished if you
keep asking for cosmetic changes.

> > +	dir = debugfs_create_blob(name, 0444, gsp->dir, blob);
> > +	if (IS_ERR(dir)) {
> > +		nvkm_error(&gsp->subdev,
> > +			   "failed to create %s debugfs entry\n", name);
> > +		return NULL;
> > +	}
> > +
> > +	/*
> > +	 * For some reason, debugfs_create_blob doesn't set the size of the
> > +	 * dentry, so do that here.
> > +	 */
> > +	i_size_write(d_inode(dir), blob->size);
> 
> I think debugfs entries typically don't need this. Do we need it?

Yes.  I submitted a patch to debugfs earlier this year that would fix it for
all debugfs blobs, but it was rejected because I was asked to fix all of
debugfs itself, which I wasn't willing to do.

https://www.spinics.net/lists/linux-fsdevel/msg262843.html

> > +	gsp->dir_init = create_debugfs(gsp, "loginit", &gsp->blob_init);
> 
> Here you use your helper, but for...
> 
> > +	if (!gsp->dir_init)
> > +		goto error;
> > +
> > +	gsp->dir_intr = debugfs_create_blob("logintr", 0444, gsp->dir, &gsp->blob_intr);
> > +	if (IS_ERR(gsp->dir_intr)) {
> > +		nvkm_error(&gsp->subdev, "failed to create logintr debugfs entry\n");
> > +		goto error;
> > +	}
> > +
> > +	gsp->dir_rm = debugfs_create_blob("logrm", 0444, gsp->dir, &gsp->blob_rm);
> > +	if (IS_ERR(gsp->dir_rm)) {
> > +		nvkm_error(&gsp->subdev, "failed to create logrm debugfs entry\n");
> > +		goto error;
> > +	}
> > +
> > +	/*
> > +	 * Since the PMU buffer is copied from an RPC, it doesn't need to be
> > +	 * a DMA buffer.
> > +	 */
> > +	gsp->blob_pmu.size = GSP_PAGE_SIZE;
> > +	gsp->blob_pmu.data = kzalloc(gsp->blob_pmu.size, GFP_KERNEL);
> > +	if (!gsp->blob_pmu.data)
> > +		goto error;
> > +
> > +	gsp->dir_pmu = debugfs_create_blob("logpmu", 0444, gsp->dir, &gsp->blob_pmu);
> > +	if (IS_ERR(gsp->dir_pmu)) {
> > +		nvkm_error(&gsp->subdev, "failed to create logpmu debugfs entry\n");
> > +		kfree(gsp->blob_pmu.data);
> > +		goto error;
> > +	}
> > +
> > +	i_size_write(d_inode(gsp->dir_init), gsp->blob_init.size);
> > +	i_size_write(d_inode(gsp->dir_intr), gsp->blob_intr.size);
> > +	i_size_write(d_inode(gsp->dir_rm), gsp->blob_rm.size);
> > +	i_size_write(d_inode(gsp->dir_pmu), gsp->blob_pmu.size);
> 
> ...all those, it seems you forgot to switch to your helper.

Oops.  I think I've been working on this patch too long.

I will post a v10 with struct { ... } debugfs and create_debugfs fixes.  But
I will need your help with the module_init/exit change if you still want
that.