public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org,
	Paulo Zanoni <paulo.r.zanoni@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: destroy connector sysfs files earlier
Date: Mon, 30 Sep 2013 23:10:44 +0200	[thread overview]
Message-ID: <20130930211044.GO26592@phenom.ffwll.local> (raw)
In-Reply-To: <1380236762-1698-3-git-send-email-przanoni@gmail.com>

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

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

  reply	other threads:[~2013-09-30 21:10 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
2013-09-26 23:05 ` [PATCH 1/5] drm/i915: redisable VGA when we disable the power well Paulo Zanoni
2013-10-01 13:47   ` Ville Syrjälä
2013-10-01 18:18     ` Ville Syrjälä
2013-11-29 13:36       ` [PATCH] drm/i915: disable VGA mem when reenabling " Paulo Zanoni
2013-12-11 17:44         ` Paulo Zanoni
2013-12-11 18:39           ` Daniel Vetter
2013-12-11 18:50             ` Paulo Zanoni
2013-12-11 19:02               ` Daniel Vetter
2013-09-26 23:05 ` [PATCH 2/5] drm/i915: destroy connector sysfs files earlier Paulo Zanoni
2013-09-30 21:10   ` Daniel Vetter [this message]
2013-09-26 23:06 ` [PATCH 3/5] tty/vt: add con_bind and con_unbind functions Paulo Zanoni
2013-10-01 13:50   ` Ville Syrjälä
2013-10-08 21:12     ` Paulo Zanoni
2013-10-09  8:10       ` Ville Syrjälä
2013-09-26 23:06 ` [PATCH 4/5] console/fbcon: implement con_bind and con_unbind Paulo Zanoni
2013-09-26 23:06 ` [PATCH 5/5] drm/i915: put/get the power well at the FB bind/unbind functions Paulo Zanoni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130930211044.GO26592@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=paulo.r.zanoni@intel.com \
    --cc=przanoni@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox