| Message ID | 20180823005116.4336-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: protect against division by zero. | expand |
Op 23-08-18 om 02:51 schreef Rodrigo Vivi: > Static analyzer tools thinks it is possible to have a division by zero > here. > > I don't believe we would really reach this path without any crtc enabled, > but may be good to protect against some unexpected path or behavior. > > Fixes: cf1f697acb04 ("drm/i915/skl: distribute DDB based on panel resolution") > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index d99e5fabe93c..fac6e159a640 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3878,6 +3878,12 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > pipe_width = hdisplay; > } > > + if (WARN_ON(total_width == 0)) { > + alloc->start = 0; > + alloc->end = 0; > + return; > + } > + > alloc->start = ddb_size * width_before_pipe / total_width; > alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width; > } It's actually quite impossible.. only way I can imagine if hw is already borked. It must be the case that we will set cstate->active, which implies cstate->enable, which implies a valid mode is set at least on the current crtc. I don't think static analysis tools could deduce it, but definitely a false positive. Also too generic commit message oneliner? ~Maarten
On Thu, Aug 23, 2018 at 07:11:16AM +0200, Maarten Lankhorst wrote: > Op 23-08-18 om 02:51 schreef Rodrigo Vivi: > > Static analyzer tools thinks it is possible to have a division by zero > > here. > > > > I don't believe we would really reach this path without any crtc enabled, > > but may be good to protect against some unexpected path or behavior. > > > > Fixes: cf1f697acb04 ("drm/i915/skl: distribute DDB based on panel resolution") > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index d99e5fabe93c..fac6e159a640 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3878,6 +3878,12 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > pipe_width = hdisplay; > > } > > > > + if (WARN_ON(total_width == 0)) { > > + alloc->start = 0; > > + alloc->end = 0; > > + return; > > + } > > + > > alloc->start = ddb_size * width_before_pipe / total_width; > > alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width; > > } > > It's actually quite impossible.. only way I can imagine if hw is already borked. > > It must be the case that we will set cstate->active, which implies cstate->enable, > which implies a valid mode is set at least on the current crtc. yeap... that's what I thought... but I decided to check just in case I missed something. > > I don't think static analysis tools could deduce it, but definitely a false positive. I'll mart that so... > > Also too generic commit message oneliner? it just reflects the amount of confidence I had on the need of this ;) just dropping it then. thanks for the confirmation > > ~Maarten >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d99e5fabe93c..fac6e159a640 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3878,6 +3878,12 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, pipe_width = hdisplay; } + if (WARN_ON(total_width == 0)) { + alloc->start = 0; + alloc->end = 0; + return; + } + alloc->start = ddb_size * width_before_pipe / total_width; alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width; }
Static analyzer tools thinks it is possible to have a division by zero here. I don't believe we would really reach this path without any crtc enabled, but may be good to protect against some unexpected path or behavior. Fixes: cf1f697acb04 ("drm/i915/skl: distribute DDB based on panel resolution") Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mahesh Kumar <mahesh1.kumar@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ 1 file changed, 6 insertions(+)