From patchwork Tue Oct 21 10:54:54 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dheeraj Jamwal X-Patchwork-Id: 5120491 Return-Path: X-Original-To: patchwork-ltsi-dev@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A41F0C11AC for ; Tue, 21 Oct 2014 11:55:38 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A4DD420145 for ; Tue, 21 Oct 2014 11:55:37 +0000 (UTC) Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9235D20138 for ; Tue, 21 Oct 2014 11:55:36 +0000 (UTC) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 4B758B92; Tue, 21 Oct 2014 11:11:30 +0000 (UTC) X-Original-To: ltsi-dev@lists.linuxfoundation.org Delivered-To: ltsi-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 92CAACB9 for ; Tue, 21 Oct 2014 11:11:26 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by smtp1.linuxfoundation.org (Postfix) with ESMTP id 03A87201F0 for ; Tue, 21 Oct 2014 11:11:25 +0000 (UTC) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 21 Oct 2014 04:10:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,761,1406617200"; d="scan'208";a="617798670" Received: from ubuntu-desktop.png.intel.com ([10.221.122.25]) by fmsmga002.fm.intel.com with ESMTP; 21 Oct 2014 04:10:23 -0700 From: Dheeraj Jamwal To: ltsi-dev@lists.linuxfoundation.org Date: Tue, 21 Oct 2014 18:54:54 +0800 Message-Id: <1413889294-31328-695-git-send-email-dheerajx.s.jamwal@intel.com> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1413889294-31328-1-git-send-email-dheerajx.s.jamwal@intel.com> References: <1413889294-31328-1-git-send-email-dheerajx.s.jamwal@intel.com> X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org Subject: [LTSI-dev] [PATCH 0694/1094] drm/irq: remove cargo-culted locking from irq_install/uninstall X-BeenThere: ltsi-dev@lists.linuxfoundation.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: "A list to discuss patches, development, and other things related to the LTSI project" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ltsi-dev-bounces@lists.linuxfoundation.org Errors-To: ltsi-dev-bounces@lists.linuxfoundation.org X-Virus-Scanned: ClamAV using ClamSMTP From: Daniel Vetter The dev->struct_mutex locking in drm_irq.c only protects dev->irq_enabled. Which isn't really much at all and only prevents especially nasty ums userspace from concurrently installing the interrupt handling a few times. Or at least trying. There are tons of unlocked readers of dev->irqs_enabled in the vblank wait code (and by extension also in the pageflip code since that uses the same vblank timestamp engine). Real modesetting drivers should ensure that nothing can go haywire with a sane setup teardown sequence. So we only really need this for the drm_control ioctl, everywhere else this will just paper over nastiness. Note that drm/i915 is a bit specially due to the gem+ums combination. So there we also need to properly protect the entervt and leavevt ioctls. But it's definitely saner to do everything in one go than to drop the lock in-between. Finally there's the gpu reset code in drm/i915. That one's just race (concurrent userspace calls to for vblank waits of pageflips could spuriously fail). So wrap it up in with a nice comment since fixing this is more involved. v2: Rebase and fix commit message (Thierry) Reviewed-by: Thierry Reding Reviewed-by: Laurent Pinchart Signed-off-by: Daniel Vetter (cherry picked from commit e090c53b21e97864a25ada5c8437d9776978a050) Signed-off-by: Dheeraj Jamwal --- drivers/gpu/drm/drm_irq.c | 30 +++++++++++++----------------- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_gem.c | 5 +++-- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 9cc907d..3b8cdfb 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -281,20 +281,13 @@ int drm_irq_install(struct drm_device *dev) if (drm_dev_to_irq(dev) == 0) return -EINVAL; - mutex_lock(&dev->struct_mutex); - /* Driver must have been initialized */ - if (!dev->dev_private) { - mutex_unlock(&dev->struct_mutex); + if (!dev->dev_private) return -EINVAL; - } - if (dev->irq_enabled) { - mutex_unlock(&dev->struct_mutex); + if (dev->irq_enabled) return -EBUSY; - } dev->irq_enabled = true; - mutex_unlock(&dev->struct_mutex); DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev)); @@ -315,9 +308,7 @@ int drm_irq_install(struct drm_device *dev) sh_flags, irqname, dev); if (ret < 0) { - mutex_lock(&dev->struct_mutex); dev->irq_enabled = false; - mutex_unlock(&dev->struct_mutex); return ret; } @@ -329,9 +320,7 @@ int drm_irq_install(struct drm_device *dev) ret = dev->driver->irq_postinstall(dev); if (ret < 0) { - mutex_lock(&dev->struct_mutex); dev->irq_enabled = false; - mutex_unlock(&dev->struct_mutex); if (!drm_core_check_feature(dev, DRIVER_MODESET)) vga_client_register(dev->pdev, NULL, NULL, NULL); free_irq(drm_dev_to_irq(dev), dev); @@ -357,10 +346,8 @@ int drm_irq_uninstall(struct drm_device *dev) if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) return -EINVAL; - mutex_lock(&dev->struct_mutex); irq_enabled = dev->irq_enabled; dev->irq_enabled = false; - mutex_unlock(&dev->struct_mutex); /* * Wake up any waiters so they don't hang. @@ -408,6 +395,7 @@ int drm_control(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_control *ctl = data; + int ret = 0; /* if we haven't irq we fallback for compatibility reasons - * this used to be a separate function in drm_dma.h @@ -426,9 +414,17 @@ int drm_control(struct drm_device *dev, void *data, if (dev->if_version < DRM_IF_VERSION(1, 2) && ctl->irq != drm_dev_to_irq(dev)) return -EINVAL; - return drm_irq_install(dev); + mutex_lock(&dev->struct_mutex); + ret = drm_irq_install(dev); + mutex_unlock(&dev->struct_mutex); + + return ret; case DRM_UNINST_HANDLER: - return drm_irq_uninstall(dev); + mutex_lock(&dev->struct_mutex); + ret = drm_irq_uninstall(dev); + mutex_unlock(&dev->struct_mutex); + + return ret; default: return -EINVAL; } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 5d8250f..5213929 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -746,6 +746,11 @@ int i915_reset(struct drm_device *dev) return ret; } + /* + * FIXME: This is horribly race against concurrent pageflip and + * vblank wait ioctls since they can observe dev->irqs_disabled + * being false when they shouldn't be able to. + */ drm_irq_uninstall(dev); drm_irq_install(dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d6b5159..8a0de9c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4662,16 +4662,15 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data, } BUG_ON(!list_empty(&dev_priv->gtt.base.active_list)); - mutex_unlock(&dev->struct_mutex); ret = drm_irq_install(dev); if (ret) goto cleanup_ringbuffer; + mutex_unlock(&dev->struct_mutex); return 0; cleanup_ringbuffer: - mutex_lock(&dev->struct_mutex); i915_gem_cleanup_ringbuffer(dev); dev_priv->ums.mm_suspended = 1; mutex_unlock(&dev->struct_mutex); @@ -4686,7 +4685,9 @@ i915_gem_leavevt_ioctl(struct drm_device *dev, void *data, if (drm_core_check_feature(dev, DRIVER_MODESET)) return 0; + mutex_lock(&dev->struct_mutex); drm_irq_uninstall(dev); + mutex_unlock(&dev->struct_mutex); return i915_gem_suspend(dev); }