Re: [PATCH] drm/i915/bxt: Fix wrongly placed ')' in I915_READ()

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx