[PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup

[Daniel Vetter]

On Mon, Jan 30, 2017 at 10:10:15AM +0100, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:03:44AM +0100, Daniel Vetter wrote:
> > On Mon, Jan 30, 2017 at 09:58:48AM +0100, Thierry Reding wrote:
> > > On Fri, Jan 27, 2017 at 03:05:46PM +0100, Daniel Vetter wrote:
> > > > On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote:
> > > > > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote:
> > > > > > > This patchset removes the need for drivers to clean up their debugfs
> > > > > > > files on exit. It is done automatically in drm_debugfs_cleanup().
> > > > > > > This funtion is also called should the driver error out in it's
> > > > > > > drm_driver.debugfs_init callback.
> > > > > > > 
> > > > > > > Two drivers still use drm_debugfs_remove_files():
> > > > > > > - tegra in it's connectors, not sure if I can remove it.
> > > > > > 
> > > > > > I read through them, and they're removed on the component device nodes
> > > > > > stuff. That looks somewhat fishy from a lifetime point of view, and I
> > > > > > think removing all that code would be better, too.
> > > > > 
> > > > > What makes you think that's problematic from a lifetime point of view?
> > > > > The component device is tied to the DRM device, so these callbacks are
> > > > > called at the right time.
> > > > 
> > > > debugfs is a userspace interface, which should disappear when
> > > > drm_dev_unregister gets called. I'm not sure at all whether that lines up
> > > > with the cleanup of all your component nodes, but otoh it's rather
> > > > academic since you can't hotplug a tegra.
> > > > 
> > > > > That said, I think it's safe to remove the other debugfs files from
> > > > > Tegra. It might not be possible to remove the cleanup functions
> > > > > altogether, though, because they have to do a special dance involving
> > > > > kmemdup() drm_debugfs_create_files() and kfree() in order to support
> > > > > debugfs files for multiple instances of subdevices.
> > > > 
> > > > Hm, that entire "do debugfs on the minor" thing makes almost never sense.
> > > > All the things we have left in modern drivers are either per-fd, or
> > > > per-device. Nothing of interest is per-minor. Or do you mean something
> > > > else?
> > > 
> > > I'm not sure I understand what you're saying. We have plenty of code
> > > that adds debugfs files to the connector's debugfs entry. And that's
> > > within the minor's debugfs root.
> > > 
> > > Am I missing something?
> > 
> > Per-connector entries are fine, per-minor imo not.
> 
> Most, if not all, debugfs files in Tegra a per-connector. We have a
> couple that are per-CRTC. And then we have two files that are on the
> minor, which is something I had copied from i915, if I remember
> correctly, though I can't seem to find the original anymore. Maybe
> that was moved somewhere else in the meantime?

All the code I've found in tegra about debugfs is per drm_minor. Some of
it create subdirectories within that, but nothing uses the crtc and
connector ->debugfs_root stuff (which is only in the primary drm_minor
debugfs directory).

> > This is a historical accident, but it also doesn't really hurt anyone.
> > I think it'd make much more sense to move everything into a
> > per-devices entry (with maybe backwards compat links from minor to
> > devices).
> 
> With per-device entries you mean rooted at the device backing the CRTC,
> encoder, connector, ...?

I didn't see any drm support for encoders, no idea what you mean.

> > But really, this is 100% orthogonal to the cleanup here.
> 
> If we want to get rid of the remainder of the cleanup, then it's not
> entirely orthogonal anymore. =)
> 
> Not to say that this cleanup isn't useful in its own right.

Well, looking at this a bit more we might go even further by using
debugfs_remove_recursive(), then we could remove even the tegra stuff. Atm
I think that's not doable because tegra creates its own subdirectories in
drm_minor->debugfs_root. I guess that's a mess for a different day though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch