[Intel-gfx] [PATCH 1/3] drm/i915: dp: fix order of dp aux i2c device cleanup

[Daniel Vetter]

On Mon, Feb 10, 2014 at 12:15:27PM +0200, Imre Deak wrote:
> On Mon, 2014-02-10 at 10:23 +0100, Daniel Vetter wrote:
> > On Sat, Feb 08, 2014 at 02:52:11AM +0200, Imre Deak wrote:
> > > Atm we set the parent of the dp i2c device to be the correspondig
> > > connector device. During driver cleanup we first remove the connector
> > > device through intel_modeset_cleanup()->drm_sysfs_connector_remove() and
> > > only after that the i2c device through the encoder's destroy callback.
> > > This order is not supported by the device core and we'll get a warning,
> > > see the below bugzilla ticket. The proper order is to remove first any
> > > child device and only then the parent device.
> > > 
> > > The first part of the fix changes the i2c device's parent to be the drm
> > > device. Its logical owner is not the connector anyway, but the encoder.
> > > Since the encoder doesn't have a device object, the next best choice is
> > > the drm device. This is the same what we do in the case of the sdvo i2c
> > > device and what the nouveau driver does.
> > > 
> > > The second part creates a symlink in the connector's sysfs directory
> > > pointing to the i2c device. This is so, that we keep the current ABI,
> > > which also makes sense in case someone wants to look up the i2c device
> > > belonging to a specific connector. To remove this symlink a new
> > > unregister callback is added to the connector. In general this should
> > > remove any custom interfaces we added, through which the connector can
> > > be accessed.
> > > 
> > > I also thought of moving the connector sysfs cleanup into its destroy
> > > callback, but that would mean allowing user space access to race with
> > > the destruction of the encoders for example. The following comment
> > > for drm_mode_config_cleanup() also tells me that we have to make sure
> > > there is no pending or new access to the connectors before we call it:
> > > 
> > > """
> > > Note that since this /should/ happen single-threaded at driver/device
> > > teardown time, no locking is required. It's the driver's job to
> > > ensure that this guarantee actually holds true.
> > > """
> > > 
> > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-January/038782.html
> > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-February/039427.html
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  5 +++++
> > >  drivers/gpu/drm/i915/intel_dp.c      | 24 +++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  7 +++++++
> > >  3 files changed, 35 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 4d4a0d9..5b5ec8a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11472,6 +11472,11 @@ void intel_modeset_cleanup(struct drm_device *dev)
> > >  
> > >  	/* destroy the backlight and sysfs files before encoders/connectors */
> > >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > > +		struct intel_connector *intel_connector;
> > > +
> > > +		intel_connector = to_intel_connector(connector);
> > > +		if (intel_connector->unregister)
> > > +			intel_connector->unregister(intel_connector);
> > >  		intel_panel_destroy_backlight(connector);
> > >  		drm_sysfs_connector_remove(connector);
> > >  	}
> > 
> > I don't see what this new callback buys us compared to moving everything
> > into the ->destroy callbacks: The driver is already half torn-down, so
> > userspace better not poke at any sysfs/debugfs files.
> 
> But the above part makes sure exactly that userspace can't access any
> sysfs files. The ->unregister callback is doing the connector specific
> part of that. If we move these things into ->destroy userspace will have
> access to all these sysfs files and if that happens during the following
> encoder/connector ->destroy callbacks things will break. See for
> example 
> 
> commit d9255d57147e1dbcebdf6670409c2fa0ac3609e6
> Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Date:   Thu Sep 26 20:05:59 2013 -0300
> 
> which fixed a bug caused by such a userspace access.
> 
> > Fixing this correctly will be much more invasive (and to be able to do it
> > we need to start at the drm core), adding new stuff here will only make
> > things more complicated once we get around to it.
> 
> I think it still make sense to do an early removal of any userspace
> interface to the driver before we start tearing down things. It's
> similar to how ioctls and other open fds of the driver node prevent the
> teardown from starting, by holding a reference on the module.

Yeah, in the end we need to have a two-phase driver unload sequence:
1. Unregister all userspace interfaces.
2. Clean up the mess.

I'm just afraid we'll run into the wrong direction with the bigger picture
in mind. On 2nd thought the connector->unregister kinda makes sense, so I
guess I'll merge this one. But I can you please follow up with a patch to
also shovel the destroy_backlight call into the respective ->unregister
functions? Then we have all the connector-specific stuff neatly tied up,
the current split looks a bit strange imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch