[PATCH] drm/i915: fix dp/sdvo i2c cleanup

[PATCH] drm/i915: fix dp/sdvo i2c cleanup

[Imre Deak]

Atm we try to remove the connector's i2c sysfs entry too late in the
encoder's destroy callback. By that time the kobject used as the parent
for all connector sysfs entries is already removed when we do an early
removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
this by adding an early_destory encoder callback, where we remove the
encoder's i2c adapter.

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 |  8 ++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 10 +++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_sdvo.c    | 10 +++++++++-
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ec96002..3a2c75b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
+	struct drm_encoder *encoder;
 
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
@@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
 
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+
+		if (intel_encoder->early_destroy)
+			intel_encoder->early_destroy(intel_encoder);
+	}
+
 	/* destroy the backlight and sysfs files before encoders/connectors */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		intel_panel_destroy_backlight(connector);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e37c7a0..ecca2ad 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
@@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_dig_port);
 }
 
+
+void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+	i2c_del_adapter(&intel_dp->adapter);
+}
+
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
@@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 		intel_encoder->pre_enable = g4x_pre_enable_dp;
 		intel_encoder->enable = g4x_enable_dp;
 	}
+	intel_encoder->early_destroy = intel_dp_encoder_early_destroy;
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b3c209..216f490 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -149,6 +149,7 @@ struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_config *pipe_config);
+	void (*early_destroy)(struct intel_encoder *);
 	int crtc_mask;
 	enum hpd_pin hpd_pin;
 };
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 95bdfb3..58375cd 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
 		drm_mode_destroy(encoder->dev,
 				 intel_sdvo->sdvo_lvds_fixed_mode);
 
-	i2c_del_adapter(&intel_sdvo->ddc);
 	intel_encoder_destroy(encoder);
 }
 
@@ -2912,6 +2911,14 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo,
 	return i2c_add_adapter(&sdvo->ddc) == 0;
 }
 
+static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
+{
+	struct intel_sdvo *intel_sdvo;
+
+	intel_sdvo = container_of(intel_encoder, struct intel_sdvo, base);
+	i2c_del_adapter(&intel_sdvo->ddc);
+}
+
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2951,6 +2958,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	intel_encoder->enable = intel_enable_sdvo;
 	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
 	intel_encoder->get_config = intel_sdvo_get_config;
+	intel_encoder->early_destroy = intel_sdvo_early_destroy;
 
 	/* In default case sdvo lvds is false */
 	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
-- 
1.8.4

Re: [PATCH] drm/i915: fix dp/sdvo i2c cleanup

[Chris Wilson]

On Fri, Jan 24, 2014 at 11:15:55AM +0200, Imre Deak wrote:
> Atm we try to remove the connector's i2c sysfs entry too late in the
> encoder's destroy callback. By that time the kobject used as the parent
> for all connector sysfs entries is already removed when we do an early
> removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> this by adding an early_destory encoder callback, where we remove the
                         ^destroy
> encoder's i2c adapter.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---

> +void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)

> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> +	i2c_del_adapter(&intel_dp->adapter);

Sigh. ->adapter is too generic, as there are quite a few things
associated with the link that may be called adapter.

> +}
> +
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
>  	.dpms = intel_connector_dpms,
>  	.detect = intel_dp_detect,
> @@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  		intel_encoder->pre_enable = g4x_pre_enable_dp;
>  		intel_encoder->enable = g4x_enable_dp;
>  	}

> +static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_sdvo *intel_sdvo;
> +
> +	intel_sdvo = container_of(intel_encoder, struct intel_sdvo, base);

struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);

> +	i2c_del_adapter(&intel_sdvo->ddc);
> +}

The intel_prefix is redundant in these cases as there is no confusion
with a generic drm_sdvo or drm_dp.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Imre Deak]

Atm we try to remove the connector's i2c sysfs entry too late in the
encoder's destroy callback. By that time the kobject used as the parent
for all connector sysfs entries is already removed when we do an early
removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
this by adding an early_destroy encoder callback, where we remove the
encoder's i2c adapter.

