[Intel-gfx] [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

Jani Nikula jani.nikula at intel.com
Tue Nov 24 09:38:22 PST 2015


On Tue, 24 Nov 2015, Jani Nikula <jani.nikula at intel.com> 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)++) \
>                  for_each_if ((1 << (domain)) & (mask))

Blah, copy-paste from after the patch, should be s/for_each_if/if/

>
> 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.
>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 14 ++++++++------
>  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, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3d8741eff7d3..f9c779fd84ad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -263,6 +263,8 @@ struct i915_hotplug {
>  	 I915_GEM_DOMAIN_INSTRUCTION | \
>  	 I915_GEM_DOMAIN_VERTEX)
>  
> +#define for_each_if(condition) if (!(condition)); else
> +
>  #define for_each_pipe(__dev_priv, __p) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
>  #define for_each_plane(__dev_priv, __pipe, __p)				\
> @@ -286,7 +288,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 +305,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 +732,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 +1971,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);

-- 
Jani Nikula, Intel Open Source Technology Center


More information about the Intel-gfx mailing list