| Message ID | 1450441647-23924-2-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/12/15 12:27, Joonas Lahtinen wrote: > Take advantage of WARN return value to simplify the flow. > > Cc: Rob Clark <robdclark@gmail.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1d28d90..5a5a3e0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -87,23 +87,18 @@ > */ > #define I915_STATE_WARN(condition, format...) ({ \ > int __ret_warn_on = !!(condition); \ > - if (unlikely(__ret_warn_on)) { \ > - if (i915.verbose_state_checks) \ > - WARN(1, format); \ > - else \ > + if (unlikely(__ret_warn_on)) \ > + if (!WARN(i915.verbose_state_checks, format)) \ > DRM_ERROR(format); \ > - } \ > unlikely(__ret_warn_on); \ > }) > > #define I915_STATE_WARN_ON(condition) ({ \ > int __ret_warn_on = !!(condition); \ > - if (unlikely(__ret_warn_on)) { \ > - if (i915.verbose_state_checks) \ > - WARN(1, "WARN_ON(" #condition ")\n"); \ > - else \ > + if (unlikely(__ret_warn_on)) \ > + if (!WARN(i915.verbose_state_checks, \ > + "WARN_ON(" #condition ")\n")) \ > DRM_ERROR("WARN_ON(" #condition ")\n"); \ These last two lines still have the text of the condition as part of a format string :( For compile-testing, you might want to change: static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv ... if (WARN_ON(steps % 5 != 0)) return; to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a compile-time warning if the '%' ends up in the format string. .Dave.
On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote: > On 18/12/15 12:27, Joonas Lahtinen wrote: > > Take advantage of WARN return value to simplify the flow. > > > > Cc: Rob Clark <robdclark@gmail.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 15 +++++---------- > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 1d28d90..5a5a3e0 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -87,23 +87,18 @@ > > */ > > #define I915_STATE_WARN(condition, format...) ({ > > \ > > int __ret_warn_on = !!(condition); > > \ > > - if (unlikely(__ret_warn_on)) { > > \ > > - if (i915.verbose_state_checks) > > \ > > - WARN(1, format); > > \ > > - else > > \ > > + if (unlikely(__ret_warn_on)) > > \ > > + if (!WARN(i915.verbose_state_checks, format)) > > \ > > DRM_ERROR(format); > > \ > > - } > > \ > > unlikely(__ret_warn_on); > > \ > > }) > > > > #define I915_STATE_WARN_ON(condition) ({ > > \ > > int __ret_warn_on = !!(condition); > > \ > > - if (unlikely(__ret_warn_on)) { > > \ > > - if (i915.verbose_state_checks) > > \ > > - WARN(1, "WARN_ON(" #condition ")\n"); > > \ > > - else > > \ > > + if (unlikely(__ret_warn_on)) > > \ > > + if (!WARN(i915.verbose_state_checks, > > \ > > + "WARN_ON(" #condition ")\n")) > > \ > > DRM_ERROR("WARN_ON(" #condition ")\n"); > > \ > > These last two lines still have the text of the condition as part of > a > format string :( > > For compile-testing, you might want to change: > > static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv > ... > if (WARN_ON(steps % 5 != 0)) > return; > > to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a > compile-time warning if the '%' ends up in the format string. > This is just a patch to convert the old macros to different order before changing them. The way of constructing the strings is intact. Regards, Joonas > .Dave. >
On 21/12/15 08:11, Joonas Lahtinen wrote: > On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote: >> On 18/12/15 12:27, Joonas Lahtinen wrote: >>> Take advantage of WARN return value to simplify the flow. >>> >>> Cc: Rob Clark <robdclark@gmail.com> >>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Reported-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 15 +++++---------- >>> 1 file changed, 5 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h >>> index 1d28d90..5a5a3e0 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -87,23 +87,18 @@ >>> */ >>> #define I915_STATE_WARN(condition, format...) ({ >>> \ >>> int __ret_warn_on = !!(condition); >>> \ >>> - if (unlikely(__ret_warn_on)) { >>> \ >>> - if (i915.verbose_state_checks) >>> \ >>> - WARN(1, format); >>> \ >>> - else >>> \ >>> + if (unlikely(__ret_warn_on)) >>> \ >>> + if (!WARN(i915.verbose_state_checks, format)) >>> \ >>> DRM_ERROR(format); >>> \ >>> - } >>> \ >>> unlikely(__ret_warn_on); >>> \ >>> }) >>> >>> #define I915_STATE_WARN_ON(condition) ({ >>> \ >>> int __ret_warn_on = !!(condition); >>> \ >>> - if (unlikely(__ret_warn_on)) { >>> \ >>> - if (i915.verbose_state_checks) >>> \ >>> - WARN(1, "WARN_ON(" #condition ")\n"); >>> \ >>> - else >>> \ >>> + if (unlikely(__ret_warn_on)) >>> \ >>> + if (!WARN(i915.verbose_state_checks, >>> \ >>> + "WARN_ON(" #condition ")\n")) >>> \ >>> DRM_ERROR("WARN_ON(" #condition ")\n"); >>> \ >> >> These last two lines still have the text of the condition as part of >> a >> format string :( >> >> For compile-testing, you might want to change: >> >> static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv >> ... >> if (WARN_ON(steps % 5 != 0)) >> return; >> >> to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a >> compile-time warning if the '%' ends up in the format string. >> > > This is just a patch to convert the old macros to different order > before changing them. The way of constructing the strings is intact. > > Regards, Joonas Yes, I agree, you didn't break them -- they were already wrong! .Dave.
On Mon, Dec 21, 2015 at 11:53:52AM +0000, Dave Gordon wrote: > On 21/12/15 08:11, Joonas Lahtinen wrote: > >On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote: > >>On 18/12/15 12:27, Joonas Lahtinen wrote: > >>>Take advantage of WARN return value to simplify the flow. > >>> > >>>Cc: Rob Clark <robdclark@gmail.com> > >>>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >>>--- > >>> drivers/gpu/drm/i915/i915_drv.h | 15 +++++---------- > >>> 1 file changed, 5 insertions(+), 10 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/i915_drv.h > >>>b/drivers/gpu/drm/i915/i915_drv.h > >>>index 1d28d90..5a5a3e0 100644 > >>>--- a/drivers/gpu/drm/i915/i915_drv.h > >>>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>>@@ -87,23 +87,18 @@ > >>> */ > >>> #define I915_STATE_WARN(condition, format...) ({ > >>>\ > >>> int __ret_warn_on = !!(condition); > >>> \ > >>>- if (unlikely(__ret_warn_on)) { > >>> \ > >>>- if (i915.verbose_state_checks) > >>> \ > >>>- WARN(1, format); > >>>\ > >>>- else > >>> \ > >>>+ if (unlikely(__ret_warn_on)) > >>> \ > >>>+ if (!WARN(i915.verbose_state_checks, format)) > >>> \ > >>> DRM_ERROR(format); > >>> \ > >>>- } > >>>\ > >>> unlikely(__ret_warn_on); > >>>\ > >>> }) > >>> > >>> #define I915_STATE_WARN_ON(condition) ({ > >>>\ > >>> int __ret_warn_on = !!(condition); > >>> \ > >>>- if (unlikely(__ret_warn_on)) { > >>> \ > >>>- if (i915.verbose_state_checks) > >>> \ > >>>- WARN(1, "WARN_ON(" #condition ")\n"); > >>> \ > >>>- else > >>> \ > >>>+ if (unlikely(__ret_warn_on)) > >>> \ > >>>+ if (!WARN(i915.verbose_state_checks, > >>> \ > >>>+ "WARN_ON(" #condition ")\n")) > >>> \ > >>> DRM_ERROR("WARN_ON(" #condition ")\n"); > >>> \ > >> > >>These last two lines still have the text of the condition as part of > >>a > >>format string :( > >> > >>For compile-testing, you might want to change: > >> > >> static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv > >> ... > >> if (WARN_ON(steps % 5 != 0)) > >> return; > >> > >>to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a > >>compile-time warning if the '%' ends up in the format string. > >> > > > >This is just a patch to convert the old macros to different order > >before changing them. The way of constructing the strings is intact. > > > >Regards, Joonas > > Yes, I agree, you didn't break them -- they were already wrong! Yeah I think it makes sense to fix that. I'll wait for v4. -Daniel
On ma, 2015-12-21 at 14:41 +0100, Daniel Vetter wrote: > On Mon, Dec 21, 2015 at 11:53:52AM +0000, Dave Gordon wrote: > > On 21/12/15 08:11, Joonas Lahtinen wrote: > > > On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote: > > > > On 18/12/15 12:27, Joonas Lahtinen wrote: > > > > > Take advantage of WARN return value to simplify the flow. > > > > > > > > > > Cc: Rob Clark <robdclark@gmail.com> > > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Signed-off-by: Joonas Lahtinen < > > > > > joonas.lahtinen@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.h | 15 +++++---------- > > > > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > > index 1d28d90..5a5a3e0 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -87,23 +87,18 @@ > > > > > */ > > > > > #define I915_STATE_WARN(condition, format...) ({ > > > > > > > > > > \ > > > > > int __ret_warn_on = !!(condition); > > > > > \ > > > > > - if (unlikely(__ret_warn_on)) { > > > > > > > > > > \ > > > > > - if (i915.verbose_state_checks) > > > > > > > > > > \ > > > > > - WARN(1, format); > > > > > > > > > > \ > > > > > - else > > > > > > > > > > \ > > > > > + if (unlikely(__ret_warn_on)) > > > > > \ > > > > > + if (!WARN(i915.verbose_state_checks, > > > > > format)) > > > > > \ > > > > > DRM_ERROR(format); > > > > > \ > > > > > - } > > > > > > > > > > \ > > > > > unlikely(__ret_warn_on); > > > > > > > > > > \ > > > > > }) > > > > > > > > > > #define I915_STATE_WARN_ON(condition) ({ > > > > > > > > > > \ > > > > > int __ret_warn_on = !!(condition); > > > > > \ > > > > > - if (unlikely(__ret_warn_on)) { > > > > > > > > > > \ > > > > > - if (i915.verbose_state_checks) > > > > > > > > > > \ > > > > > - WARN(1, "WARN_ON(" #condition > > > > > ")\n"); > > > > > \ > > > > > - else > > > > > > > > > > \ > > > > > + if (unlikely(__ret_warn_on)) > > > > > \ > > > > > + if (!WARN(i915.verbose_state_checks, > > > > > \ > > > > > + "WARN_ON(" #condition ")\n")) > > > > > > > > > > \ > > > > > DRM_ERROR("WARN_ON(" #condition > > > > > ")\n"); > > > > > \ > > > > > > > > These last two lines still have the text of the condition as > > > > part of > > > > a > > > > format string :( > > > > > > > > For compile-testing, you might want to change: > > > > > > > > static void lpt_bend_clkout_dp(struct drm_i915_private > > > > *dev_priv > > > > ... > > > > if (WARN_ON(steps % 5 != 0)) > > > > return; > > > > > > > > to use I915_STATE_WARN_ON() instead of WARN_ON, then you should > > > > get a > > > > compile-time warning if the '%' ends up in the format string. > > > > > > > > > > This is just a patch to convert the old macros to different order > > > before changing them. The way of constructing the strings is > > > intact. > > > > > > Regards, Joonas > > > > Yes, I agree, you didn't break them -- they were already wrong! > > Yeah I think it makes sense to fix that. I'll wait for v4. Ok, to be perfectly clear; This is patch 1/2, which does as the commit message specifies, it simplifies the function flow, patch 2/2 of the series changes the string construction and addresses the mentioned problem. Sorry for being little bit unclear about that on the first comment. Regards, Joonas > -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1d28d90..5a5a3e0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -87,23 +87,18 @@ */ #define I915_STATE_WARN(condition, format...) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) { \ - if (i915.verbose_state_checks) \ - WARN(1, format); \ - else \ + if (unlikely(__ret_warn_on)) \ + if (!WARN(i915.verbose_state_checks, format)) \ DRM_ERROR(format); \ - } \ unlikely(__ret_warn_on); \ }) #define I915_STATE_WARN_ON(condition) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) { \ - if (i915.verbose_state_checks) \ - WARN(1, "WARN_ON(" #condition ")\n"); \ - else \ + if (unlikely(__ret_warn_on)) \ + if (!WARN(i915.verbose_state_checks, \ + "WARN_ON(" #condition ")\n")) \ DRM_ERROR("WARN_ON(" #condition ")\n"); \ - } \ unlikely(__ret_warn_on); \ })