| Message ID | 1486559530-15141-1-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 08, 2017 at 03:12:09PM +0200, Joonas Lahtinen wrote: > "BIT(max) - 1" will overflow when max = 32, and GCC will complain. > We already have GENMASK for generating the mask, use it! > > v2: Majestic off by one spotted (Chris) > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Em Qua, 2017-02-08 às 15:12 +0200, Joonas Lahtinen escreveu: > "BIT(max) - 1" will overflow when max = 32, and GCC will complain. > We already have GENMASK for generating the mask, use it! > > v2: Majestic off by one spotted (Chris) > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_device_info.c | 2 +- > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index fcf8181..0891cc0 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct > drm_i915_private *dev_priv) > * The subslice disable field is global, i.e. it applies > * to each of the enabled slices. > */ > - sseu->subslice_mask = BIT(ss_max) - 1; > + sseu->subslice_mask = GENMASK(ss_max - 1, 0); > sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >> > GEN8_F2_SS_DIS_SHIFT); > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index 281c5c4..e6f3eb2d 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct > drm_fb_helper *fb_helper, > return false; > > memcpy(save_enabled, enabled, count); > - mask = BIT(count) - 1; > + mask = GENMASK(count - 1, 0); Due to some debugging accident I ended up with a machine where count is zero. In this case: "BIT(count) - 1" is 0 "GENMASK(count - 1, 0)" is 0xFFFFFFFFFFFFFFFF The consequence is that the machine freezes after i915.ko is loaded. So we have a "short blanket" problem here: one solution is wrong for the maximum value while the other solution is wrong for the minimum value. Either your chest or your feet gets warm, not both. I see that "count" comes from fb_helper->connector_count. The drm_fb_helper.h documentation says that this is "number of connected connectors". So now I'm wondering that maybe zero is actually a possible value (outside of my accident), in which case this patch would be considered a regression. Maybe for those PCH_NONE/PCH_NOP cases? Perhaps we could only revert this specific chunk and keep the power domain chunk using GENMASK? Any other solutions? Also, I have no idea if the subslice_mask case would accept zero as input. I'll let you do the analysis of this piece of the code. Thanks, Paulo > conn_configured = 0; > retry: > for (i = 0; i < count; i++) { > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 66aa1bb..94df466 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1249,7 +1249,7 @@ static void > vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, > vlv_set_power_well(dev_priv, power_well, false); > } > > -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) > +#define POWER_DOMAIN_MASK (GENMASK(POWER_DOMAIN_NUM - 1, 0)) > > static struct i915_power_well *lookup_power_well(struct > drm_i915_private *dev_priv, > int power_well_id)
On Fri, Feb 24, 2017 at 06:03:09PM -0300, Paulo Zanoni wrote: > Em Qua, 2017-02-08 às 15:12 +0200, Joonas Lahtinen escreveu: > > "BIT(max) - 1" will overflow when max = 32, and GCC will complain. > > We already have GENMASK for generating the mask, use it! > > > > v2: Majestic off by one spotted (Chris) > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_device_info.c | 2 +- > > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > b/drivers/gpu/drm/i915/intel_device_info.c > > index fcf8181..0891cc0 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > @@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct > > drm_i915_private *dev_priv) > > * The subslice disable field is global, i.e. it applies > > * to each of the enabled slices. > > */ > > - sseu->subslice_mask = BIT(ss_max) - 1; > > + sseu->subslice_mask = GENMASK(ss_max - 1, 0); > > sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >> > > GEN8_F2_SS_DIS_SHIFT); > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > b/drivers/gpu/drm/i915/intel_fbdev.c > > index 281c5c4..e6f3eb2d 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct > > drm_fb_helper *fb_helper, > > return false; > > > > memcpy(save_enabled, enabled, count); > > - mask = BIT(count) - 1; > > + mask = GENMASK(count - 1, 0); > > Due to some debugging accident I ended up with a machine where count is > zero. > > In this case: > > "BIT(count) - 1" is 0 > "GENMASK(count - 1, 0)" is 0xFFFFFFFFFFFFFFFF > > The consequence is that the machine freezes after i915.ko is loaded. > > So we have a "short blanket" problem here: one solution is wrong for > the maximum value while the other solution is wrong for the minimum > value. Either your chest or your feet gets warm, not both. > > I see that "count" comes from fb_helper->connector_count. The > drm_fb_helper.h documentation says that this is "number of connected > connectors". So now I'm wondering that maybe zero is actually a > possible value (outside of my accident), in which case this patch would > be considered a regression. Maybe for those PCH_NONE/PCH_NOP cases? > > Perhaps we could only revert this specific chunk and keep the power > domain chunk using GENMASK? Any other solutions? > > Also, I have no idea if the subslice_mask case would accept zero as > input. I'll let you do the analysis of this piece of the code. It's already been fixed (hopefully). commit 754a76591b12c88f57ad8b4ca533a5c9566a1922 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Feb 24 11:43:06 2017 +0000 drm/i915/fbdev: Stop repeating tile configuration on stagnation -Chris
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index fcf8181..0891cc0 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv) * The subslice disable field is global, i.e. it applies * to each of the enabled slices. */ - sseu->subslice_mask = BIT(ss_max) - 1; + sseu->subslice_mask = GENMASK(ss_max - 1, 0); sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >> GEN8_F2_SS_DIS_SHIFT); diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 281c5c4..e6f3eb2d 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, return false; memcpy(save_enabled, enabled, count); - mask = BIT(count) - 1; + mask = GENMASK(count - 1, 0); conn_configured = 0; retry: for (i = 0; i < count; i++) { diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 66aa1bb..94df466 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1249,7 +1249,7 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, vlv_set_power_well(dev_priv, power_well, false); } -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) +#define POWER_DOMAIN_MASK (GENMASK(POWER_DOMAIN_NUM - 1, 0)) static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id)
"BIT(max) - 1" will overflow when max = 32, and GCC will complain. We already have GENMASK for generating the mask, use it! v2: Majestic off by one spotted (Chris) Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_device_info.c | 2 +- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)