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

[Timur Tabi]

On Wed, 2024-07-31 at 16:54 +0200, Danilo Krummrich wrote:
> > 
> > gsp_logs.shutdown(&gsp_logs) -- are you sure you want this?  This is some
> > weird C++ wanna-be code, IMHO.  I don't think this is an improvement.  I'd
> > rather keep it as-is.
> 
> That's why I wrote "maybe a bit too sneaky". :)
> 
> I think what I asked for originally in one of the last versions of this patch
> is having both, struct nvif_log (exactly the way you have it) and a separate
> struct nvif_logs:
> 
> 	struct nvif_logs {
> 		struct list_head head;
> 	};
> 
> Then you use this in NVIF_LOGS_DECLARE() and nvif_log_shutdown()
> 
> 	static inline void nvif_log_shutdown(struct nvif_logs *logs)
> 
> and in nouveau_drm_exit() you just pass &gsp_logs.
> 
> 	nvif_log_shutdown(&gsp_logs);
> 
> This way things are more type safe, i.e. nvif_log_shutdown() can't be called
> with a random list_head and struct nvif_log::shutdown can't be called with the
> "head instance" of struct struct nvif_log.

Ok, I think I got it this time.  I'll post a v7 soon.

> > However, I think it can happen.  drm_core_init() does not fail if
> > debugfs_create_dir() fails:
> > 
> > 	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> > 
> > It doesn't even print a warning message.  It just keeps going.  So I think
> > there should be some error checking somewhere.
> 
> For DRM probably not, since the ERR_PTR is honored by other debugfs functions
> as described in [1].

From that comment:

 * Drivers should generally work fine even if debugfs fails to init anyway.

So technically you are correct, that Nouveau will still "work" if debugfs
fails to init, but since this code is all about debugfs, and since I don't
want to blindly allocate buffers and linked lists that won't actually do
anything, I would prefer that the code bails early if the infrastructure is
not there.

> > I tested this, and if drm_core_init() fails to create the dentry, then
> > r535_gsp_retain_logging() will just keep going trying to create debugfs
> > entries, because a root of NULL is actually valid, and the entries are
> > created in /sys/kernel/debug/0000:65:00.0/ instead of
> > /sys/kernel/debug/dri/0000:65:00.0/
> 
> This is because debugfs_lookup() doesn't return an ERR_PTR, but NULL. It'd
> probably better go along with what is documented in [1] if debugfs_lookup()
> would return ERR_PTR(-ENOENT) if no entry was found.
> 
> (This is where I was heading to in my previous reply.)

So I'm not sure what you're asking now.  I definitely think that the "if
(!root)" check is necessary, because we don't want to accidentally create
these debugfs entries in /sys/kernel/debug/0000:65:00.0/.

So that leaves the error checks for debugfs_create_dir() and
debugfs_create_blob() in r535_gsp_copy_log().  Both of these functions could
fail. 

If I ignore the error from debugfs_create_dir(), then the code will allocate
buffers that are never used, and make false statements about the existence
of them.

Same thing is true with debugfs_create_blob().

	dir = debugfs_create_blob(name, 0444, parent, d);
	if (IS_ERR(dir)) {
		kfree(d->data);
		memset(d, 0, sizeof(*d));
		return PTR_ERR(dir);
	}

The one thing I could do is that is ignore errors from r535_gsp_copy_log(),
and just blindly try to create all logs even if some of them fail.  I can't
imagine a situation where create the loginit debugfs entry could fail, but
then creating logintr succeeds.
> 

> > In fact, I think I found a small bug in dput():
> > 
> > void dput(struct dentry *dentry)
> > {
> > 	if (!dentry)
> > 		return;
> > 
> > This should probably be "if (IS_ERR_OR_NULL(dentry))".
> 
> Yes, I agree, good catch.

I will submit a separate patch for that.