| Message ID | 1462441630-681-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 10:47:10AM +0100, Chris Wilson wrote: > If we fail to setup the debugfs we lose debugging convenience, but we > should still endeavour to enable modesetting so that we can control the > outputs and enable use of the system to debug the issue. In all > likelihood if we can not create our debugfs files then there is a larger > underlying issue that will prevent the module loading, but this should > get us further towards resilience. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: dri-devel@lists.freedesktop.org Usually the bug is that someone forgot to clean up debugfs files on module unload, and there's gunk left behind, and that's causing the failure to load. Since you can't ever recover from leaking debugfs files in this fashion you need to reboot anyway to make sure your patches are fixed properly. So I don't see the value of this all that much ... -Daniel > --- > drivers/gpu/drm/drm_debugfs.c | 13 +++++++++---- > drivers/gpu/drm/drm_drv.c | 19 ++++++++----------- > 2 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index 3bcf8e6a85b3..8f36e014fbd2 100644 > --- a/drivers/gpu/drm/drm_debugfs.c > +++ b/drivers/gpu/drm/drm_debugfs.c > @@ -160,10 +160,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, > ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, > minor->debugfs_root, minor); > if (ret) { > - debugfs_remove(minor->debugfs_root); > - minor->debugfs_root = NULL; > DRM_ERROR("Failed to create core drm debugfs files\n"); > - return ret; > + goto err_root; > } > > if (dev->driver->debugfs_init) { > @@ -171,10 +169,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, > if (ret) { > DRM_ERROR("DRM: Driver failed to initialize " > "/sys/kernel/debug/dri.\n"); > - return ret; > + goto err_core; > } > } > return 0; > + > +err_core: > + drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor); > +err_root: > + debugfs_remove(minor->debugfs_root); > + minor->debugfs_root = NULL; > + return ret; > } > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index bff89226a344..18de82e4388a 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -307,11 +307,11 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > if (!minor) > return 0; > > - ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root); > - if (ret) { > + ret = -ENOENT; > + if (drm_debugfs_root) > + ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root); > + if (ret) > DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n"); > - return ret; > - } > > ret = device_add(minor->kdev); > if (ret) > @@ -904,17 +904,13 @@ static int __init drm_core_init(void) > } > > drm_debugfs_root = debugfs_create_dir("dri", NULL); > - if (!drm_debugfs_root) { > + if (!drm_debugfs_root) > DRM_ERROR("Cannot create /sys/kernel/debug/dri\n"); > - ret = -1; > - goto err_p3; > - } > > DRM_INFO("Initialized %s %d.%d.%d %s\n", > CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); > return 0; > -err_p3: > - drm_sysfs_destroy(); > + > err_p2: > unregister_chrdev(DRM_MAJOR, "drm"); > > @@ -925,7 +921,8 @@ err_p1: > > static void __exit drm_core_exit(void) > { > - debugfs_remove(drm_debugfs_root); > + if (drm_debugfs_root) > + debugfs_remove(drm_debugfs_root); > drm_sysfs_destroy(); > > unregister_chrdev(DRM_MAJOR, "drm"); > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, May 05, 2016 at 01:53:57PM +0200, Daniel Vetter wrote: > On Thu, May 05, 2016 at 10:47:10AM +0100, Chris Wilson wrote: > > If we fail to setup the debugfs we lose debugging convenience, but we > > should still endeavour to enable modesetting so that we can control the > > outputs and enable use of the system to debug the issue. In all > > likelihood if we can not create our debugfs files then there is a larger > > underlying issue that will prevent the module loading, but this should > > get us further towards resilience. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: dri-devel@lists.freedesktop.org > > Usually the bug is that someone forgot to clean up debugfs files on module > unload, and there's gunk left behind, and that's causing the failure to > load. Since you can't ever recover from leaking debugfs files in this > fashion you need to reboot anyway to make sure your patches are fixed > properly. So I don't see the value of this all that much ... The point is to get a console, even GUI, up and running to be able to reboot. Everything that is not fatal to loading the driver should not prevent the driver from loading. -Chris
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 3bcf8e6a85b3..8f36e014fbd2 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -160,10 +160,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor->debugfs_root, minor); if (ret) { - debugfs_remove(minor->debugfs_root); - minor->debugfs_root = NULL; DRM_ERROR("Failed to create core drm debugfs files\n"); - return ret; + goto err_root; } if (dev->driver->debugfs_init) { @@ -171,10 +169,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, if (ret) { DRM_ERROR("DRM: Driver failed to initialize " "/sys/kernel/debug/dri.\n"); - return ret; + goto err_core; } } return 0; + +err_core: + drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor); +err_root: + debugfs_remove(minor->debugfs_root); + minor->debugfs_root = NULL; + return ret; } diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index bff89226a344..18de82e4388a 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -307,11 +307,11 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) if (!minor) return 0; - ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root); - if (ret) { + ret = -ENOENT; + if (drm_debugfs_root) + ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root); + if (ret) DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n"); - return ret; - } ret = device_add(minor->kdev); if (ret) @@ -904,17 +904,13 @@ static int __init drm_core_init(void) } drm_debugfs_root = debugfs_create_dir("dri", NULL); - if (!drm_debugfs_root) { + if (!drm_debugfs_root) DRM_ERROR("Cannot create /sys/kernel/debug/dri\n"); - ret = -1; - goto err_p3; - } DRM_INFO("Initialized %s %d.%d.%d %s\n", CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); return 0; -err_p3: - drm_sysfs_destroy(); + err_p2: unregister_chrdev(DRM_MAJOR, "drm"); @@ -925,7 +921,8 @@ err_p1: static void __exit drm_core_exit(void) { - debugfs_remove(drm_debugfs_root); + if (drm_debugfs_root) + debugfs_remove(drm_debugfs_root); drm_sysfs_destroy(); unregister_chrdev(DRM_MAJOR, "drm");
If we fail to setup the debugfs we lose debugging convenience, but we should still endeavour to enable modesetting so that we can control the outputs and enable use of the system to debug the issue. In all likelihood if we can not create our debugfs files then there is a larger underlying issue that will prevent the module loading, but this should get us further towards resilience. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_debugfs.c | 13 +++++++++---- drivers/gpu/drm/drm_drv.c | 19 ++++++++----------- 2 files changed, 17 insertions(+), 15 deletions(-)