diff mbox

[v4,4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

Message ID 20170820125119.27210-4-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hans de Goede Aug. 20, 2017, 12:51 p.m. UTC 
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 <hdegoede@redhat.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
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 mbox

Patch

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);