[Intel-gfx] [PATCH v2] drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()

[Michal Wajdeczko]

On Mon, Apr 10, 2017 at 04:55:31PM +0100, Chris Wilson wrote:
> We acquire the forcewake and use I915_READ_FW instead for the atomic
> wait within intel_uncore_wait_for_register. However, this still leaves
> us vulnerable to concurrent mmio access to the register, which can cause
> system hangs on gen7. The protection is to acquire uncore.lock around
> each register, so lets add it back.
> 
> v2: Wrap __intel_wait_for_register_fw() to re-use its atomic wait_for
> loop and spare adding another for ourselves.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 53c8457869f6..01cea3b7a704 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1661,14 +1661,20 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
>  			    u32 value,
>  			    unsigned int timeout_ms)
>  {
> -
>  	unsigned fw =
>  		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
>  	int ret;
>  
> -	intel_uncore_forcewake_get(dev_priv, fw);
> -	ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2);
> -	intel_uncore_forcewake_put(dev_priv, fw);
> +	spin_lock_irq(&dev_priv->uncore.lock);
> +	intel_uncore_forcewake_get__locked(dev_priv, fw);
> +
> +	ret = __intel_wait_for_register_fw(dev_priv,
> +					   reg, mask, value,
> +					   2, 0, NULL);
> +
> +	intel_uncore_forcewake_put__locked(dev_priv, fw);
> +	spin_unlock_irq(&dev_priv->uncore.lock);

Hmm, shouldn't we use spin_lock_irqsave/spin_unlock_irqrestore here?
Like in GEN6_READ_HEADER/GEN6_READ_FOOTER.

-Michal

> +
>  	if (ret)
>  		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
>  			       timeout_ms);
> -- 
> 2.11.0
>