| Message ID | 20180820233139.11936-2-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915: kill intel_display_power_well_is_enabled() | expand |
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@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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@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 >
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. + */ + 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))
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@intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)