| Message ID | 1448392916-2281-2-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ffwll.ch> > Signed-off-by: Jani Nikula <jani.nikula@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 >
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);
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@ffwll.ch> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- 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(-)