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@xxxxxxxxxx> 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: > 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@xxxxxxxxxx> > Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > 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); -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
