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

[Imre Deak]

On Mon, 2014-02-10 at 18:37 +0100, Daniel Vetter wrote:
> 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.

Yep. And btw for driver init we would need a similar split:

1. Init everything
2. Register all userspace interfaces

Which is again not how things work today ..

> 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.

Ok.

--Imre

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140210/4bb90c86/attachment.sig>