| Message ID | 1487326666-15789-1-git-send-email-tomasz.lis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 17, 2017 at 11:17:45AM +0100, Tomasz Lis wrote: > The recently introduced patch changed behavior of masks when > the bit number is negative. Instead of no bits set, the new way > makes all bits set. Problematic patch: > drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0) > > This behaviour was not considered when making changes, and boundary > value of count (=0) is now resulting in a mask with all bits on, since > the value is directly decreased and therefore negative. Checking if > all bits are set leads to infinite loop. > > This patch introduces an additional check to avoid empty masks. It > reverts the control flow to the exact same way it worked before > the problematic patch. > > Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> Change commit tile to "drm/i915/fbdev: Proper support of boundary". Other than that: Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
On Fri, 17 Feb 2017, Tomasz Lis <tomasz.lis@intel.com> wrote: > The recently introduced patch changed behavior of masks when > the bit number is negative. Instead of no bits set, the new way > makes all bits set. Problematic patch: > drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0) For future reference, please find the commit id of the committed patch, and reference that with the Fixes: tag. Please Cc the folks from the commit. Whoever commits this must add: Fixes: 3c779a49bd7c ("drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)") Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Joonas, Chris, please check the rest of the regressing commit that it doesn't suffer from the same issue. Thanks, Jani. > This behaviour was not considered when making changes, and boundary > value of count (=0) is now resulting in a mask with all bits on, since > the value is directly decreased and therefore negative. Checking if > all bits are set leads to infinite loop. > > This patch introduces an additional check to avoid empty masks. It > reverts the control flow to the exact same way it worked before > the problematic patch. > > Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> > --- > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index e6f3eb2d..bc65ecf 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -496,7 +496,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, > conn_configured |= BIT(i); > } > > - if ((conn_configured & mask) != mask) { > + if (count > 0 && (conn_configured & mask) != mask) { > pass++; > goto retry; > }
On ma, 2017-02-20 at 10:00 +0200, Jani Nikula wrote: > On Fri, 17 Feb 2017, Tomasz Lis <tomasz.lis@intel.com> wrote: > > > > The recently introduced patch changed behavior of masks when > > the bit number is negative. Instead of no bits set, the new way > > makes all bits set. Problematic patch: > > drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0) > > For future reference, please find the commit id of the committed patch, > and reference that with the Fixes: tag. Please Cc the folks from the > commit. > > Whoever commits this must add: > > Fixes: 3c779a49bd7c ("drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)") > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Joonas, Chris, please check the rest of the regressing commit that it > doesn't suffer from the same issue. > > Thanks, > Jani. This line was actually rest of the commit. So all good, thanks for catching this. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index e6f3eb2d..bc65ecf 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -496,7 +496,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, conn_configured |= BIT(i); } - if ((conn_configured & mask) != mask) { + if (count > 0 && (conn_configured & mask) != mask) { pass++; goto retry; }
The recently introduced patch changed behavior of masks when the bit number is negative. Instead of no bits set, the new way makes all bits set. Problematic patch: drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0) This behaviour was not considered when making changes, and boundary value of count (=0) is now resulting in a mask with all bits on, since the value is directly decreased and therefore negative. Checking if all bits are set leads to infinite loop. This patch introduces an additional check to avoid empty masks. It reverts the control flow to the exact same way it worked before the problematic patch. Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> --- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)