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.