v2:
- add missing static to function, use existing sdvo cast helper,
  s/intel_sdvo/sdvo/ (Chris)

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 |  8 ++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 10 +++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_sdvo.c    |  9 ++++++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e94901..5d7f1fa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
+	struct drm_encoder *encoder;
 
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
@@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
 
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+
+		if (intel_encoder->early_destroy)
+			intel_encoder->early_destroy(intel_encoder);
+	}
+
 	/* destroy the backlight and sysfs files before encoders/connectors */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		intel_panel_destroy_backlight(connector);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e37c7a0..33d6943 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
@@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_dig_port);
 }
 
+
+static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+	i2c_del_adapter(&intel_dp->adapter);
+}
+
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
@@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 		intel_encoder->pre_enable = g4x_pre_enable_dp;
 		intel_encoder->enable = g4x_enable_dp;
 	}
+	intel_encoder->early_destroy = intel_dp_encoder_early_destroy;
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b3c209..216f490 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -149,6 +149,7 @@ struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_config *pipe_config);
+	void (*early_destroy)(struct intel_encoder *);
 	int crtc_mask;
 	enum hpd_pin hpd_pin;
 };
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 95bdfb3..8ecc702 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
 		drm_mode_destroy(encoder->dev,
 				 intel_sdvo->sdvo_lvds_fixed_mode);
 
-	i2c_del_adapter(&intel_sdvo->ddc);
 	intel_encoder_destroy(encoder);
 }
 
@@ -2912,6 +2911,13 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo,
 	return i2c_add_adapter(&sdvo->ddc) == 0;
 }
 
+static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
+{
+	struct intel_sdvo *sdvo = to_sdvo(intel_encoder);
+
+	i2c_del_adapter(&sdvo->ddc);
+}
+
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2951,6 +2957,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	intel_encoder->enable = intel_enable_sdvo;
 	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
 	intel_encoder->get_config = intel_sdvo_get_config;
+	intel_encoder->early_destroy = intel_sdvo_early_destroy;
 
 	/* In default case sdvo lvds is false */
 	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
-- 
1.8.4

Re: [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Daniel Vetter]

On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
> Atm we try to remove the connector's i2c sysfs entry too late in the
> encoder's destroy callback. By that time the kobject used as the parent
> for all connector sysfs entries is already removed when we do an early
> removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> this by adding an early_destroy encoder callback, where we remove the
> encoder's i2c adapter.
> 
> v2:
> - add missing static to function, use existing sdvo cast helper,
>   s/intel_sdvo/sdvo/ (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

This looks fishy ... sysfs should all be reference-counted, so I'm
confused what's going on here. Also I think this smells a bit like it's
going overall in the wrong direction - essentially with sysfs we can't
really force-remove stuff but have to wait until the refcount drops to 0.
Or at least that's how I think it works, I'd need to blow through a pile
of time to figure this all out.

Someone enlighten me please ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_sdvo.c    |  9 ++++++++-
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e94901..5d7f1fa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	struct drm_connector *connector;
> +	struct drm_encoder *encoder;
>  
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
> @@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	/* flush any delayed tasks or pending work */
>  	flush_scheduled_work();
>  
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> +		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> +
> +		if (intel_encoder->early_destroy)
> +			intel_encoder->early_destroy(intel_encoder);
> +	}
> +
>  	/* destroy the backlight and sysfs files before encoders/connectors */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		intel_panel_destroy_backlight(connector);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e37c7a0..33d6943 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  
> -	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> @@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_dig_port);
>  }
>  
> +
> +static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> +	i2c_del_adapter(&intel_dp->adapter);
> +}
> +
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
>  	.dpms = intel_connector_dpms,
>  	.detect = intel_dp_detect,
> @@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  		intel_encoder->pre_enable = g4x_pre_enable_dp;
>  		intel_encoder->enable = g4x_enable_dp;
>  	}
> +	intel_encoder->early_destroy = intel_dp_encoder_early_destroy;
>  
>  	intel_dig_port->port = port;
>  	intel_dig_port->dp.output_reg = output_reg;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b3c209..216f490 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -149,6 +149,7 @@ struct intel_encoder {
>  	 * be set correctly before calling this function. */
>  	void (*get_config)(struct intel_encoder *,
>  			   struct intel_crtc_config *pipe_config);
> +	void (*early_destroy)(struct intel_encoder *);
>  	int crtc_mask;
>  	enum hpd_pin hpd_pin;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 95bdfb3..8ecc702 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
>  		drm_mode_destroy(encoder->dev,
>  				 intel_sdvo->sdvo_lvds_fixed_mode);
>  
> -	i2c_del_adapter(&intel_sdvo->ddc);
>  	intel_encoder_destroy(encoder);
>  }
>  
> @@ -2912,6 +2911,13 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo,
>  	return i2c_add_adapter(&sdvo->ddc) == 0;
>  }
>  
> +static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_sdvo *sdvo = to_sdvo(intel_encoder);
> +
> +	i2c_del_adapter(&sdvo->ddc);
> +}
> +
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2951,6 +2957,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	intel_encoder->enable = intel_enable_sdvo;
>  	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
>  	intel_encoder->get_config = intel_sdvo_get_config;
> +	intel_encoder->early_destroy = intel_sdvo_early_destroy;
>  
>  	/* In default case sdvo lvds is false */
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
> -- 
> 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 v2] drm/i915: fix dp/sdvo i2c cleanup

