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

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Oct 29, 2024 at 07:50:06PM +0000, Timur Tabi wrote:
> 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.

You added those fields in *this* version, so I don't see how that makes any
sense.

If you don't want to rename them -- fine too. It was a suggestion, and since
you'll need a v9 anyways it doesn't seem like too much to ask for.

> 
> > > +/*
> > > + * 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.

You can create the root debugfs entry in module_init() and remove it in
module_exit() and make it available as a global. Then you don't need any mutex
and reference count.

Please let me know if anything is still unclear or if I miss anything.

> 
> > > +/**
> > > + * 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.

Being the second most popular name doesn't make it a good name. I wonder how
often people use "password" as their password...

> 
> 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.

Agreed, however, I think it got my attention, since you added all those

+	struct dentry *dir;		/* Parent dentry */
+	struct dentry *dir_init;
+	struct dentry *dir_rm;
+	struct dentry *dir_intr;
+	struct dentry *dir_pmu;

to struct nvkm_gsp in *this* version.

Feel free to keep it, but as mentioned above, you need a v9 anyways, so it also
shouldn't be a big deal to change those.

Please also note that none of the revisions you have been sending were motivated
by "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

I see...

Can you please add a very brief comment and this [1] link?

[1] https://lore.kernel.org/r/linux-fsdevel/20240207200619.3354549-1-ttabi@nvidia.com/

> 
> > > +	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.
>