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); } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5477a72..b82fd53 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -777,10 +777,20 @@ out: return ret; } +static void +intel_dp_connector_unregister(struct intel_connector *intel_connector) +{ + struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); + + sysfs_remove_link(&intel_connector->base.kdev->kobj, + intel_dp->adapter.dev.kobj.name); +} + static int intel_dp_i2c_init(struct intel_dp *intel_dp, struct intel_connector *intel_connector, const char *name) { + struct drm_device *dev; int ret; DRM_DEBUG_KMS("i2c_init %s\n", name); @@ -794,9 +804,20 @@ intel_dp_i2c_init(struct intel_dp *intel_dp, strncpy(intel_dp->adapter.name, name, sizeof(intel_dp->adapter.name) - 1); intel_dp->adapter.name[sizeof(intel_dp->adapter.name) - 1] = '\0'; intel_dp->adapter.algo_data = &intel_dp->algo; - intel_dp->adapter.dev.parent = intel_connector->base.kdev; + dev = intel_connector->base.dev; + intel_dp->adapter.dev.parent = dev->dev; ret = i2c_dp_aux_add_bus(&intel_dp->adapter); + if (ret < 0) + return ret; + + ret = sysfs_create_link(&intel_connector->base.kdev->kobj, + &intel_dp->adapter.dev.kobj, + intel_dp->adapter.dev.kobj.name); + + if (ret < 0) + i2c_del_adapter(&intel_dp->adapter); + return ret; } @@ -3803,6 +3824,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_connector->get_hw_state = intel_ddi_connector_get_hw_state; else intel_connector->get_hw_state = intel_connector_get_hw_state; + intel_connector->unregister = intel_dp_connector_unregister; intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; if (HAS_DDI(dev)) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 44067bc..6b1f2f5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -187,6 +187,13 @@ struct intel_connector { * and active (i.e. dpms ON state). */ bool (*get_hw_state)(struct intel_connector *); + /* + * Removes any interface to the connector like sysfs, making sure + * any pending operations on the connector are finished and there + * won't be any new access to it. + */ + void (*unregister)(struct intel_connector *); + /* Panel info for eDP and LVDS */ struct intel_panel panel; -- 1.8.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
