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@xxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
