Am 17.02.23 um 20:38 schrieb Daniel Vetter:
On Fri, Feb 17, 2023 at 11:01:18AM +0100, Stanislaw Gruszka wrote:
On Fri, Feb 17, 2023 at 10:22:25AM +0100, Christian König wrote:
Am 16.02.23 um 20:54 schrieb Daniel Vetter:
On Thu, Feb 16, 2023 at 07:08:49PM +0200, Jani Nikula wrote:
On Thu, 16 Feb 2023, Christian König <christian.koenig@xxxxxxx> wrote:
Am 16.02.23 um 17:46 schrieb Jani Nikula:
On Thu, 16 Feb 2023, Christian König <christian.koenig@xxxxxxx> wrote:
Am 16.02.23 um 12:33 schrieb Daniel Vetter:
On Thu, Feb 09, 2023 at 09:18:38AM +0100, Christian König wrote:
The mutex was completely pointless in the first place since any
parallel adding of files to this list would result in random
behavior since the list is filled and consumed multiple times.
Completely drop that approach and just create the files directly.
This also re-adds the debugfs files to the render node directory and
removes drm_debugfs_late_register().
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
drivers/gpu/drm/drm_debugfs.c | 32 +++++++------------------------
drivers/gpu/drm/drm_drv.c | 3 ---
drivers/gpu/drm/drm_internal.h | 5 -----
drivers/gpu/drm/drm_mode_config.c | 2 --
include/drm/drm_device.h | 15 ---------------
5 files changed, 7 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 558e3a7271a5..a40288e67264 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -246,31 +246,9 @@ void drm_debugfs_dev_register(struct drm_device *dev)
void drm_debugfs_minor_register(struct drm_minor *minor)
{
struct drm_device *dev = minor->dev;
- struct drm_debugfs_entry *entry, *tmp;
if (dev->driver->debugfs_init)
dev->driver->debugfs_init(minor);
-
- list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
- debugfs_create_file(entry->file.name, 0444,
- minor->debugfs_root, entry, &drm_debugfs_entry_fops);
- list_del(&entry->list);
- }
-}
-
-void drm_debugfs_late_register(struct drm_device *dev)
-{
- struct drm_minor *minor = dev->primary;
- struct drm_debugfs_entry *entry, *tmp;
-
- if (!minor)
- return;
-
- list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
- debugfs_create_file(entry->file.name, 0444,
- minor->debugfs_root, entry, &drm_debugfs_entry_fops);
- list_del(&entry->list);
- }
}
int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
@@ -343,9 +321,13 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
entry->file.data = data;
entry->dev = dev;
- mutex_lock(&dev->debugfs_mutex);
- list_add(&entry->list, &dev->debugfs_list);
- mutex_unlock(&dev->debugfs_mutex);
+ debugfs_create_file(name, 0444, dev->primary->debugfs_root, entry,
+ &drm_debugfs_entry_fops);
+
+ /* TODO: This should probably only be a symlink */
+ if (dev->render)
+ debugfs_create_file(name, 0444, dev->render->debugfs_root,
+ entry, &drm_debugfs_entry_fops);
Nope. You are fundamentally missing the point of all this, which is:
- drivers create debugfs files whenever they want to, as long as it's
_before_ drm_dev_register is called.
- drm_dev_register will set them all up.
This is necessary because otherwise you have the potential for some nice
oops and stuff when userspace tries to access these files before the
driver is ready.
Note that with sysfs all this infrastructure already exists, which is why
you can create sysfs files whenever you feel like, and things wont go
boom.
Well Yeah I've considered that, I just don't think it's a good idea for
debugfs.
debugfs is meant to be a helper for debugging things and that especially
includes the time between drm_dev_init() and drm_dev_register() because
that's where we probe the hardware and try to get it working.
Not having the debugfs files which allows for things like hardware
register access and reading internal state during that is a really and I
mean REALLY bad idea. This is essentially what we have those files for.
So you mean you want to have early debugfs so you can have some script
hammering the debugfs to get info out between init and register during
probe?
Well not hammering. What we usually do in bringup is to set firmware
timeout to infinity and the driver then sits and waits for the hw.
The tool used to access registers then goes directly through the PCI bar
at the moment, but that's essentially a bad idea for registers which you
grab a lock for to access (like index/data).
I just think registering debugfs before everything is ready is a recipe
for disaster. All of the debugfs needs to check all the conditions that
they need across all of the probe stages. It'll be difficult to get it
right. And you'll get cargo culted checks copy pasted all over the
place.
Yeah, but it's debugfs. That is not supposed to work under all conditions.
Just try to read amdgpu_regs on a not existing register index. This will
just hang or reboot your box immediately on APUs.
I'm firmly in the camp that debugfs does not need to work under all
conditions, but that it must fail gracefully instead of crashing.
Yeah I mean once we talk bring-up, you can just hand-roll the necessary
bring debugfs things that you need to work before the driver is ready to
do anything.
But bring-up debugfs fun is rather special, same way pre-silicon support
tends to be rather special. Shipping that in distros does not sound like a
good idea at all to me.
Yeah, that's indeed a really good point.
I can't remember how often I had to note that module parameters would also
be used by end users.
How about if the create the debugfs directory with a "." as name prefix
first and then rename it as soon as the device is registered?
Good idea. Or the dir could have this drm_dev->unique name and be created
during alloc, and link in minor created during registration. That would
mean minor link is safe to use and unique potentially dangerous before
registration.
Alternatively
we could clear the i_mode of the directory.
I checked that yesterday and this does not prevent to access the file
for root user. Perhaps there is other smart way for blocking
root access in vfs just by modifying some inode field, but just
'chmod 0000 file' does not prevent that.
If a power user or engineer wants to debug startup problems stuff it should
be trivial to work around that from userspace, and if people do such things
they should also know the potential consequences.
Fully agree.
So what about a drm module option instead (that taints the kernel as usual
for these), which:
- registers the debugfs dir right away
- registers any debugfs files as soon as they get populated, instead of
postponing until drm_dev_register
Yeah, works for me as well.
It would only neatly work with the add_file stuff, but I guess drivers
could still hand-roll this if needed.
I think funny games with trying to hide the files while not hiding them is
not a great idea, and explicit "I'm debugging stuff, please stand back"
knob sounds much better to me.
Well the challenge is that we have to consider the whole spectrum of end
users for this. This reaches from the grandmother which just tries every
possible random knob to get her printer working again over the script
kiddie all the wait to the power users and engineers.
Some option to give an experience level to module parameters would be
rather helpful.
Christian.
-Daniel
Regards
Stanislaw
