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@xxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > --- > 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@xxxxxxxxx> > + 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
