From patchwork Wed Oct 16 23:35:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilija Hadzic X-Patchwork-Id: 3058231 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 672559F243 for ; Wed, 16 Oct 2013 23:33:55 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3782720460 for ; Wed, 16 Oct 2013 23:33:54 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id D9B932045B for ; Wed, 16 Oct 2013 23:33:52 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BCFAEE648A for ; Wed, 16 Oct 2013 16:33:51 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-qa0-f46.google.com (mail-qa0-f46.google.com [209.85.216.46]) by gabe.freedesktop.org (Postfix) with ESMTP id 708C5E62E4 for ; Wed, 16 Oct 2013 16:32:23 -0700 (PDT) Received: by mail-qa0-f46.google.com with SMTP id j15so5000498qaq.5 for ; Wed, 16 Oct 2013 16:32:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=6tqjN8Sdyq9a3xWCWKv7v+hT284ZWMglkurhoGBazp4=; b=XJTjbP46AZW1IDMDcQv48cuXnyTSt9QwW4WuSewrlPCwWyVNKOOOe5uyS9Fs4/MC7s OcBuvbrE7l8IO1/FWw1EmXQUh8TF5+Cy2Qs1acMGkbRoYQNxHaTfApmM1GarU5OMdQJh VnFZ3afspjoSUKp2wNh7zOdSvo5tt+FIap9VQw0PWH5s6zw44bp8hWzhM/VC8OUgLzUJ IDXCz27LR8vu3q2Ea2cp7/DKBmQqNy+rmfgwTSdIhS/ULbDTL7n8SKwQ82uKydjtr+q4 NUw85LbLfGqeZAO5I4exJ0fZ0qqfvVs7g/Vwv6jHpGiiS2CLX73Yle/jqUV9uuY0R8a5 VdSw== X-Received: by 10.224.171.67 with SMTP id g3mr8249552qaz.13.1381966343054; Wed, 16 Oct 2013 16:32:23 -0700 (PDT) Received: from caterpillar.hsd1.nj.comcast.net. (c-98-221-32-44.hsd1.nj.comcast.net. [98.221.32.44]) by mx.google.com with ESMTPSA id a9sm131268610qed.6.1969.12.31.16.00.00 (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 16 Oct 2013 16:32:22 -0700 (PDT) From: Ilija Hadzic To: airlied@gmail.com, dri-devel@lists.freedesktop.org Subject: [PATCH] drm: allocate crtc mutex separately from crtc Date: Wed, 16 Oct 2013 19:35:19 -0400 Message-Id: <1381966519-4185-1-git-send-email-ihadzic@research.bell-labs.com> X-Mailer: git-send-email 1.8.2.1 Cc: Ilija Hadzic , stable@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 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 X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Embedding a mutex inside drm_crtc structure evokes a subtle and rare corruption in drm_crtc_helper_set_config failure-recovery path. Namely, if drm_crtc_helper_set_config takes the path under 'fail:' label *and* some other process has attempted to grab the crtc mutex (and got blocked), restoring the CRTC structure (which is done by bit-copying the entire structure from saved_crtcs array) will overwrite the mutex state and the waiters list pointer within the mutex structure. Consequently the blocked process will never be scheduled. This patch fixes the issue by un-embeding the mutex structure and allocating it separately from the drm_crtc structure when the CRTC is initialized. The bit-copying in drm_crtc_helper_set_config helper will now overwrite the pointer which is never modified during the CRTC's lifetime, thus avoiding corruption. Signed-off-by: Ilija Hadzic Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_crtc.c | 22 +++++++++++++--------- drivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------- drivers/gpu/drm/omapdrm/omap_crtc.c | 10 +++++----- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++---- include/drm/drm_crtc.h | 2 +- 5 files changed, 31 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d7a8370..16b4001 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -52,7 +52,7 @@ void drm_modeset_lock_all(struct drm_device *dev) mutex_lock(&dev->mode_config.mutex); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) - mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); + mutex_lock_nest_lock(crtc->mutex, &dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_lock_all); @@ -65,7 +65,7 @@ void drm_modeset_unlock_all(struct drm_device *dev) struct drm_crtc *crtc; list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); mutex_unlock(&dev->mode_config.mutex); } @@ -84,7 +84,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev) return; list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) - WARN_ON(!mutex_is_locked(&crtc->mutex)); + WARN_ON(!mutex_is_locked(crtc->mutex)); WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); } @@ -634,8 +634,11 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, crtc->invert_dimensions = false; drm_modeset_lock_all(dev); - mutex_init(&crtc->mutex); - mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); + crtc->mutex = kmalloc(sizeof(struct mutex), GFP_KERNEL); + if (!crtc->mutex) + return -ENOMEM; + mutex_init(crtc->mutex); + mutex_lock_nest_lock(crtc->mutex, &dev->mode_config.mutex); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) @@ -670,6 +673,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) drm_mode_object_put(dev, &crtc->base); list_del(&crtc->head); + kfree(crtc->mutex); dev->mode_config.num_crtc--; } EXPORT_SYMBOL(drm_crtc_cleanup); @@ -2284,7 +2288,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, } crtc = obj_to_crtc(obj); - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO; @@ -2308,7 +2312,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, } } out: - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); return ret; @@ -3618,7 +3622,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, return -EINVAL; crtc = obj_to_crtc(obj); - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); if (crtc->fb == NULL) { /* The framebuffer is currently unbound, presumably * due to a hotplug event, that userspace has not @@ -3700,7 +3704,7 @@ out: drm_framebuffer_unreference(fb); if (old_fb) drm_framebuffer_unreference(old_fb); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); return ret; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 617b963..330c0ae 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2229,11 +2229,11 @@ void intel_display_handle_reset(struct drm_device *dev) list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); if (intel_crtc->active) dev_priv->display.update_plane(crtc, crtc->fb, crtc->x, crtc->y); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); } } @@ -7375,7 +7375,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, if (encoder->crtc) { crtc = encoder->crtc; - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); old->dpms_mode = connector->dpms; old->load_detect_temp = false; @@ -7406,7 +7406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, return false; } - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); intel_encoder->new_crtc = to_intel_crtc(crtc); to_intel_connector(connector)->new_encoder = intel_encoder; @@ -7434,7 +7434,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n"); if (IS_ERR(fb)) { DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n"); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); return false; } @@ -7442,7 +7442,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); if (old->release_fb) old->release_fb->funcs->destroy(old->release_fb); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); return false; } @@ -7473,7 +7473,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, drm_framebuffer_unreference(old->release_fb); } - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); return; } @@ -7481,7 +7481,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (old->dpms_mode != DRM_MODE_DPMS_ON) connector->funcs->dpms(connector, old->dpms_mode); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); } static int i9xx_pll_refclk(struct drm_device *dev, diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 0fd2eb1..7b402a3 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -307,13 +307,13 @@ static void page_flip_worker(struct work_struct *work) struct drm_display_mode *mode = &crtc->mode; struct drm_gem_object *bo; - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb, 0, 0, mode->hdisplay, mode->vdisplay, crtc->x << 16, crtc->y << 16, mode->hdisplay << 16, mode->vdisplay << 16, vblank_cb, crtc); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); bo = omap_framebuffer_bo(crtc->fb, 0); drm_gem_object_unreference_unlocked(bo); @@ -446,7 +446,7 @@ static void apply_worker(struct work_struct *work) * the callbacks and list modification all serialized * with respect to modesetting ioctls from userspace. */ - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); dispc_runtime_get(); /* @@ -491,7 +491,7 @@ static void apply_worker(struct work_struct *work) out: dispc_runtime_put(); - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); } int omap_crtc_apply(struct drm_crtc *crtc, @@ -499,7 +499,7 @@ int omap_crtc_apply(struct drm_crtc *crtc, { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - WARN_ON(!mutex_is_locked(&crtc->mutex)); + WARN_ON(!mutex_is_locked(crtc->mutex)); /* no need to queue it again if it is already queued: */ if (apply->queued) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index fc43c06..7874313 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -186,7 +186,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, * can do this since the caller in the drm core doesn't check anything * which is protected by any looks. */ - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); drm_modeset_lock_all(dev_priv->dev); /* A lot of the code assumes this */ @@ -251,7 +251,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, ret = 0; out: drm_modeset_unlock_all(dev_priv->dev); - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); return ret; } @@ -272,7 +272,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) * can do this since the caller in the drm core doesn't check anything * which is protected by any looks. */ - mutex_unlock(&crtc->mutex); + mutex_unlock(crtc->mutex); drm_modeset_lock_all(dev_priv->dev); vmw_cursor_update_position(dev_priv, shown, @@ -280,7 +280,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_y + du->hotspot_y); drm_modeset_unlock_all(dev_priv->dev); - mutex_lock(&crtc->mutex); + mutex_lock(crtc->mutex); return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ba407f6..c8543f2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -416,7 +416,7 @@ struct drm_crtc { * state, ...) and a write lock for everything which can be update * without a full modeset (fb, cursor data, ...) */ - struct mutex mutex; + struct mutex *mutex; struct drm_mode_object base;