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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxx> > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
