[PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier
assert_rpm_wakelock_held is triggered from i915_pmic_bus_access_notifier even though it gets unregistered on (runtime) suspend, this is caused by a race happening under the following circumstances:
intel_runtime_pm_put does:
atomic_dec(&dev_priv->pm.wakeref_count);
pm_runtime_mark_last_busy(kdev); pm_runtime_put_autosuspend(kdev);
And pm_runtime_put_autosuspend calls intel_runtime_suspend from a workqueue, so there is ample of time between the atomic_dec() and intel_runtime_suspend() unregistering the notifier. If the notifier gets called in this windowd assert_rpm_wakelock_held falsely triggers (at this point we're not runtime-suspended yet).
This commit adds disable_rpm_wakeref_asserts and enable_rpm_wakeref_asserts calls around the intel_uncore_forcewake_get(FORCEWAKE_ALL) call in i915_pmic_bus_access_notifier fixing the false-positive WARN_ON.
Reported-by: FKr bugs-freedesktop@ubermail.me 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: -Reword comment explaining why disabling the wakeref asserts is ok and necessary -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/intel_uncore.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 1d7b879cc68c..2d3aad319229 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1171,8 +1171,15 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb, * bus, which will be busy after this notification, leading to: * "render: timed out waiting for forcewake ack request." * errors. + * + * The notifier is unregistered during intel_runtime_suspend(), + * so it's ok to access the HW here without holding a RPM + * wake reference -> disable wakeref asserts for the time of + * the access. */ + disable_rpm_wakeref_asserts(dev_priv); intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); + enable_rpm_wakeref_asserts(dev_priv); break; case MBI_PMIC_BUS_ACCESS_END: intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
intel_uncore_suspend() unregisters the uncore code's PMIC bus access notifier and gets called on both normal and runtime suspend.
intel_uncore_resume_early() re-registers the notifier, but only on normal resume. Add a new intel_uncore_runtime_resume() function which only re-registers the notifier and call that on runtime resume.
Reported-by: Imre Deak imre.deak@intel.com 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: -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++ drivers/gpu/drm/i915/intel_uncore.h | 1 + 3 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 43100229613c..4715f320c8fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2585,6 +2585,8 @@ static int intel_runtime_resume(struct device *kdev) ret = vlv_resume_prepare(dev_priv, true); }
+ intel_uncore_runtime_resume(dev_priv); + /* * No point of rolling back things in case of an error, as the best * we can do is to hope that things will still work (and disable RPM). diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2d3aad319229..e9ed02518406 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -434,6 +434,12 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv) i915_check_and_clear_faults(dev_priv); }
+void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv) +{ + iosf_mbi_register_pmic_bus_access_notifier( + &dev_priv->uncore.pmic_bus_access_nb); +} + void intel_uncore_sanitize(struct drm_i915_private *dev_priv) { i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6); diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 5f90278da461..0bdc3fcc0e64 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -121,6 +121,7 @@ bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv void intel_uncore_fini(struct drm_i915_private *dev_priv); void intel_uncore_suspend(struct drm_i915_private *dev_priv); void intel_uncore_resume_early(struct drm_i915_private *dev_priv); +void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv);
u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
Quoting Ville: "the forcewake timer might still be active until the uncore suspend, and having active forcewakes while we've already told the GT wake stuff to stop acting normally doesn't seem quite right to me."
Reported-by: Ville Syrjälä ville.syrjala@linux.intel.com Suggested-by: Imre Deak imre.deak@intel.com 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: -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/i915_drv.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4715f320c8fa..5bf231abe010 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2490,6 +2490,8 @@ static int intel_runtime_suspend(struct device *kdev)
intel_runtime_pm_disable_interrupts(dev_priv);
+ intel_uncore_suspend(dev_priv); + ret = 0; if (IS_GEN9_LP(dev_priv)) { bxt_display_core_uninit(dev_priv); @@ -2502,6 +2504,8 @@ static int intel_runtime_suspend(struct device *kdev)
if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); + intel_uncore_runtime_resume(dev_priv); + intel_runtime_pm_enable_interrupts(dev_priv);
enable_rpm_wakeref_asserts(dev_priv); @@ -2509,8 +2513,6 @@ static int intel_runtime_suspend(struct device *kdev) return ret; }
- intel_uncore_suspend(dev_priv); - enable_rpm_wakeref_asserts(dev_priv); WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
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 --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);
On Sun, Aug 20, 2017 at 02:59:20PM +0200, Hans de Goede wrote:
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:
- Before registering the pmic_bus_access_notifier
- After unregistering the pmic_bus_access_notifier
- 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 --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));
Missed to change this to iosf_mbi_assert_punit_acquired(). The rest looks ok to me. I can change the above while applying, no need to resend for this.
Adding also Chris.
- /* 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);- 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);
-- 2.13.4
Hi,
On 22-08-17 11:00, Imre Deak wrote:
On Sun, Aug 20, 2017 at 02:59:20PM +0200, Hans de Goede wrote:
<snip>
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));
Missed to change this to iosf_mbi_assert_punit_acquired(). The rest looks ok to me. I can change the above while applying, no need to resend for this.
Cool, thank you.
Regards,
Hans
Thomas, Ingo, Peter -
Can I have your ack on merging the below patch via drm/i915? Unfortunately it wasn't originally sent to the proper mailing list. You can see the full series it's part of at [1].
BR, Jani.
[1] https://patchwork.freedesktop.org/series/29058/
On Sun, 20 Aug 2017, Hans de Goede hdegoede@redhat.com wrote:
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:
- Before registering the pmic_bus_access_notifier
- After unregistering the pmic_bus_access_notifier
- 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 --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);- 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);
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Imre Deak -
Jani Nikula