| Message ID | 1442496032-28403-1-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2015-09-17 at 14:20 +0100, Damien Lespiau wrote: > Not the first time! not the last time? > > There is a possibility to use gcc 5's -Wbool-compare to try and compare > (reg) in those macros to a constant and gcc will warn that the > comparison between a boolean expression and a constant is always either > true or false. Maybe. > > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> Errrr. Thanks for catching it: Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 4823184..5b600bf 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > * here just read out lanes 0/1 and output a note if lanes 2/3 differ. > */ > hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); > - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) > + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) > DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", > hw_state->pcsdw12, > I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
On Thu, 17 Sep 2015, Imre Deak <imre.deak@intel.com> wrote: > On to, 2015-09-17 at 14:20 +0100, Damien Lespiau wrote: >> Not the first time! not the last time? >> >> There is a possibility to use gcc 5's -Wbool-compare to try and compare >> (reg) in those macros to a constant and gcc will warn that the >> comparison between a boolean expression and a constant is always either >> true or false. Maybe. >> >> Cc: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > Errrr. Thanks for catching it: > Reviewed-by: Imre Deak <imre.deak@intel.com> Pushed to drm-intel-next-queued, thanks for the patch and review. BR, Jani. > >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 4823184..5b600bf 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >> * here just read out lanes 0/1 and output a note if lanes 2/3 differ. >> */ >> hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); >> - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) >> + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) >> DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", >> hw_state->pcsdw12, >> I915_READ(BXT_PORT_PCS_DW12_LN23(port))); > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 17/09/15 14:20, Damien Lespiau wrote: > Not the first time! not the last time? > > There is a possibility to use gcc 5's -Wbool-compare to try and compare > (reg) in those macros to a constant and gcc will warn that the > comparison between a boolean expression and a constant is always either > true or false. Maybe. Since boolean true (1) cannot be a valid argument to this macro, it could contain a compile-time check that the parameter is not 1; if boolean false (0) happens not to be a valid register address (BSpec says MMIO 0 is reserved) the macro could check that the argument is neither of these values, and the compiler might then detect that all possible paths lead to a compile-time error. Something like this? #define I915_READ(reg) \ ({ \ if (__builtin_constant_p(reg)) { \ BUILD_BUG_ON((reg) == false); \ BUILD_BUG_ON((reg) == true); \ } \ dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true); \ }) Interestingly, that reported three errors, all in intel_dsi.c where the port-selection macros use 0s to fill in dummy elements when less than 3 ports are being used. In function ‘intel_dsi_get_hw_state’ In function ‘intel_dsi_port_disable’ In function ‘intel_dsi_port_enable’ Other than that, there weren't any cases where a bool constant was being passed to this specific macro (as of today). .Dave. > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 4823184..5b600bf 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > * here just read out lanes 0/1 and output a note if lanes 2/3 differ. > */ > hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); > - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) > + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) > DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", > hw_state->pcsdw12, > I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
On Wed, Oct 14, 2015 at 01:58:59PM +0100, Dave Gordon wrote: > On 17/09/15 14:20, Damien Lespiau wrote: > >Not the first time! not the last time? > > > >There is a possibility to use gcc 5's -Wbool-compare to try and compare > >(reg) in those macros to a constant and gcc will warn that the > >comparison between a boolean expression and a constant is always either > >true or false. Maybe. > > Since boolean true (1) cannot be a valid argument to this macro, it could > contain a compile-time check that the parameter is not 1; if boolean false > (0) happens not to be a valid register address (BSpec says MMIO 0 is > reserved) the macro could check that the argument is neither of these > values, and the compiler might then detect that all possible paths lead to a > compile-time error. Something like this? > > #define I915_READ(reg) \ > ({ \ > if (__builtin_constant_p(reg)) { \ > BUILD_BUG_ON((reg) == false); \ > BUILD_BUG_ON((reg) == true); \ > } \ > dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true); \ > }) I like this. > Interestingly, that reported three errors, all in intel_dsi.c where the > port-selection macros use 0s to fill in dummy elements when less than 3 > ports are being used. > > In function ‘intel_dsi_get_hw_state’ > In function ‘intel_dsi_port_disable’ > In function ‘intel_dsi_port_enable’ > > Other than that, there weren't any cases where a bool constant was being > passed to this specific macro (as of today). I guess we could fix for_each_dsi_port to only enumerate A and C with something like #define for_each_dsi_port(port, mask) \ for (port = PORT_A, port <= PORT_C; port == PORT_A ? PORT_C : port++) \ if (...) I still think that Ville's approach should catch even more fallout (like mixing up values and registers for write maybe), but this is a great interim hack. Patch wanted ;-) -Daniel
On Wed, 14 Oct 2015, Dave Gordon <david.s.gordon@intel.com> wrote: > On 17/09/15 14:20, Damien Lespiau wrote: >> Not the first time! not the last time? >> >> There is a possibility to use gcc 5's -Wbool-compare to try and compare >> (reg) in those macros to a constant and gcc will warn that the >> comparison between a boolean expression and a constant is always either >> true or false. Maybe. > > Since boolean true (1) cannot be a valid argument to this macro, it > could contain a compile-time check that the parameter is not 1; if > boolean false (0) happens not to be a valid register address (BSpec says > MMIO 0 is reserved) the macro could check that the argument is neither > of these values, and the compiler might then detect that all possible > paths lead to a compile-time error. Something like this? The past errors have also been of the form if (I915_READ(SOME_REG(pipe) & MASK) == val) which isn't caught by your check. See [1]. BR, Jani. [1] http://mid.gmane.org/1442595836-23981-1-git-send-email-ville.syrjala@linux.intel.com > > #define I915_READ(reg) \ > ({ \ > if (__builtin_constant_p(reg)) { \ > BUILD_BUG_ON((reg) == false); \ > BUILD_BUG_ON((reg) == true); \ > } \ > dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true); \ > }) > > Interestingly, that reported three errors, all in intel_dsi.c where the > port-selection macros use 0s to fill in dummy elements when less than 3 > ports are being used. > > In function ‘intel_dsi_get_hw_state’ > In function ‘intel_dsi_port_disable’ > In function ‘intel_dsi_port_enable’ > > Other than that, there weren't any cases where a bool constant was being > passed to this specific macro (as of today). > > .Dave. > >> Cc: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 4823184..5b600bf 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >> * here just read out lanes 0/1 and output a note if lanes 2/3 differ. >> */ >> hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); >> - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) >> + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) >> DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", >> hw_state->pcsdw12, >> I915_READ(BXT_PORT_PCS_DW12_LN23(port))); > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 14 Oct 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 14 Oct 2015, Dave Gordon <david.s.gordon@intel.com> wrote: >> On 17/09/15 14:20, Damien Lespiau wrote: >>> Not the first time! not the last time? >>> >>> There is a possibility to use gcc 5's -Wbool-compare to try and compare >>> (reg) in those macros to a constant and gcc will warn that the >>> comparison between a boolean expression and a constant is always either >>> true or false. Maybe. >> >> Since boolean true (1) cannot be a valid argument to this macro, it >> could contain a compile-time check that the parameter is not 1; if >> boolean false (0) happens not to be a valid register address (BSpec says >> MMIO 0 is reserved) the macro could check that the argument is neither >> of these values, and the compiler might then detect that all possible >> paths lead to a compile-time error. Something like this? > > The past errors have also been of the form > > if (I915_READ(SOME_REG(pipe) & MASK) == val) > > which isn't caught by your check. See [1]. > > > BR, > Jani. > > > [1] http://mid.gmane.org/1442595836-23981-1-git-send-email-ville.syrjala@linux.intel.com > > > > > >> >> #define I915_READ(reg) \ >> ({ \ >> if (__builtin_constant_p(reg)) { \ Also, what Damien fixed wouldn't have been caught because "BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12" is not a builtin constant. Daniel, I don't think this is interim solution material, sorry. BR, Jani. >> BUILD_BUG_ON((reg) == false); \ >> BUILD_BUG_ON((reg) == true); \ >> } \ >> dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true); \ >> }) >> >> Interestingly, that reported three errors, all in intel_dsi.c where the >> port-selection macros use 0s to fill in dummy elements when less than 3 >> ports are being used. >> >> In function ‘intel_dsi_get_hw_state’ >> In function ‘intel_dsi_port_disable’ >> In function ‘intel_dsi_port_enable’ >> >> Other than that, there weren't any cases where a bool constant was being >> passed to this specific macro (as of today). >> >> .Dave. >> >>> Cc: Imre Deak <imre.deak@intel.com> >>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_ddi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>> index 4823184..5b600bf 100644 >>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>> @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >>> * here just read out lanes 0/1 and output a note if lanes 2/3 differ. >>> */ >>> hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); >>> - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) >>> + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) >>> DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", >>> hw_state->pcsdw12, >>> I915_READ(BXT_PORT_PCS_DW12_LN23(port))); >> >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 4823184..5b600bf 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, * here just read out lanes 0/1 and output a note if lanes 2/3 differ. */ hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n", hw_state->pcsdw12, I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
Not the first time! not the last time? There is a possibility to use gcc 5's -Wbool-compare to try and compare (reg) in those macros to a constant and gcc will warn that the comparison between a boolean expression and a constant is always either true or false. Maybe. Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)