[Intel-gfx] [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well()

[Imre Deak]

On Mon, Aug 20, 2018 at 04:31:36PM -0700, Paulo Zanoni wrote:
> None of the current lookup_power_well() callers are actually checking
> for NULL return values, they all just use the pointer right away.  The
> first idea was to replace these theoretical segfaults with a BUG()
> since this would at least make our code a little more explicit to the
> reader. It was suggested that just converting the BUG() to a WARN()
> and returning any power well would probably be better since it would
> still keep the system running while at the same time exposing the
> driver bug.
> 
> We can only hit this NULL/BUG()/WARN() condition if we try to lookup a
> power well that isn't defined on a given platform. If that ever
> happens, we have to fix our code, making it lookup the correct power
> well. Because of this, I don't think it's worth trying to implement
> error checking in every caller. Improving our CI system will be a
> better use of our time once a bug is found in the wild.
> 
> v2: Avoid the BUG() with a WARN() return a random PW (Michal).
> 
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f75837e45144..f07ed70b839f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1096,7 +1096,15 @@ lookup_power_well(struct drm_i915_private *dev_priv,
>  			return power_well;
>  	}
>  
> -	return NULL;
> +	/*
> +	 * It's not feasible to add error checking code to the callers since
> +	 * this condition really shouldn't happen and it doesn't even make sense
> +	 * to abort things like display initialization sequences. Just return
> +	 * the first power well and hope the WARN gets reported so we can fix
> +	 * our driver.
> +	 */

The first power well is the always-on power well on all platforms, which
has nop get/put handlers so comes handy to use it here, making side effects
less likely. The change makes sense:

Reviewed-by: Imre Deak <imre.deak at intel.com>

> +	WARN(1, "Power well %d not defined for this platform\n", power_well_id);
> +	return &power_domains->power_wells[0];
>  }
>  
>  #define BITS_SET(val, bits) (((val) & (bits)) == (bits))
> -- 
> 2.14.4
>