From: Lyude <cpaul@redhat.com> To: intel-gfx@lists.freedesktop.org Cc: Rob Clark <rclark@redhat.com>, Lyude <cpaul@redhat.com>, stable@vger.kernel.org, Daniel Vetter <daniel.vetter@intel.com>, Jani Nikula <jani.nikula@linux.intel.com>, David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org (open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)) Subject: [PATCH] drm/i915: Fix race condition in intel_dp_destroy_mst_connector() Date: Wed, 16 Mar 2016 15:18:04 -0400 [thread overview] Message-ID: <1458155884-13877-1-git-send-email-cpaul@redhat.com> (raw) After unplugging a DP MST display from the system, we have to go through and destroy all of the DRM connectors associated with it since none of them are valid anymore. Unfortunately, intel_dp_destroy_mst_connector() doesn't do a good enough job of ensuring that throughout the destruction process that no modesettings can be done with the connectors. As it is right now, intel_dp_destroy_mst_connector() works like this: * Take all modeset locks * Clear the configuration of the crtc on the connector, if there is one * Drop all modeset locks, this is required because of circular dependency issues that arise with trying to remove the connector from sysfs with modeset locks held * Unregister the connector * Take all modeset locks, again * Do the rest of the required cleaning for destroying the connector * Finally drop all modeset locks for good This only works sometimes. During the destruction process, it's very possible that a userspace application will attempt to do a modesetting using the connector. When we drop the modeset locks, an ioctl handler such as drm_mode_setcrtc has the oppurtunity to take all of the modeset locks from us. When this happens, one thing leads to another and eventually we end up committing a mode with the non-existent connector: [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* failed to enable link training [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f [drm:intel_dp_start_link_train [i915]] *ERROR* failed to start channel equalization [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f [drm:intel_mst_pre_enable_dp [i915]] *ERROR* failed to allocate vcpi And in some cases, such as with the T460s using an MST dock, this results in breaking modesetting and/or panicking the system. To work around this, we now unregister the connector at the very beginning of intel_dp_destroy_mst_connector(), grab all the modesetting locks, and then hold them until we finish the rest of the function. CC: stable@vger.kernel.org Signed-off-by: Lyude <cpaul@redhat.com> Signed-off-by: Rob Clark <rclark@redhat.com> --- drivers/gpu/drm/i915/intel_dp_mst.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index fa0dabf..b21ac88 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -499,6 +499,8 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct intel_connector *intel_connector = to_intel_connector(connector); struct drm_device *dev = connector->dev; + intel_connector->unregister(intel_connector); + /* need to nuke the connector */ drm_modeset_lock_all(dev); if (connector->state->crtc) { @@ -512,11 +514,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, WARN(ret, "Disabling mst crtc failed with %i\n", ret); } - drm_modeset_unlock_all(dev); - intel_connector->unregister(intel_connector); - - drm_modeset_lock_all(dev); intel_connector_remove_from_fbdev(intel_connector); drm_connector_cleanup(connector); drm_modeset_unlock_all(dev); -- 2.5.0
WARNING: multiple messages have this Message-ID (diff)
From: Lyude <cpaul@redhat.com> To: intel-gfx@lists.freedesktop.org Cc: Rob Clark <rclark@redhat.com>, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., linux-kernel@vger.kernel.org open list" <dri-devel@lists.freedesktop.org>, stable@vger.kernel.org, Daniel Vetter <daniel.vetter@intel.com>, Lyude <cpaul@redhat.com> Subject: [PATCH] drm/i915: Fix race condition in intel_dp_destroy_mst_connector() Date: Wed, 16 Mar 2016 15:18:04 -0400 [thread overview] Message-ID: <1458155884-13877-1-git-send-email-cpaul@redhat.com> (raw) After unplugging a DP MST display from the system, we have to go through and destroy all of the DRM connectors associated with it since none of them are valid anymore. Unfortunately, intel_dp_destroy_mst_connector() doesn't do a good enough job of ensuring that throughout the destruction process that no modesettings can be done with the connectors. As it is right now, intel_dp_destroy_mst_connector() works like this: * Take all modeset locks * Clear the configuration of the crtc on the connector, if there is one * Drop all modeset locks, this is required because of circular dependency issues that arise with trying to remove the connector from sysfs with modeset locks held * Unregister the connector * Take all modeset locks, again * Do the rest of the required cleaning for destroying the connector * Finally drop all modeset locks for good This only works sometimes. During the destruction process, it's very possible that a userspace application will attempt to do a modesetting using the connector. When we drop the modeset locks, an ioctl handler such as drm_mode_setcrtc has the oppurtunity to take all of the modeset locks from us. When this happens, one thing leads to another and eventually we end up committing a mode with the non-existent connector: [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* failed to enable link training [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f [drm:intel_dp_start_link_train [i915]] *ERROR* failed to start channel equalization [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f [drm:intel_mst_pre_enable_dp [i915]] *ERROR* failed to allocate vcpi And in some cases, such as with the T460s using an MST dock, this results in breaking modesetting and/or panicking the system. To work around this, we now unregister the connector at the very beginning of intel_dp_destroy_mst_connector(), grab all the modesetting locks, and then hold them until we finish the rest of the function. CC: stable@vger.kernel.org Signed-off-by: Lyude <cpaul@redhat.com> Signed-off-by: Rob Clark <rclark@redhat.com> --- drivers/gpu/drm/i915/intel_dp_mst.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index fa0dabf..b21ac88 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -499,6 +499,8 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct intel_connector *intel_connector = to_intel_connector(connector); struct drm_device *dev = connector->dev; + intel_connector->unregister(intel_connector); + /* need to nuke the connector */ drm_modeset_lock_all(dev); if (connector->state->crtc) { @@ -512,11 +514,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, WARN(ret, "Disabling mst crtc failed with %i\n", ret); } - drm_modeset_unlock_all(dev); - intel_connector->unregister(intel_connector); - - drm_modeset_lock_all(dev); intel_connector_remove_from_fbdev(intel_connector); drm_connector_cleanup(connector); drm_modeset_unlock_all(dev); -- 2.5.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next reply other threads:[~2016-03-16 19:18 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-03-16 19:18 Lyude [this message] 2016-03-16 19:18 ` [PATCH] drm/i915: Fix race condition in intel_dp_destroy_mst_connector() Lyude 2016-03-16 19:39 ` Ville Syrjälä 2016-03-16 19:39 ` Ville Syrjälä 2016-03-16 19:44 ` Lyude Paul 2016-03-16 19:44 ` Lyude Paul 2016-03-16 19:59 ` Ville Syrjälä 2016-03-16 19:59 ` Ville Syrjälä 2016-03-17 8:12 ` Daniel Vetter 2016-03-17 8:12 ` Daniel Vetter
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=1458155884-13877-1-git-send-email-cpaul@redhat.com \ --to=cpaul@redhat.com \ --cc=airlied@linux.ie \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jani.nikula@linux.intel.com \ --cc=rclark@redhat.com \ --cc=stable@vger.kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.