Re: [PATCH 3/3] drm/debugfs: remove dev->debugfs_list and debugfs_mutex

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 16, 2023 at 07:06:46PM +0200, Jani Nikula wrote:
> On Thu, 16 Feb 2023, Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx> wrote:
> > On Thu, Feb 16, 2023 at 12:33:08PM +0100, Daniel Vetter wrote:
> >> 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.
> >
> > But should not this the driver responsibility, call drm_debugfs_add_file()
> > whenever you are ready to handle operations on added file ?
> 
> In theory, yes, but in practice it's pretty hard for a non-trivial
> driver to maintain that all the conditions are met.
> 
> In i915 we call debugfs register all over the place only after we've
> called drm_dev_register(), because it's the only sane way. But it means
> we need the init and register separated everywhere, instead of init
> adding files to a list to be registered later.

Yup, it just forces a ton of boilerplate on drivers for no gain.

Like devm_* and drmm_* are also not needed in the strict sense, and they
are all optional. But you're a fool for not using them when you can.

Same thing with these debugfs helpers here, you can outright bypass them,
and then end up doing what amdgpu/i915 currently do: A massive and
somewhat fragile parallel function call hierarchy.

Which is just not very nice thing to be forced into.
-Daniel

> BR,
> Jani.
> 
> 
> 
> >
> > Regards
> > Stanislaw
> >
> >> 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.
> >> 
> >> So yeah we need the list.
> >> 
> >> This also means that we really should not create the debugfs directories
> >> _before_ drm_dev_register is called. That's just fundamentally not how
> >> device interface setup should work:
> >> 
> >> 1. you allocate stucts and stuff
> >> 2. you fully init everything
> >> 3. you register interfaces so they become userspace visible
> >> -Daniel
> >> 
> >> >  }
> >> >  EXPORT_SYMBOL(drm_debugfs_add_file);
> >> >  
> >> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> > index 2cbe028e548c..e7b88b65866c 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 5ff7bf88f162..e215d00ba65c 100644
> >> > --- a/drivers/gpu/drm/drm_internal.h
> >> > +++ b/drivers/gpu/drm/drm_internal.h
> >> > @@ -188,7 +188,6 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> >> >  void drm_debugfs_dev_register(struct drm_device *dev);
> >> >  void drm_debugfs_minor_register(struct drm_minor *minor);
> >> >  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);
> >> > @@ -205,10 +204,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 7cf4afae2e79..900ad7478dd8 100644
> >> > --- a/include/drm/drm_device.h
> >> > +++ b/include/drm/drm_device.h
> >> > @@ -311,21 +311,6 @@ struct drm_device {
> >> >  	 */
> >> >  	struct drm_fb_helper *fb_helper;
> >> >  
> >> > -	/**
> >> > -	 * @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
> >> > 
> >> 
> >> -- 
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux