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

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

[Damien Lespiau]

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(-)

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)));
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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)));


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

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)));


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx