From patchwork Sun Aug 20 12:51:19 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 9914023 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EC325600C5 for ; Tue, 22 Aug 2017 00:37:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E13BF287BD for ; Tue, 22 Aug 2017 00:37:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D6317287EB; Tue, 22 Aug 2017 00:37:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,FREEMAIL_FROM,RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5E695287F0 for ; Tue, 22 Aug 2017 00:37:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 907576E335; Tue, 22 Aug 2017 00:36:39 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wr0-x241.google.com (mail-wr0-x241.google.com [IPv6:2a00:1450:400c:c0c::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id BD2BE6E182; Sun, 20 Aug 2017 12:51:30 +0000 (UTC) Received: by mail-wr0-x241.google.com with SMTP id z91so13483749wrc.4; Sun, 20 Aug 2017 05:51:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=PB3KfO6ciN0edEM8S9RBIwll74MR6oSEoz+n+rNE2Eo=; b=Vsh84AvXQY0DRAh9i1WJDcpEzPnwkE0xbV/bliWEikLl4Y7HQfgVa8lbeFbHW0NGk4 ABzYrhdg51MDGioqf7IWZnv1QlybIBTNYO4fvB+F54rYVTkJx8s6EfqgHBGjFyJ4vhGf oGX/eWowN6Ku9IfGct1twJeGFOtDr5zwMV/GzhoncTVUSOTc7/2YT/hyqEHoQZkiWDfl la01bVyCfKyeQ8JasbkIulq1zxbOaVzo7W+BHd+gz/4eWONE0XXFfMxiO4D++UWJ7RrL eonpoeojB2BK86/Ing9r4FLKRjpnEayGTo6DGulgKsYwg5Qbt0uxaA6RkYEP1oI2vE5j 2zPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=PB3KfO6ciN0edEM8S9RBIwll74MR6oSEoz+n+rNE2Eo=; b=nCAw5leFFKg0BrhtwyAhwiuy8qt9WIBI36LJWtgf7mbb3meKYGR/zcf2bFLuXJ3l1l FdBEt6KieweyJ4aUXte+bp3RjFPxSXuiEQLtLZuLfG/f/8yZ0F6zDjG3RhLb94a+Z/R3 UpgPN756mVEWSSzKhhQTzwRFN0gzqVW1Kxu1AwRhTEtJ2ncMlfCC+VO0lCvgFg9VBBIK zu6klBhvNxZNtrEMjAE7kr0+KuBCdEBREjgkP5WNfyQZr0g6QyyypBsYzsd5o8gECr07 QcEoXZ0oybkgdPCUZFxJbhN8OXwaU001kga9e8oIlxEZ6UjMw6SOhxcEE9l0qxspaJ2C cbvg== X-Gm-Message-State: AHYfb5if5abZGGuM+/na28LCJGHq7S7Y3X1sl8TE+PdlvINSAjOOUiEJ 8NZrKxn3+fYyJg== X-Received: by 10.28.103.138 with SMTP id b132mr5346170wmc.35.1503233489309; Sun, 20 Aug 2017 05:51:29 -0700 (PDT) Received: from shalem.localdomain.com (546A5441.cm-12-3b.dynamic.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id k45sm6163225wre.1.2017.08.20.05.51.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 20 Aug 2017 05:51:28 -0700 (PDT) From: Hans de Goede X-Google-Original-From: Hans de Goede To: Daniel Vetter , Jani Nikula , =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= , Imre Deak Subject: [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Date: Sun, 20 Aug 2017 14:51:19 +0200 Message-Id: <20170820125119.27210-4-hdegoede@redhat.com> X-Mailer: git-send-email 2.13.4 In-Reply-To: <20170820125119.27210-1-hdegoede@redhat.com> References: <20170820125119.27210-1-hdegoede@redhat.com> X-Mailman-Approved-At: Tue, 22 Aug 2017 00:36:37 +0000 Cc: Hans de Goede , intel-gfx , dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP intel_uncore_forcewake_reset() does forcewake puts and gets as such we need to make sure that no-one tries to access the PUNIT->PMIC bus (on systems where this bus is shared) while it runs, otherwise bad things happen. Normally this is taken care of by the i915_pmic_bus_access_notifier() which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other driver tries to access the PMIC bus, so that later forcewake gets are no-ops (for the duration of the bus access). But intel_uncore_forcewake_reset gets called in 3 cases: 1) Before registering the pmic_bus_access_notifier 2) After unregistering the pmic_bus_access_notifier 3) To reset forcewake state on a GPU reset In all 3 cases the i915_pmic_bus_access_notifier() protection is insufficient. This commit fixes this race by calling iosf_mbi_punit_acquire() before calling intel_uncore_forcewake_reset(). In the case where it is called directly after unregistering the pmic_bus_access_notifier, we need to hold the punit-lock over both calls to avoid a race where intel_uncore_fw_release_timer() may execute between the 2 calls. To allow holding the lock over both calls we need an unlocked variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since intel_uncore.c is the only user of this function, we simply modify it in this commit. Doing this in a separate commit would require first adding an unlocked variant, then this commit and then removing the unused normal variant. Signed-off-by: Hans de Goede Reviewed-by: Imre Deak --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Keep punit acquired / locked over the unregister + forcewake_reset call combo to avoid a race hitting between the 2 calls -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier to not take the lock itself, since we are the only users this is done in this same commit Changes in v4: -Fix missing rename in doc-comment -Add and use iosf_mbi_assert_punit_acquired() helper -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside intel_uncore_check_forcewake_domains() -Add Imre's Reviewed-by --- arch/x86/include/asm/iosf_mbi.h | 20 +++++++++++++++++--- arch/x86/platform/intel/iosf_mbi.c | 20 +++++++++++--------- drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++---- drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++ 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index c313cac36f56..0f0de4303180 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void); int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb); /** - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus + * notifier + * + * Note the caller must call iosf_mbi_punit_acquire() before calling this + * to ensure the bus is inactive before unregistering (and call + * iosf_mbi_punit_release() afterwards). * * @nb: notifier_block to unregister */ -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb); /** * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); */ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v); +/** + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired. + */ +void iosf_mbi_assert_punit_acquired(void); + #else /* CONFIG_IOSF_MBI is not enabled */ static inline bool iosf_mbi_available(void) @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } static inline -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { return 0; } @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) return 0; } +static inline void iosf_mbi_assert_punit_acquired(void) {} + #endif /* CONFIG_IOSF_MBI */ #endif /* IOSF_MBI_SYMS_H */ diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c index a952ac199741..a5361fd11e6e 100644 --- a/arch/x86/platform/intel/iosf_mbi.c +++ b/arch/x86/platform/intel/iosf_mbi.c @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { - int ret; + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); - /* Wait for the bus to go inactive before unregistering */ - mutex_lock(&iosf_mbi_punit_mutex); - ret = blocking_notifier_chain_unregister( + return blocking_notifier_chain_unregister( &iosf_mbi_pmic_bus_access_notifier, nb); - mutex_unlock(&iosf_mbi_punit_mutex); - - return ret; } -EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier); +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked); int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) { @@ -239,6 +235,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) } EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain); +void iosf_mbi_assert_punit_acquired(void) +{ + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); +} +EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired); + #ifdef CONFIG_IOSF_MBI_DEBUG static u32 dbg_mdr; static u32 dbg_mcr; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e9ed02518406..80e75c029e59 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) return HRTIMER_NORESTART; } +/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, bool restore) { @@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, int retry_count = 100; enum forcewake_domains fw, active_domains; + iosf_mbi_assert_punit_acquired(); + /* Hold uncore.lock across reset to prevent any register access * with forcewake not set correctly. Wait until all pending * timers are run before holding. @@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, GT_FIFO_CTL_RC6_POLICY_STALL); } + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, restore_forcewake); + iosf_mbi_punit_release(); } void intel_uncore_suspend(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } void intel_uncore_resume_early(struct drm_i915_private *dev_priv) @@ -1246,12 +1253,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv) void intel_uncore_fini(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( - &dev_priv->uncore.pmic_bus_access_nb); - /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev_priv); + + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c index 2d0fef2cfca6..55d0ef4fbcef 100644 --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c @@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri for_each_set_bit(offset, valid, FW_RANGE) { i915_reg_t reg = { offset }; + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); + check_for_unclaimed_mmio(dev_priv); (void)I915_READ(reg);