| Message ID | 1362670228-19494-3-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote: > Our static analysis tool noticed that 'reg' could be used uninitialized if > we are trying to get a PLL to drive VGA and SPLL is already in use > (plls->spll_refcoung != 0). > > In the (error) case above, let's return false to the caller and emit an > error. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index a1505e3..0eab200 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock) > plls->spll_refcount++; > reg = SPLL_CTL; > intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; > + } else { > + DRM_ERROR("SPLL already in use\n"); > + return false; > } > > WARN(I915_READ(reg) & SPLL_PLL_ENABLE, I know very little about this code, but the warning still seems valuable to me. How about instead of else, move the reg = SPLL_CTL above the 'if'
Hi 2013/3/8 Ben Widawsky <ben@bwidawsk.net>: > On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote: >> Our static analysis tool noticed that 'reg' could be used uninitialized if >> we are trying to get a PLL to drive VGA and SPLL is already in use >> (plls->spll_refcoung != 0). This was also reported by a human a few weeks ago and went to my TODO list. Thanks for clearing my TODO list :) >> >> In the (error) case above, let's return false to the caller and emit an >> error. >> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index a1505e3..0eab200 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock) >> plls->spll_refcount++; >> reg = SPLL_CTL; >> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; >> + } else { >> + DRM_ERROR("SPLL already in use\n"); >> + return false; >> } >> >> WARN(I915_READ(reg) & SPLL_PLL_ENABLE, > > I know very little about this code, but the warning still seems valuable > to me. The WARN will still be hit in case no one is using SPLL but it's enabled. It's a different problem. The code added by Damien is correct, but it should never happen with the current code because only the analog encoder tries to use SPLL and there's only 1 analog encoder. But we can always introduce bugs to trigger these errors :) Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > How about instead of else, move the reg = SPLL_CTL above the 'if' > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 22, 2013 at 06:14:25PM -0300, Paulo Zanoni wrote: > Hi > > 2013/3/8 Ben Widawsky <ben@bwidawsk.net>: > > On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote: > >> Our static analysis tool noticed that 'reg' could be used uninitialized if > >> we are trying to get a PLL to drive VGA and SPLL is already in use > >> (plls->spll_refcoung != 0). > > This was also reported by a human a few weeks ago and went to my TODO > list. Thanks for clearing my TODO list :) > > >> > >> In the (error) case above, let's return false to the caller and emit an > >> error. > >> > >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_ddi.c | 3 +++ > >> 1 files changed, 3 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index a1505e3..0eab200 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock) > >> plls->spll_refcount++; > >> reg = SPLL_CTL; > >> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; > >> + } else { > >> + DRM_ERROR("SPLL already in use\n"); > >> + return false; > >> } > >> > >> WARN(I915_READ(reg) & SPLL_PLL_ENABLE, > > > > I know very little about this code, but the warning still seems valuable > > to me. > > The WARN will still be hit in case no one is using SPLL but it's > enabled. It's a different problem. > > The code added by Damien is correct, but it should never happen with > the current code because only the analog encoder tries to use SPLL and > there's only 1 analog encoder. But we can always introduce bugs to > trigger these errors :) > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index a1505e3..0eab200 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock) plls->spll_refcount++; reg = SPLL_CTL; intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; + } else { + DRM_ERROR("SPLL already in use\n"); + return false; } WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
Our static analysis tool noticed that 'reg' could be used uninitialized if we are trying to get a PLL to drive VGA and SPLL is already in use (plls->spll_refcoung != 0). In the (error) case above, let's return false to the caller and emit an error. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)