[2/5] drm/i915: destroy connector sysfs files earlier

Message ID	1380236762-1698-3-git-send-email-przanoni@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org> From: Paulo Zanoni <przanoni@gmail.com> To: intel-gfx@lists.freedesktop.org Subject: [PATCH 2/5] drm/i915: destroy connector sysfs files earlier Date: Thu, 26 Sep 2013 20:05:59 -0300 Message-Id: <1380236762-1698-3-git-send-email-przanoni@gmail.com> In-Reply-To: <1380236762-1698-1-git-send-email-przanoni@gmail.com> References: <1380236762-1698-1-git-send-email-przanoni@gmail.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>, dri-devel@lists.freedesktop.org Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org

Message ID

1380236762-1698-3-git-send-email-przanoni@gmail.com (mailing list archive)

State

New, archived

Headers

From: Paulo Zanoni <przanoni@gmail.com>
To: intel-gfx@lists.freedesktop.org
Subject: [PATCH 2/5] drm/i915: destroy connector sysfs files earlier
Date: Thu, 26 Sep 2013 20:05:59 -0300
Message-Id: <1380236762-1698-3-git-send-email-przanoni@gmail.com>
In-Reply-To: <1380236762-1698-1-git-send-email-przanoni@gmail.com>
References: <1380236762-1698-1-git-send-email-przanoni@gmail.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>, dri-devel@lists.freedesktop.org
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org
Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org

Commit Message

Paulo Zanoni Sept. 26, 2013, 11:05 p.m. UTC

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

For some reason, every single time I try to run module_reload
something tries to read the connector sysfs files. This happens
after we destroy the encoders and before we destroy the connectors, so
when the sysfs read triggers the connector detect() function,
intel_conector->encoder points to memory that was already freed.

The bad backtrace is just:
    [<ffffffff8163ca9a>] dump_stack+0x54/0x74
    [<ffffffffa00c2c8e>] intel_dp_detect+0x1e/0x4b0 [i915]
    [<ffffffffa001913d>] status_show+0x3d/0x80 [drm]
    [<ffffffff813d5340>] dev_attr_show+0x20/0x60
    [<ffffffff81221f50>] ? sysfs_read_file+0x80/0x1b0
    [<ffffffff81221f79>] sysfs_read_file+0xa9/0x1b0
    [<ffffffff811aaf1e>] vfs_read+0x9e/0x170
    [<ffffffff811aba4c>] SyS_read+0x4c/0xa0
    [<ffffffff8164e392>] system_call_fastpath+0x16/0x1b

But if you add tons of memory checking debug options to your Kernel
you'll also see:
 - general protection fault: 0000
 - BUG kmalloc-4096 (Tainted: G      D W   ): Poison overwritten
 - INFO: Allocated in intel_ddi_init+0x65/0x270 [i915]
 - INFO: Freed in intel_dp_encoder_destroy+0x69/0xb0 [i915]
Among a bunch of other error messages.

So this commit just destroys the sysfs files before both the encoder
and connectors are freed.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     | 1 -
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 1 -
 drivers/gpu/drm/i915/intel_dsi.c     | 1 -
 drivers/gpu/drm/i915/intel_dvo.c     | 1 -
 drivers/gpu/drm/i915/intel_hdmi.c    | 1 -
 drivers/gpu/drm/i915/intel_lvds.c    | 1 -
 drivers/gpu/drm/i915/intel_sdvo.c    | 7 +++++--
 drivers/gpu/drm/i915/intel_tv.c      | 1 -
 9 files changed, 10 insertions(+), 9 deletions(-)

Comments

Daniel Vetter Sept. 30, 2013, 9:10 p.m. UTC | #1

On Thu, Sep 26, 2013 at 08:05:59PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> For some reason, every single time I try to run module_reload
> something tries to read the connector sysfs files. This happens
> after we destroy the encoders and before we destroy the connectors, so
> when the sysfs read triggers the connector detect() function,
> intel_conector->encoder points to memory that was already freed.
> 
> The bad backtrace is just:
>     [<ffffffff8163ca9a>] dump_stack+0x54/0x74
>     [<ffffffffa00c2c8e>] intel_dp_detect+0x1e/0x4b0 [i915]
>     [<ffffffffa001913d>] status_show+0x3d/0x80 [drm]
>     [<ffffffff813d5340>] dev_attr_show+0x20/0x60
>     [<ffffffff81221f50>] ? sysfs_read_file+0x80/0x1b0
>     [<ffffffff81221f79>] sysfs_read_file+0xa9/0x1b0
>     [<ffffffff811aaf1e>] vfs_read+0x9e/0x170
>     [<ffffffff811aba4c>] SyS_read+0x4c/0xa0
>     [<ffffffff8164e392>] system_call_fastpath+0x16/0x1b
> 
> But if you add tons of memory checking debug options to your Kernel
> you'll also see:
>  - general protection fault: 0000
>  - BUG kmalloc-4096 (Tainted: G      D W   ): Poison overwritten
>  - INFO: Allocated in intel_ddi_init+0x65/0x270 [i915]
>  - INFO: Freed in intel_dp_encoder_destroy+0x69/0xb0 [i915]
> Among a bunch of other error messages.
> 
> So this commit just destroys the sysfs files before both the encoder
> and connectors are freed.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch. I'm unsure what to do with the
first patch, maybe Ville can take a look. For the next three patches to
wire up bind/unbind hooks I think the right approach would be to just
disallow the vgaconsole once drm/i915 is loaded:
- We don't want to even tighter integrate with the locking madness that is
  fbcon.
- We want to kick out the vgacon anyway, even when e.g. fbcon isn't
  loaded.
David Herrmann might have an idea how to solve this.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_crt.c     | 1 -
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  drivers/gpu/drm/i915/intel_dp.c      | 1 -
>  drivers/gpu/drm/i915/intel_dsi.c     | 1 -
>  drivers/gpu/drm/i915/intel_dvo.c     | 1 -
>  drivers/gpu/drm/i915/intel_hdmi.c    | 1 -
>  drivers/gpu/drm/i915/intel_lvds.c    | 1 -
>  drivers/gpu/drm/i915/intel_sdvo.c    | 7 +++++--
>  drivers/gpu/drm/i915/intel_tv.c      | 1 -
>  9 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 0263629..942b9ac 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -677,7 +677,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  
>  static void intel_crt_destroy(struct drm_connector *connector)
>  {
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 065ffed..793061f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10726,6 +10726,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;
>  
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
> @@ -10768,6 +10769,10 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	/* destroy backlight, if any, before the connectors */
>  	intel_panel_destroy_backlight(dev);
>  
> +	/* destroy the sysfs files before encoders/connectors */
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +		drm_sysfs_connector_remove(connector);
> +
>  	drm_mode_config_cleanup(dev);
>  
>  	intel_cleanup_overlay(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 95a3159..7bdbb36 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3139,7 +3139,6 @@ intel_dp_connector_destroy(struct drm_connector *connector)
>  	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>  		intel_panel_fini(&intel_connector->panel);
>  
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 674fd49..9a2fdd2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -504,7 +504,6 @@ static void intel_dsi_destroy(struct drm_connector *connector)
>  
>  	DRM_DEBUG_KMS("\n");
>  	intel_panel_fini(&intel_connector->panel);
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 91287d1..1b64145 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -367,7 +367,6 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
>  
>  static void intel_dvo_destroy(struct drm_connector *connector)
>  {
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 6004f9c..4f4d346 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1181,7 +1181,6 @@ static void intel_hdmi_post_disable(struct intel_encoder *encoder)
>  
>  static void intel_hdmi_destroy(struct drm_connector *connector)
>  {
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index fb3f8ef..ae0c843 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -474,7 +474,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
>  
>  	intel_panel_fini(&lvds_connector->base.panel);
>  
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 5e59d64..a583e8f 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2009,7 +2009,6 @@ static void intel_sdvo_destroy(struct drm_connector *connector)
>  				     intel_sdvo_connector->tv_format);
>  
>  	intel_sdvo_destroy_enhance_property(connector);
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(intel_sdvo_connector);
>  }
> @@ -2482,6 +2481,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
>  	return true;
>  
>  err:
> +	drm_sysfs_connector_remove(connector);
>  	intel_sdvo_destroy(connector);
>  	return false;
>  }
> @@ -2553,6 +2553,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
>  	return true;
>  
>  err:
> +	drm_sysfs_connector_remove(connector);
>  	intel_sdvo_destroy(connector);
>  	return false;
>  }
> @@ -2624,8 +2625,10 @@ static void intel_sdvo_output_cleanup(struct intel_sdvo *intel_sdvo)
>  
>  	list_for_each_entry_safe(connector, tmp,
>  				 &dev->mode_config.connector_list, head) {
> -		if (intel_attached_encoder(connector) == &intel_sdvo->base)
> +		if (intel_attached_encoder(connector) == &intel_sdvo->base) {
> +			drm_sysfs_connector_remove(connector);
>  			intel_sdvo_destroy(connector);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 75925a1..92895f9 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1433,7 +1433,6 @@ intel_tv_get_modes(struct drm_connector *connector)
>  static void
>  intel_tv_destroy(struct drm_connector *connector)
>  {
> -	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 0263629..942b9ac 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -677,7 +677,6 @@  intel_crt_detect(struct drm_connector *connector, bool force)
 
 static void intel_crt_destroy(struct drm_connector *connector)
 {
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 065ffed..793061f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10726,6 +10726,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;
 
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
@@ -10768,6 +10769,10 @@  void intel_modeset_cleanup(struct drm_device *dev)
 	/* destroy backlight, if any, before the connectors */
 	intel_panel_destroy_backlight(dev);
 
+	/* destroy the sysfs files before encoders/connectors */
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+		drm_sysfs_connector_remove(connector);
+
 	drm_mode_config_cleanup(dev);
 
 	intel_cleanup_overlay(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 95a3159..7bdbb36 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3139,7 +3139,6 @@  intel_dp_connector_destroy(struct drm_connector *connector)
 	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
 		intel_panel_fini(&intel_connector->panel);
 
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 674fd49..9a2fdd2 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -504,7 +504,6 @@  static void intel_dsi_destroy(struct drm_connector *connector)
 
 	DRM_DEBUG_KMS("\n");
 	intel_panel_fini(&intel_connector->panel);
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 91287d1..1b64145 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -367,7 +367,6 @@  static int intel_dvo_get_modes(struct drm_connector *connector)
 
 static void intel_dvo_destroy(struct drm_connector *connector)
 {
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6004f9c..4f4d346 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1181,7 +1181,6 @@  static void intel_hdmi_post_disable(struct intel_encoder *encoder)
 
 static void intel_hdmi_destroy(struct drm_connector *connector)
 {
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index fb3f8ef..ae0c843 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -474,7 +474,6 @@  static void intel_lvds_destroy(struct drm_connector *connector)
 
 	intel_panel_fini(&lvds_connector->base.panel);
 
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5e59d64..a583e8f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2009,7 +2009,6 @@  static void intel_sdvo_destroy(struct drm_connector *connector)
 				     intel_sdvo_connector->tv_format);
 
 	intel_sdvo_destroy_enhance_property(connector);
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(intel_sdvo_connector);
 }
@@ -2482,6 +2481,7 @@  intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 	return true;
 
 err:
+	drm_sysfs_connector_remove(connector);
 	intel_sdvo_destroy(connector);
 	return false;
 }
@@ -2553,6 +2553,7 @@  intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 	return true;
 
 err:
+	drm_sysfs_connector_remove(connector);
 	intel_sdvo_destroy(connector);
 	return false;
 }
@@ -2624,8 +2625,10 @@  static void intel_sdvo_output_cleanup(struct intel_sdvo *intel_sdvo)
 
 	list_for_each_entry_safe(connector, tmp,
 				 &dev->mode_config.connector_list, head) {
-		if (intel_attached_encoder(connector) == &intel_sdvo->base)
+		if (intel_attached_encoder(connector) == &intel_sdvo->base) {
+			drm_sysfs_connector_remove(connector);
 			intel_sdvo_destroy(connector);
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 75925a1..92895f9 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1433,7 +1433,6 @@  intel_tv_get_modes(struct drm_connector *connector)
 static void
 intel_tv_destroy(struct drm_connector *connector)
 {
-	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }

[2/5] drm/i915: destroy connector sysfs files earlier

Commit Message

Comments

Patch