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

[Imre Deak]

On Thu, Jul 06, 2017 at 09:24:50PM +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:
> 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 the pmic bus access race this causes
> by making intel_uncore_forcewake_reset() call iosf_mbi_punit_acquire()
> (and iosf_mbi_punit_release() when done).
> 
> Note that iosf_mbi_punit_acquire() locks a mutex and thus
> intel_uncore_forcewake_reset() may sleep after this commit. I've checked
> all callers and they all already take other mutexes, so this is not a
> problem.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> Changes in v2:
> -Rebase on current (July 6th 2017) drm-next
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4a547cdfafa9..f9441c9ae226 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -237,6 +237,9 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  	int retry_count = 100;
>  	enum forcewake_domains fw, active_domains;
>  
> +	/* Acquire the PUNIT->PMIC bus before modifying forcewake settings */
> +	iosf_mbi_punit_acquire();
> +
>  	/* Hold uncore.lock across reset to prevent any register access
>  	 * with forcewake not set correctly. Wait until all pending
>  	 * timers are run before holding.
> @@ -294,6 +297,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  		assert_forcewakes_inactive(dev_priv);
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	iosf_mbi_punit_release();
>  }

Looks ok in general, but during intel_uncore_suspend() and
intel_uncore_fini() this still allows the following race:

1. (task 1) i915 MMIO access requiring forcewake, arms
   intel_uncore_fw_release_timer().
2. (task 1) intel_uncore_suspend()->iosf_mbi_unregister_pmic_bus_access_notifier()
3. (task 2) DW I2C access start
4. (hrtimer irq) intel_uncore_fw_release_timer() does forcewake disable
5. (task 2) DW I2C access end

One way to avoid this would be holding iosf_mbi_punit_mutex() around the
whole

iosf_mbi_unregister_pmic_bus_access_notifier()
intel_uncore_forcewake_reset()

sequence.

--Imre

>  
>  static u64 gen9_edram_size(struct drm_i915_private *dev_priv)
> -- 
> 2.13.0
>