[Imre Deak]

On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
> > Atm we try to remove the connector's i2c sysfs entry too late in the
> > encoder's destroy callback. By that time the kobject used as the parent
> > for all connector sysfs entries is already removed when we do an early
> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> > this by adding an early_destroy encoder callback, where we remove the
> > encoder's i2c adapter.
> > 
> > v2:
> > - add missing static to function, use existing sdvo cast helper,
> >   s/intel_sdvo/sdvo/ (Chris)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> This looks fishy ... sysfs should all be reference-counted, so I'm
> confused what's going on here. Also I think this smells a bit like it's
> going overall in the wrong direction - essentially with sysfs we can't
> really force-remove stuff but have to wait until the refcount drops to 0.
> Or at least that's how I think it works, I'd need to blow through a pile
> of time to figure this all out.

Hm, I haven't thought about refcounting :) Now, I agree that should
normally allow for removing a parent and child device in both order.

What happens and why we can't remove first the parent then the child:

In

intel_dp_init_connector()->
  intel_dp_i2c_init()

we set the i2c adapter.dev.parent = intel_connector->base.kdev;

device_register will then add the i2c sysfs entry to the connector sysfs
dir. Refcounting here looks ok, both the parent connector kobject and
its sysfs dir entry gets a reference from the child. 

During module cleanup, we call

intel_modeset_cleanup()->
  drm_sysfs_connector_remove()    
    device_unregister(connector->kdev)

which is the i2c adapter's parent kdev. Then:

device_del()->
  kobject_del()->
    sysfs_remove_dir()

will remove all entries recursively in the connector's sysfs dir, along
with all the i2c sysfs entries. Afterwards the intel encoder->destroy()
callback calls i2c_del_adapter()->device_unregister()->device_del() and
that will try to remove its own sysfs attributes, namely the power sysfs
group, but won't find it since it was removed above and give a WARN.

Note that the parent's recursive removal happens regardless of its
kobject's or sysfs entry's refcount. The kobject itself will be put
after device_del() in put_device() and only destroyed after the i2c
adapter releases the refcount it holds on it. I admit it feels strange
that the sysfs entries are removed before the last reference on the
kobject is dropped, not sure if it's by design or an overlook..

--Imre 

> 
> Someone enlighten me please ;-)
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 10 +++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_sdvo.c    |  9 ++++++++-
> >  4 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5e94901..5d7f1fa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  	struct drm_connector *connector;
> > +	struct drm_encoder *encoder;
> >  
> >  	/*
> >  	 * Interrupts and polling as the first thing to avoid creating havoc.
> > @@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  	/* flush any delayed tasks or pending work */
> >  	flush_scheduled_work();
> >  
> > +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> > +		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> > +
> > +		if (intel_encoder->early_destroy)
> > +			intel_encoder->early_destroy(intel_encoder);
> > +	}
> > +
> >  	/* destroy the backlight and sysfs files before encoders/connectors */
> >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> >  		intel_panel_destroy_backlight(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e37c7a0..33d6943 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  
> > -	i2c_del_adapter(&intel_dp->adapter);
> >  	drm_encoder_cleanup(encoder);
> >  	if (is_edp(intel_dp)) {
> >  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> > @@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> >  	kfree(intel_dig_port);
> >  }
> >  
> > +
> > +static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
> > +{
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > +
> > +	i2c_del_adapter(&intel_dp->adapter);
> > +}
> > +
> >  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> >  	.dpms = intel_connector_dpms,
> >  	.detect = intel_dp_detect,
> > @@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >  		intel_encoder->pre_enable = g4x_pre_enable_dp;
> >  		intel_encoder->enable = g4x_enable_dp;
> >  	}
> > +	intel_encoder->early_destroy = intel_dp_encoder_early_destroy;
> >  
> >  	intel_dig_port->port = port;
> >  	intel_dig_port->dp.output_reg = output_reg;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7b3c209..216f490 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -149,6 +149,7 @@ struct intel_encoder {
> >  	 * be set correctly before calling this function. */
> >  	void (*get_config)(struct intel_encoder *,
> >  			   struct intel_crtc_config *pipe_config);
> > +	void (*early_destroy)(struct intel_encoder *);
> >  	int crtc_mask;
> >  	enum hpd_pin hpd_pin;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 95bdfb3..8ecc702 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
> >  		drm_mode_destroy(encoder->dev,
> >  				 intel_sdvo->sdvo_lvds_fixed_mode);
> >  
> > -	i2c_del_adapter(&intel_sdvo->ddc);
> >  	intel_encoder_destroy(encoder);
> >  }
> >  
> > @@ -2912,6 +2911,13 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo,
> >  	return i2c_add_adapter(&sdvo->ddc) == 0;
> >  }
> >  
> > +static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
> > +{
> > +	struct intel_sdvo *sdvo = to_sdvo(intel_encoder);
> > +
> > +	i2c_del_adapter(&sdvo->ddc);
> > +}
> > +
> >  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2951,6 +2957,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
> >  	intel_encoder->enable = intel_enable_sdvo;
> >  	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
> >  	intel_encoder->get_config = intel_sdvo_get_config;
> > +	intel_encoder->early_destroy = intel_sdvo_early_destroy;
> >  
> >  	/* In default case sdvo lvds is false */
> >  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Daniel Vetter]

