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

Jani Nikula
2015-10-14T13:31:41Z

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

--
Jani Nikula, Intel Open Source Technology Center