| Message ID | 20230412145206.2483-4-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/debugfs: drop debugfs_init() for the render and accel node v2 | expand |
On Wed, Apr 12, 2023 at 04:52:04PM +0200, 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 but > return -ENODEV while opening the file when the minors are not > registered yet. This part seems to be missing? The other issue is that partial driver load cleanup now goes boom I think, you need to both remove them all in _unregister but also in final drm_dev cleanup. Or I'm missing how this works. Maybe you could also use drmm_ for that to make sure it's always done. There shouldn't be a race as long as the "are we registered yet" check is there, since in that case the driver never registered. So no problems trying to access the files when they could try to access things which are already cleaned up. I think with those details ironed out this makes some sense to me, assuming I understood it correctly. > > v2: rebase on debugfs directory rework, limit access before minors are > registered. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_debugfs.c | 27 ++------------------------- > 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, 2 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index 59c3d76d28f4..427c5eb4eca0 100644 > --- a/drivers/gpu/drm/drm_debugfs.c > +++ b/drivers/gpu/drm/drm_debugfs.c > @@ -245,7 +245,6 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, > struct dentry *root) > { > struct drm_device *dev = minor->dev; > - struct drm_debugfs_entry *entry, *tmp; > char name[64]; > > INIT_LIST_HEAD(&minor->debugfs_list); > @@ -260,30 +259,9 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, > if (dev->driver->debugfs_init && dev->render != minor) > 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); > - } > - > return 0; > } > > -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, > struct drm_minor *minor) > { > @@ -353,9 +331,8 @@ 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->debugfs_root, entry, > + &drm_debugfs_entry_fops); > } > EXPORT_SYMBOL(drm_debugfs_add_file); > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 05b831f9fe71..f928b4490ece 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -597,7 +597,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) > mutex_destroy(&dev->clientlist_mutex); > mutex_destroy(&dev->filelist_mutex); > mutex_destroy(&dev->struct_mutex); > - mutex_destroy(&dev->debugfs_mutex); > drm_legacy_destroy_members(dev); > } > > @@ -638,14 +637,12 @@ static int drm_dev_init(struct drm_device *dev, > INIT_LIST_HEAD(&dev->filelist_internal); > INIT_LIST_HEAD(&dev->clientlist); > INIT_LIST_HEAD(&dev->vblank_event_list); > - INIT_LIST_HEAD(&dev->debugfs_list); > > spin_lock_init(&dev->event_lock); > mutex_init(&dev->struct_mutex); > mutex_init(&dev->filelist_mutex); > mutex_init(&dev->clientlist_mutex); > mutex_init(&dev->master_mutex); > - mutex_init(&dev->debugfs_mutex); > > ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL); > if (ret) > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 5c6e1cadf09b..c5fadbd3fd7d 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -185,7 +185,6 @@ void drm_debugfs_dev_register(struct drm_device *dev); > int drm_debugfs_register(struct drm_minor *minor, int minor_id, > struct dentry *root); > void drm_debugfs_cleanup(struct drm_minor *minor); > -void drm_debugfs_late_register(struct drm_device *dev); > void drm_debugfs_connector_add(struct drm_connector *connector); > void drm_debugfs_connector_remove(struct drm_connector *connector); > void drm_debugfs_crtc_add(struct drm_crtc *crtc); > @@ -206,10 +205,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor) > { > } > > -static inline void drm_debugfs_late_register(struct drm_device *dev) > -{ > -} > - > static inline void drm_debugfs_connector_add(struct drm_connector *connector) > { > } > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 87eb591fe9b5..8525ef851540 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev) > if (ret) > goto err_connector; > > - drm_debugfs_late_register(dev); > - > return 0; > > err_connector: > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 3cf12b14232d..c490977ee250 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -318,21 +318,6 @@ struct drm_device { > */ > struct dentry *debugfs_root; > > - /** > - * @debugfs_mutex: > - * > - * Protects &debugfs_list access. > - */ > - struct mutex debugfs_mutex; > - > - /** > - * @debugfs_list: > - * > - * List of debugfs files to be created by the DRM device. The files > - * must be added during drm_dev_register(). > - */ > - struct list_head debugfs_list; > - > /* Everything below here is for legacy driver, never use! */ > /* private: */ > #if IS_ENABLED(CONFIG_DRM_LEGACY) > -- > 2.34.1 >
Am 12.04.23 um 17:11 schrieb Daniel Vetter: > On Wed, Apr 12, 2023 at 04:52:04PM +0200, 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 but >> return -ENODEV while opening the file when the minors are not >> registered yet. > This part seems to be missing? I wanted to move this into a separate patch and then forgot to add it. Going to fix that. > The other issue is that partial driver load cleanup now goes boom I think, > you need to both remove them all in _unregister but also in final drm_dev > cleanup. Or I'm missing how this works. Maybe you could also use drmm_ for > that to make sure it's always done. Good point. Should we call the cleanup function from both the unregister and the final drm_dev cleanup or just the later? Thanks, Christian. > > There shouldn't be a race as long as the "are we registered yet" check is > there, since in that case the driver never registered. So no problems > trying to access the files when they could try to access things which are > already cleaned up. > > I think with those details ironed out this makes some sense to me, > assuming I understood it correctly. > >> v2: rebase on debugfs directory rework, limit access before minors are >> registered. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/drm_debugfs.c | 27 ++------------------------- >> 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, 2 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c >> index 59c3d76d28f4..427c5eb4eca0 100644 >> --- a/drivers/gpu/drm/drm_debugfs.c >> +++ b/drivers/gpu/drm/drm_debugfs.c >> @@ -245,7 +245,6 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, >> struct dentry *root) >> { >> struct drm_device *dev = minor->dev; >> - struct drm_debugfs_entry *entry, *tmp; >> char name[64]; >> >> INIT_LIST_HEAD(&minor->debugfs_list); >> @@ -260,30 +259,9 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, >> if (dev->driver->debugfs_init && dev->render != minor) >> 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); >> - } >> - >> return 0; >> } >> >> -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, >> struct drm_minor *minor) >> { >> @@ -353,9 +331,8 @@ 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->debugfs_root, entry, >> + &drm_debugfs_entry_fops); >> } >> EXPORT_SYMBOL(drm_debugfs_add_file); >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 05b831f9fe71..f928b4490ece 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -597,7 +597,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >> mutex_destroy(&dev->clientlist_mutex); >> mutex_destroy(&dev->filelist_mutex); >> mutex_destroy(&dev->struct_mutex); >> - mutex_destroy(&dev->debugfs_mutex); >> drm_legacy_destroy_members(dev); >> } >> >> @@ -638,14 +637,12 @@ static int drm_dev_init(struct drm_device *dev, >> INIT_LIST_HEAD(&dev->filelist_internal); >> INIT_LIST_HEAD(&dev->clientlist); >> INIT_LIST_HEAD(&dev->vblank_event_list); >> - INIT_LIST_HEAD(&dev->debugfs_list); >> >> spin_lock_init(&dev->event_lock); >> mutex_init(&dev->struct_mutex); >> mutex_init(&dev->filelist_mutex); >> mutex_init(&dev->clientlist_mutex); >> mutex_init(&dev->master_mutex); >> - mutex_init(&dev->debugfs_mutex); >> >> ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL); >> if (ret) >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h >> index 5c6e1cadf09b..c5fadbd3fd7d 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -185,7 +185,6 @@ void drm_debugfs_dev_register(struct drm_device *dev); >> int drm_debugfs_register(struct drm_minor *minor, int minor_id, >> struct dentry *root); >> void drm_debugfs_cleanup(struct drm_minor *minor); >> -void drm_debugfs_late_register(struct drm_device *dev); >> void drm_debugfs_connector_add(struct drm_connector *connector); >> void drm_debugfs_connector_remove(struct drm_connector *connector); >> void drm_debugfs_crtc_add(struct drm_crtc *crtc); >> @@ -206,10 +205,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor) >> { >> } >> >> -static inline void drm_debugfs_late_register(struct drm_device *dev) >> -{ >> -} >> - >> static inline void drm_debugfs_connector_add(struct drm_connector *connector) >> { >> } >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >> index 87eb591fe9b5..8525ef851540 100644 >> --- a/drivers/gpu/drm/drm_mode_config.c >> +++ b/drivers/gpu/drm/drm_mode_config.c >> @@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev) >> if (ret) >> goto err_connector; >> >> - drm_debugfs_late_register(dev); >> - >> return 0; >> >> err_connector: >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index 3cf12b14232d..c490977ee250 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -318,21 +318,6 @@ struct drm_device { >> */ >> struct dentry *debugfs_root; >> >> - /** >> - * @debugfs_mutex: >> - * >> - * Protects &debugfs_list access. >> - */ >> - struct mutex debugfs_mutex; >> - >> - /** >> - * @debugfs_list: >> - * >> - * List of debugfs files to be created by the DRM device. The files >> - * must be added during drm_dev_register(). >> - */ >> - struct list_head debugfs_list; >> - >> /* Everything below here is for legacy driver, never use! */ >> /* private: */ >> #if IS_ENABLED(CONFIG_DRM_LEGACY) >> -- >> 2.34.1 >>
On Thu, Apr 13, 2023 at 11:34:45AM +0200, Christian König wrote: > Am 12.04.23 um 17:11 schrieb Daniel Vetter: > > On Wed, Apr 12, 2023 at 04:52:04PM +0200, 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 but > > > return -ENODEV while opening the file when the minors are not > > > registered yet. > > This part seems to be missing? > > I wanted to move this into a separate patch and then forgot to add it. Going > to fix that. > > > The other issue is that partial driver load cleanup now goes boom I think, > > you need to both remove them all in _unregister but also in final drm_dev > > cleanup. Or I'm missing how this works. Maybe you could also use drmm_ for > > that to make sure it's always done. > > Good point. Should we call the cleanup function from both the unregister and > the final drm_dev cleanup or just the later? I think both: - unregister to make sure userspace can't access the files when the driver is unloading, because the _remove() is the only thing that actually synchronizes with parallel read/write() and waits for those to finish. Really don't want to hand-roll that, it's a lot more tricky than the "are we registered already" check :-) - As cleanup for the case we never registered, and hence drm_dev_unregister wont be called. I guess if you make both the exact same function and that unconditionally (any checks needed within that function) it hopefully aviods the not-registered error path from bitrotting. -Daniel > > Thanks, > Christian. > > > > > There shouldn't be a race as long as the "are we registered yet" check is > > there, since in that case the driver never registered. So no problems > > trying to access the files when they could try to access things which are > > already cleaned up. > > > > I think with those details ironed out this makes some sense to me, > > assuming I understood it correctly. > > > > > v2: rebase on debugfs directory rework, limit access before minors are > > > registered. > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > --- > > > drivers/gpu/drm/drm_debugfs.c | 27 ++------------------------- > > > 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, 2 insertions(+), 50 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > > > index 59c3d76d28f4..427c5eb4eca0 100644 > > > --- a/drivers/gpu/drm/drm_debugfs.c > > > +++ b/drivers/gpu/drm/drm_debugfs.c > > > @@ -245,7 +245,6 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, > > > struct dentry *root) > > > { > > > struct drm_device *dev = minor->dev; > > > - struct drm_debugfs_entry *entry, *tmp; > > > char name[64]; > > > INIT_LIST_HEAD(&minor->debugfs_list); > > > @@ -260,30 +259,9 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, > > > if (dev->driver->debugfs_init && dev->render != minor) > > > 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); > > > - } > > > - > > > return 0; > > > } > > > -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, > > > struct drm_minor *minor) > > > { > > > @@ -353,9 +331,8 @@ 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->debugfs_root, entry, > > > + &drm_debugfs_entry_fops); > > > } > > > EXPORT_SYMBOL(drm_debugfs_add_file); > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 05b831f9fe71..f928b4490ece 100644 > > > --- a/drivers/gpu/drm/drm_drv.c > > > +++ b/drivers/gpu/drm/drm_drv.c > > > @@ -597,7 +597,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) > > > mutex_destroy(&dev->clientlist_mutex); > > > mutex_destroy(&dev->filelist_mutex); > > > mutex_destroy(&dev->struct_mutex); > > > - mutex_destroy(&dev->debugfs_mutex); > > > drm_legacy_destroy_members(dev); > > > } > > > @@ -638,14 +637,12 @@ static int drm_dev_init(struct drm_device *dev, > > > INIT_LIST_HEAD(&dev->filelist_internal); > > > INIT_LIST_HEAD(&dev->clientlist); > > > INIT_LIST_HEAD(&dev->vblank_event_list); > > > - INIT_LIST_HEAD(&dev->debugfs_list); > > > spin_lock_init(&dev->event_lock); > > > mutex_init(&dev->struct_mutex); > > > mutex_init(&dev->filelist_mutex); > > > mutex_init(&dev->clientlist_mutex); > > > mutex_init(&dev->master_mutex); > > > - mutex_init(&dev->debugfs_mutex); > > > ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL); > > > if (ret) > > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > > > index 5c6e1cadf09b..c5fadbd3fd7d 100644 > > > --- a/drivers/gpu/drm/drm_internal.h > > > +++ b/drivers/gpu/drm/drm_internal.h > > > @@ -185,7 +185,6 @@ void drm_debugfs_dev_register(struct drm_device *dev); > > > int drm_debugfs_register(struct drm_minor *minor, int minor_id, > > > struct dentry *root); > > > void drm_debugfs_cleanup(struct drm_minor *minor); > > > -void drm_debugfs_late_register(struct drm_device *dev); > > > void drm_debugfs_connector_add(struct drm_connector *connector); > > > void drm_debugfs_connector_remove(struct drm_connector *connector); > > > void drm_debugfs_crtc_add(struct drm_crtc *crtc); > > > @@ -206,10 +205,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor) > > > { > > > } > > > -static inline void drm_debugfs_late_register(struct drm_device *dev) > > > -{ > > > -} > > > - > > > static inline void drm_debugfs_connector_add(struct drm_connector *connector) > > > { > > > } > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > > index 87eb591fe9b5..8525ef851540 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev) > > > if (ret) > > > goto err_connector; > > > - drm_debugfs_late_register(dev); > > > - > > > return 0; > > > err_connector: > > > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > > > index 3cf12b14232d..c490977ee250 100644 > > > --- a/include/drm/drm_device.h > > > +++ b/include/drm/drm_device.h > > > @@ -318,21 +318,6 @@ struct drm_device { > > > */ > > > struct dentry *debugfs_root; > > > - /** > > > - * @debugfs_mutex: > > > - * > > > - * Protects &debugfs_list access. > > > - */ > > > - struct mutex debugfs_mutex; > > > - > > > - /** > > > - * @debugfs_list: > > > - * > > > - * List of debugfs files to be created by the DRM device. The files > > > - * must be added during drm_dev_register(). > > > - */ > > > - struct list_head debugfs_list; > > > - > > > /* Everything below here is for legacy driver, never use! */ > > > /* private: */ > > > #if IS_ENABLED(CONFIG_DRM_LEGACY) > > > -- > > > 2.34.1 > > > >
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 59c3d76d28f4..427c5eb4eca0 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -245,7 +245,6 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, struct dentry *root) { struct drm_device *dev = minor->dev; - struct drm_debugfs_entry *entry, *tmp; char name[64]; INIT_LIST_HEAD(&minor->debugfs_list); @@ -260,30 +259,9 @@ int drm_debugfs_register(struct drm_minor *minor, int minor_id, if (dev->driver->debugfs_init && dev->render != minor) 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); - } - return 0; } -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, struct drm_minor *minor) { @@ -353,9 +331,8 @@ 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->debugfs_root, entry, + &drm_debugfs_entry_fops); } EXPORT_SYMBOL(drm_debugfs_add_file); diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05b831f9fe71..f928b4490ece 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -597,7 +597,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); - mutex_destroy(&dev->debugfs_mutex); drm_legacy_destroy_members(dev); } @@ -638,14 +637,12 @@ static int drm_dev_init(struct drm_device *dev, INIT_LIST_HEAD(&dev->filelist_internal); INIT_LIST_HEAD(&dev->clientlist); INIT_LIST_HEAD(&dev->vblank_event_list); - INIT_LIST_HEAD(&dev->debugfs_list); spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->filelist_mutex); mutex_init(&dev->clientlist_mutex); mutex_init(&dev->master_mutex); - mutex_init(&dev->debugfs_mutex); ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL); if (ret) diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 5c6e1cadf09b..c5fadbd3fd7d 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -185,7 +185,6 @@ void drm_debugfs_dev_register(struct drm_device *dev); int drm_debugfs_register(struct drm_minor *minor, int minor_id, struct dentry *root); void drm_debugfs_cleanup(struct drm_minor *minor); -void drm_debugfs_late_register(struct drm_device *dev); void drm_debugfs_connector_add(struct drm_connector *connector); void drm_debugfs_connector_remove(struct drm_connector *connector); void drm_debugfs_crtc_add(struct drm_crtc *crtc); @@ -206,10 +205,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor) { } -static inline void drm_debugfs_late_register(struct drm_device *dev) -{ -} - static inline void drm_debugfs_connector_add(struct drm_connector *connector) { } diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 87eb591fe9b5..8525ef851540 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev) if (ret) goto err_connector; - drm_debugfs_late_register(dev); - return 0; err_connector: diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 3cf12b14232d..c490977ee250 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -318,21 +318,6 @@ struct drm_device { */ struct dentry *debugfs_root; - /** - * @debugfs_mutex: - * - * Protects &debugfs_list access. - */ - struct mutex debugfs_mutex; - - /** - * @debugfs_list: - * - * List of debugfs files to be created by the DRM device. The files - * must be added during drm_dev_register(). - */ - struct list_head debugfs_list; - /* Everything below here is for legacy driver, never use! */ /* private: */ #if IS_ENABLED(CONFIG_DRM_LEGACY)
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 but return -ENODEV while opening the file when the minors are not registered yet. v2: rebase on debugfs directory rework, limit access before minors are registered. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_debugfs.c | 27 ++------------------------- 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, 2 insertions(+), 50 deletions(-)