On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <imre.deak@intel.com> wrote:
> On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
>> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
>> > Atm we try to remove the connector's i2c sysfs entry too late in the
>> > encoder's destroy callback. By that time the kobject used as the parent
>> > for all connector sysfs entries is already removed when we do an early
>> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
>> > this by adding an early_destroy encoder callback, where we remove the
>> > encoder's i2c adapter.
>> >
>> > v2:
>> > - add missing static to function, use existing sdvo cast helper,
>> >   s/intel_sdvo/sdvo/ (Chris)
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
>> >
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>>
>> This looks fishy ... sysfs should all be reference-counted, so I'm
>> confused what's going on here. Also I think this smells a bit like it's
>> going overall in the wrong direction - essentially with sysfs we can't
>> really force-remove stuff but have to wait until the refcount drops to 0.
>> Or at least that's how I think it works, I'd need to blow through a pile
>> of time to figure this all out.
>
> Hm, I haven't thought about refcounting :) Now, I agree that should
> normally allow for removing a parent and child device in both order.
>
> What happens and why we can't remove first the parent then the child:
>
> In
>
> intel_dp_init_connector()->
>   intel_dp_i2c_init()
>
> we set the i2c adapter.dev.parent = intel_connector->base.kdev;
>
> device_register will then add the i2c sysfs entry to the connector sysfs
> dir. Refcounting here looks ok, both the parent connector kobject and
> its sysfs dir entry gets a reference from the child.
>
> During module cleanup, we call
>
> intel_modeset_cleanup()->
>   drm_sysfs_connector_remove()
>     device_unregister(connector->kdev)
>
> which is the i2c adapter's parent kdev. Then:
>
> device_del()->
>   kobject_del()->
>     sysfs_remove_dir()
>
> will remove all entries recursively in the connector's sysfs dir, along
> with all the i2c sysfs entries. Afterwards the intel encoder->destroy()
> callback calls i2c_del_adapter()->device_unregister()->device_del() and
> that will try to remove its own sysfs attributes, namely the power sysfs
> group, but won't find it since it was removed above and give a WARN.
>
> Note that the parent's recursive removal happens regardless of its
> kobject's or sysfs entry's refcount. The kobject itself will be put
> after device_del() in put_device() and only destroyed after the i2c
> adapter releases the refcount it holds on it. I admit it feels strange
> that the sysfs entries are removed before the last reference on the
> kobject is dropped, not sure if it's by design or an overlook..

I have no idea either how exactly this is supposed to work, and I
quick scan through Documentation/ didn't point me into a useful
direction either.

Adding Greg (and more mailing lists) for insight.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Imre Deak]

[-- Attachment #1: Type: text/plain, Size: 3700 bytes --]

On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote:
> On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <imre.deak@intel.com> wrote:
> > On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
> >> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
> >> > Atm we try to remove the connector's i2c sysfs entry too late in the
> >> > encoder's destroy callback. By that time the kobject used as the parent
> >> > for all connector sysfs entries is already removed when we do an early
> >> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> >> > this by adding an early_destroy encoder callback, where we remove the
> >> > encoder's i2c adapter.
> >> >
> >> > v2:
> >> > - add missing static to function, use existing sdvo cast helper,
> >> >   s/intel_sdvo/sdvo/ (Chris)
> >> >
> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> >> >
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>
> >> This looks fishy ... sysfs should all be reference-counted, so I'm
> >> confused what's going on here. Also I think this smells a bit like it's
> >> going overall in the wrong direction - essentially with sysfs we can't
> >> really force-remove stuff but have to wait until the refcount drops to 0.
> >> Or at least that's how I think it works, I'd need to blow through a pile
> >> of time to figure this all out.
> >
> > Hm, I haven't thought about refcounting :) Now, I agree that should
> > normally allow for removing a parent and child device in both order.
> >
> > What happens and why we can't remove first the parent then the child:
> >
> > In
> >
> > intel_dp_init_connector()->
> >   intel_dp_i2c_init()
> >
> > we set the i2c adapter.dev.parent = intel_connector->base.kdev;
> >
> > device_register will then add the i2c sysfs entry to the connector sysfs
> > dir. Refcounting here looks ok, both the parent connector kobject and
> > its sysfs dir entry gets a reference from the child.
> >
> > During module cleanup, we call
> >
> > intel_modeset_cleanup()->
> >   drm_sysfs_connector_remove()
> >     device_unregister(connector->kdev)
> >
> > which is the i2c adapter's parent kdev. Then:
> >
> > device_del()->
> >   kobject_del()->
> >     sysfs_remove_dir()
> >
> > will remove all entries recursively in the connector's sysfs dir, along
> > with all the i2c sysfs entries. Afterwards the intel encoder->destroy()
> > callback calls i2c_del_adapter()->device_unregister()->device_del() and
> > that will try to remove its own sysfs attributes, namely the power sysfs
> > group, but won't find it since it was removed above and give a WARN.
> >
> > Note that the parent's recursive removal happens regardless of its
> > kobject's or sysfs entry's refcount. The kobject itself will be put
> > after device_del() in put_device() and only destroyed after the i2c
> > adapter releases the refcount it holds on it. I admit it feels strange
> > that the sysfs entries are removed before the last reference on the
> > kobject is dropped, not sure if it's by design or an overlook..
> 
> I have no idea either how exactly this is supposed to work, and I
> quick scan through Documentation/ didn't point me into a useful
> direction either.
> 
> Adding Greg (and more mailing lists) for insight.

Attached the corresponding dmesg.

Also one more thought. Imo whether or not it's a valid thing to delete
first a parent device and only then its child device, in this case we
don't have a reason to do so. We created first the connector device
(parent) and then the i2c adapter device (child) and the cleanup should
happen in reverse order. This is so regardless of what order the
corresponding kobjects get destroyed based on their refcounts.

--Imre

[-- Attachment #2: dmesg.txt --]
[-- Type: text/plain, Size: 3058 bytes --]

[ 7516.461543] WARNING: CPU: 1 PID: 4976 at fs/sysfs/group.c:215 sysfs_remove_group+0x5e/0xc0()
[ 7516.461555] sysfs group ffffffff81d11960 not found for kobject 'i2c-6' name 'power'
[ 7516.461566] Modules linked in: tun ccm fuse snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_page_alloc snd_seq_dummy snd_seq_oss arc4 snd_seq_midi iwldvm mac80211 snd_rawmidi i915(-) serio_raw snd_seq_midi_event snd_seq iwlwifi snd_seq_device btusb bnep rfcomm snd_timer bluetooth cfbfillrect thinkpad_acpi cfg80211 cfbimgblt tpm_tis tpm i2c_algo_bit cfbcopyarea snd lpc_ich drm_kms_helper rfkill drm mfd_core intel_smartconnect wmi soundcore vfat fat usbhid [last unloaded: snd_hda_intel]
[ 7516.461739] CPU: 1 PID: 4976 Comm: rmmod Not tainted 3.13.0-rc8+ #38
[ 7516.461763] Hardware name: LENOVO 3460CC6/3460CC6, BIOS G6ET93WW (2.53 ) 02/04/2013
[ 7516.461788]  0000000000000009 ffff8800d36f5b90 ffffffff816fd5bc ffff8800d36f5bd8
[ 7516.461830]  ffff8800d36f5bc8 ffffffff81057bcc ffffffff81d11960 0000000000000000
[ 7516.461860]  ffff8800d494a1b8 ffff8800d494a1a8 ffff8800d494a290 ffff8800d36f5c28
[ 7516.461890] Call Trace:
[ 7516.461908]  [<ffffffff816fd5bc>] dump_stack+0x4e/0x7a
[ 7516.461931]  [<ffffffff81057bcc>] warn_slowpath_common+0x8c/0xc0
[ 7516.461953]  [<ffffffff81057cbc>] warn_slowpath_fmt+0x4c/0x50
[ 7516.461974]  [<ffffffff8170440e>] ? mutex_unlock+0xe/0x10
[ 7516.462003]  [<ffffffff81234b1f>] ? sysfs_get_dirent_ns+0x6f/0x80
[ 7516.462027]  [<ffffffff81235c5e>] sysfs_remove_group+0x5e/0xc0
[ 7516.462049]  [<ffffffff8141382c>] dpm_sysfs_remove+0x3c/0x40
[ 7516.462071]  [<ffffffff81409943>] device_del+0x43/0x1b0
[ 7516.462092]  [<ffffffff81409af8>] device_unregister+0x48/0x60
[ 7516.462114]  [<ffffffff81544b77>] i2c_del_adapter+0x277/0x350
[ 7516.462169]  [<ffffffffa02c0502>] intel_dp_encoder_destroy+0x32/0x90 [i915]
[ 7516.462216]  [<ffffffffa004cd17>] drm_mode_config_cleanup+0x67/0x2a0 [drm]
[ 7516.462270]  [<ffffffffa02b4590>] intel_modeset_cleanup+0xd0/0x110 [i915]
[ 7516.462310]  [<ffffffffa0277675>] i915_driver_unload+0xc5/0x2f0 [i915]
[ 7516.462345]  [<ffffffffa004562c>] drm_dev_unregister+0x2c/0xb0 [drm]
[ 7516.462380]  [<ffffffffa0045a28>] drm_put_dev+0x58/0x70 [drm]
[ 7516.462413]  [<ffffffffa027380d>] i915_pci_remove+0x1d/0x20 [i915]
[ 7516.462432]  [<ffffffff813458a2>] pci_device_remove+0x52/0xd0
[ 7516.462456]  [<ffffffff8140ce4f>] __device_release_driver+0x8f/0xf0
[ 7516.462478]  [<ffffffff8140d90b>] driver_detach+0x9b/0xd0
[ 7516.462501]  [<ffffffff8140cc10>] bus_remove_driver+0xa0/0xc0
[ 7516.462525]  [<ffffffff8140dfd9>] driver_unregister+0x49/0x50
[ 7516.462545]  [<ffffffff813452d2>] pci_unregister_driver+0x22/0xa0
[ 7516.462585]  [<ffffffffa0047d07>] drm_pci_exit+0x47/0xd0 [drm]
[ 7516.462644]  [<ffffffffa02f5afc>] i915_exit+0x20/0x22 [i915]
[ 7516.462668]  [<ffffffff810db024>] SyS_delete_module+0x194/0x250
[ 7516.462719]  [<ffffffff8131b54e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 7516.462749]  [<ffffffff8170df52>] system_call_fastpath+0x16/0x1b

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Greg KH]

On Sun, Jan 26, 2014 at 01:11:15PM +0200, Imre Deak wrote:
> On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote:
> > On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak <imre.deak@intel.com> wrote:
> > > On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote:
> > >> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
> > >> > Atm we try to remove the connector's i2c sysfs entry too late in the
> > >> > encoder's destroy callback. By that time the kobject used as the parent
> > >> > for all connector sysfs entries is already removed when we do an early
> > >> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> > >> > this by adding an early_destroy encoder callback, where we remove the
> > >> > encoder's i2c adapter.
> > >> >
> > >> > v2:
> > >> > - add missing static to function, use existing sdvo cast helper,
> > >> >   s/intel_sdvo/sdvo/ (Chris)
> > >> >
> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> > >> >
> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > >>
> > >> This looks fishy ... sysfs should all be reference-counted, so I'm
> > >> confused what's going on here. Also I think this smells a bit like it's
> > >> going overall in the wrong direction - essentially with sysfs we can't
> > >> really force-remove stuff but have to wait until the refcount drops to 0.
> > >> Or at least that's how I think it works, I'd need to blow through a pile
> > >> of time to figure this all out.
> > >
> > > Hm, I haven't thought about refcounting :) Now, I agree that should
> > > normally allow for removing a parent and child device in both order.
> > >
> > > What happens and why we can't remove first the parent then the child:
> > >
> > > In
> > >
> > > intel_dp_init_connector()->
> > >   intel_dp_i2c_init()
> > >
> > > we set the i2c adapter.dev.parent = intel_connector->base.kdev;
> > >
> > > device_register will then add the i2c sysfs entry to the connector sysfs
> > > dir. Refcounting here looks ok, both the parent connector kobject and
> > > its sysfs dir entry gets a reference from the child.
> > >
> > > During module cleanup, we call
> > >
> > > intel_modeset_cleanup()->
> > >   drm_sysfs_connector_remove()
> > >     device_unregister(connector->kdev)
> > >
> > > which is the i2c adapter's parent kdev. Then:
> > >
> > > device_del()->
> > >   kobject_del()->
> > >     sysfs_remove_dir()
> > >
> > > will remove all entries recursively in the connector's sysfs dir, along
> > > with all the i2c sysfs entries. Afterwards the intel encoder->destroy()
> > > callback calls i2c_del_adapter()->device_unregister()->device_del() and
> > > that will try to remove its own sysfs attributes, namely the power sysfs
> > > group, but won't find it since it was removed above and give a WARN.
> > >
> > > Note that the parent's recursive removal happens regardless of its
> > > kobject's or sysfs entry's refcount. The kobject itself will be put
> > > after device_del() in put_device() and only destroyed after the i2c
> > > adapter releases the refcount it holds on it. I admit it feels strange
> > > that the sysfs entries are removed before the last reference on the
> > > kobject is dropped, not sure if it's by design or an overlook..
> > 
> > I have no idea either how exactly this is supposed to work, and I
> > quick scan through Documentation/ didn't point me into a useful
> > direction either.
> > 
> > Adding Greg (and more mailing lists) for insight.
> 
> Attached the corresponding dmesg.
> 
> Also one more thought. Imo whether or not it's a valid thing to delete
> first a parent device and only then its child device, in this case we
> don't have a reason to do so. We created first the connector device
> (parent) and then the i2c adapter device (child) and the cleanup should
> happen in reverse order. This is so regardless of what order the
> corresponding kobjects get destroyed based on their refcounts.

The kernel used to not complain if you removed a parent before the
child, but now it does.  As you already have all of the needed
information, just switch the removal and then all should be fine.

thanks,

greg k-h

Re: [PATCH] drm/i915: fix dp/sdvo i2c cleanup

[Daniel Vetter]

On Fri, Jan 24, 2014 at 10:15 AM, Imre Deak <imre.deak@intel.com> wrote:
> Atm we try to remove the connector's i2c sysfs entry too late in the
> encoder's destroy callback. By that time the kobject used as the parent
> for all connector sysfs entries is already removed when we do an early
> removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> this by adding an early_destory encoder callback, where we remove the
> encoder's i2c adapter.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Ok, I guess with Greg's clarification this seems to be the correct
fix. But I'm not too happy about the ->early_destroy since that
inversion of control is usually a bad sign for wrong layering. Imo
it'd be better to push all the encoder clean down into each encoders
->destroy callback and then move the dp aux cleanup at the right
place. So essentially we'd need to push the
intel_panel_destroy_backlight and drm_sysfs_connector_remove calls
down. That also allows us to only cleanup the backlight on edp/lvds.

Comments or too insane an idea?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: fix dp/sdvo i2c cleanup

[Imre Deak]

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

On Tue, 2014-02-04 at 17:13 +0100, Daniel Vetter wrote:
> On Fri, Jan 24, 2014 at 10:15 AM, Imre Deak <imre.deak@intel.com> wrote:
> > Atm we try to remove the connector's i2c sysfs entry too late in the
> > encoder's destroy callback. By that time the kobject used as the parent
> > for all connector sysfs entries is already removed when we do an early
> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> > this by adding an early_destory encoder callback, where we remove the
> > encoder's i2c adapter.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Ok, I guess with Greg's clarification this seems to be the correct
> fix. But I'm not too happy about the ->early_destroy since that
> inversion of control is usually a bad sign for wrong layering. Imo
> it'd be better to push all the encoder clean down into each encoders
> ->destroy callback and then move the dp aux cleanup at the right
> place. So essentially we'd need to push the
> intel_panel_destroy_backlight and drm_sysfs_connector_remove calls
> down. That also allows us to only cleanup the backlight on edp/lvds.
> 
> Comments or too insane an idea?

Agreed about ->early_destroy being hacky, it was the fast and usual
solution for reordering something in drm :)

I'll check this later / discuss with Jani about reworking this based on
your suggestion.

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