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

[Danilo Krummrich]

On Wed, Jul 31, 2024 at 04:22:10PM +0000, Timur Tabi wrote:
> 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

That's not what I said, I said that the DRM APIs probably don't need to handle
the error.

> 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 do not disagree. Having the NULL check of debugfs_lookup() and bail out is
the right thing to do.

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

That's because I'm not asking for anything. :) I'm just saying that
debugfs_lookup() seems a bit inconsistent given the other documentation.

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