[PATCH 2/2] drm/i915: fix potential dangling else problems in for_each_ macros

[Daniel Vetter]

On Tue, Nov 24, 2015 at 09:21:56PM +0200, Jani Nikula wrote:
> We have serious dangling else bugs waiting to happen in our for_each_
> style macros with ifs. Consider, for example,
> 
>  #define for_each_power_domain(domain, mask)                         \
>          for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \
>                  if ((1 << (domain)) & (mask))
> 
> If this is used in context:
> 
> 	if (condition)
> 		for_each_power_domain(domain, mask);
> 	else
> 		foo();
> 
> foo() will be called for each domain *not* in mask, if condition holds,
> and not at all if condition doesn't hold.
> 
> Fix this by reversing the conditions in the macros, and adding an else
> branch for the "for each" block, so that other if/else blocks can't
> interfere. Provide a "for_each_if" helper macro to make it easier to get
> this right.
> 
> v2: move for_each_if to drmP.h in a separate patch.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>

Both applied to drm-misc, thanks for respinning with drm headers included.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 12 ++++++------
>  drivers/gpu/drm/i915/intel_display.c    |  2 +-
>  drivers/gpu/drm/i915/intel_dsi.h        |  2 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  4 ++--
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3d8741eff7d3..11ae5a5a0a2e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -286,7 +286,7 @@ struct i915_hotplug {
>  	list_for_each_entry(intel_plane,				\
>  			    &(dev)->mode_config.plane_list,		\
>  			    base.head)					\
> -		if ((intel_plane)->pipe == (intel_crtc)->pipe)
> +		for_each_if ((intel_plane)->pipe == (intel_crtc)->pipe)
>  
>  #define for_each_intel_crtc(dev, intel_crtc) \
>  	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> @@ -303,15 +303,15 @@ struct i915_hotplug {
>  
>  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
>  	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
> -		if ((intel_encoder)->base.crtc == (__crtc))
> +		for_each_if ((intel_encoder)->base.crtc == (__crtc))
>  
>  #define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
>  	list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
> -		if ((intel_connector)->base.encoder == (__encoder))
> +		for_each_if ((intel_connector)->base.encoder == (__encoder))
>  
>  #define for_each_power_domain(domain, mask)				\
>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> -		if ((1 << (domain)) & (mask))
> +		for_each_if ((1 << (domain)) & (mask))
>  
>  struct drm_i915_private;
>  struct i915_mm_struct;
> @@ -730,7 +730,7 @@ struct intel_uncore {
>  	for ((i__) = 0, (domain__) = &(dev_priv__)->uncore.fw_domain[0]; \
>  	     (i__) < FW_DOMAIN_ID_COUNT; \
>  	     (i__)++, (domain__) = &(dev_priv__)->uncore.fw_domain[i__]) \
> -		if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__)))
> +		for_each_if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__)))
>  
>  #define for_each_fw_domain(domain__, dev_priv__, i__) \
>  	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
> @@ -1969,7 +1969,7 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
>  /* Iterate over initialised rings */
>  #define for_each_ring(ring__, dev_priv__, i__) \
>  	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> -		if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
> +		for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
>  
>  enum hdmi_force_audio {
>  	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 75f03e5bee51..4b21d5e137dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12417,7 +12417,7 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
>  	list_for_each_entry((intel_crtc), \
>  			    &(dev)->mode_config.crtc_list, \
>  			    base.head) \
> -		if (mask & (1 <<(intel_crtc)->pipe))
> +		for_each_if (mask & (1 <<(intel_crtc)->pipe))
>  
>  static bool
>  intel_compare_m_n(unsigned int m, unsigned int n,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index e6cb25239941..02551ff228c2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -117,7 +117,7 @@ static inline struct intel_dsi_host *to_intel_dsi_host(struct mipi_dsi_host *h)
>  
>  #define for_each_dsi_port(__port, __ports_mask) \
>  	for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)	\
> -		if ((__ports_mask) & (1 << (__port)))
> +		for_each_if ((__ports_mask) & (1 << (__port)))
>  
>  static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3fa43af94946..469927edf459 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -54,13 +54,13 @@
>  	     i < (power_domains)->power_well_count &&			\
>  		 ((power_well) = &(power_domains)->power_wells[i]);	\
>  	     i++)							\
> -		if ((power_well)->domains & (domain_mask))
> +		for_each_if ((power_well)->domains & (domain_mask))
>  
>  #define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
>  	for (i = (power_domains)->power_well_count - 1;			 \
>  	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
>  	     i--)							 \
> -		if ((power_well)->domains & (domain_mask))
> +		for_each_if ((power_well)->domains & (domain_mask))
>  
>  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>  				    int power_well_id);
> -- 
> 2.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch