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

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

[Imre Deak]

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@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);
 	}
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

[PATCH 2/3] drm/i915: sdvo: fix error path in sdvo_connector_init

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 49 +++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 95bdfb3..e9a2680 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2381,16 +2381,22 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
 		return 0x72;
 }
 
-static void
+static int
 intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 			  struct intel_sdvo *encoder)
 {
-	drm_connector_init(encoder->base.base.dev,
-			   &connector->base.base,
+	struct drm_connector *drm_connector;
+	int ret;
+
+	drm_connector = &connector->base.base;
+	ret = drm_connector_init(encoder->base.base.dev,
+			   drm_connector,
 			   &intel_sdvo_connector_funcs,
 			   connector->base.base.connector_type);
+	if (ret < 0)
+		return ret;
 
-	drm_connector_helper_add(&connector->base.base,
+	drm_connector_helper_add(drm_connector,
 				 &intel_sdvo_connector_helper_funcs);
 
 	connector->base.base.interlace_allowed = 1;
@@ -2399,7 +2405,16 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 	connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
 
 	intel_connector_attach_encoder(&connector->base, &encoder->base);
-	drm_sysfs_connector_add(&connector->base.base);
+	ret = drm_sysfs_connector_add(drm_connector);
+	if (ret < 0)
+		goto err1;
+
+	return 0;
+
+err1:
+	drm_connector_cleanup(drm_connector);
+
+	return ret;
 }
 
 static void
@@ -2459,7 +2474,11 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo->is_hdmi = true;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	if (intel_sdvo->is_hdmi)
 		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
 
@@ -2490,7 +2509,10 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 
 	intel_sdvo->is_tv = true;
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
 
 	if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
 		goto err;
@@ -2534,8 +2556,11 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector,
-				  intel_sdvo);
+	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	return true;
 }
 
@@ -2566,7 +2591,11 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
 		goto err;
 
-- 
1.8.4

[PATCH 3/3] drm/i915: sdvo: add i2c sysfs symlink to the connector's directory

[Imre Deak]

This is the same what we do for DP connectors, so make things more
consistent.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e9a2680..b269a7e 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2381,6 +2381,19 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
 		return 0x72;
 }
 
+static void
+intel_sdvo_connector_unregister(struct intel_connector *intel_connector)
+{
+	struct drm_connector *drm_connector;
+	struct intel_sdvo *sdvo_encoder;
+
+	drm_connector = &intel_connector->base;
+	sdvo_encoder = intel_attached_sdvo(&intel_connector->base);
+
+	sysfs_remove_link(&drm_connector->kdev->kobj,
+			  sdvo_encoder->ddc.dev.kobj.name);
+}
+
 static int
 intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 			  struct intel_sdvo *encoder)
@@ -2403,14 +2416,23 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 	connector->base.base.doublescan_allowed = 0;
 	connector->base.base.display_info.subpixel_order = SubPixelHorizontalRGB;
 	connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
+	connector->base.unregister = intel_sdvo_connector_unregister;
 
 	intel_connector_attach_encoder(&connector->base, &encoder->base);
 	ret = drm_sysfs_connector_add(drm_connector);
 	if (ret < 0)
 		goto err1;
 
+	ret = sysfs_create_link(&encoder->ddc.dev.kobj,
+				&drm_connector->kdev->kobj,
+				encoder->ddc.dev.kobj.name);
+	if (ret < 0)
+		goto err2;
+
 	return 0;
 
+err2:
+	drm_sysfs_connector_remove(drm_connector);
 err1:
 	drm_connector_cleanup(drm_connector);
 
-- 
1.8.4

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

[Daniel Vetter]

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

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

> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Imre Deak]

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

--Imre 

Re: [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@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@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

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

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 5886 bytes --]

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


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx