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

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx