| Message ID | 1439304272-9645-2-git-send-email-arun.siluvery@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/08/15 15:44, Arun Siluvery wrote: > From: Mika Kuoppala <mika.kuoppala@intel.com> > > Prevent leaking the if scoping by containing the WA_REG > macro inside its own scope. > > Reported-by: Arun Siluvery <arun.siluvery@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 1c14233..cf61262 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv, > return 0; > } > > -#define WA_REG(addr, mask, val) { \ > +#define WA_REG(addr, mask, val) do { \ > const int r = wa_add(dev_priv, (addr), (mask), (val)); \ > if (r) \ > return r; \ > - } > + } while(0) > > #define WA_SET_BIT_MASKED(addr, mask) \ > WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask)) On the one hand, yes, this definitely needs the do-while wrapper. OTOH, hiding a conditional 'return' inside a macro is an abomination :( At least it's only local to this file ... So, on the grounds that this makes it more correct if no less ugly: Reviewed-by: Dave Gordon <david.s.gordon@intel.com> .Dave.
On Wed, Aug 12, 2015 at 04:40:03PM +0100, Dave Gordon wrote: > On 11/08/15 15:44, Arun Siluvery wrote: > >From: Mika Kuoppala <mika.kuoppala@intel.com> > > > >Prevent leaking the if scoping by containing the WA_REG > >macro inside its own scope. > > > >Reported-by: Arun Siluvery <arun.siluvery@linux.intel.com> > >Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >--- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >index 1c14233..cf61262 100644 > >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >@@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv, > > return 0; > > } > > > >-#define WA_REG(addr, mask, val) { \ > >+#define WA_REG(addr, mask, val) do { \ > > const int r = wa_add(dev_priv, (addr), (mask), (val)); \ > > if (r) \ > > return r; \ > >- } > >+ } while(0) > > > > #define WA_SET_BIT_MASKED(addr, mask) \ > > WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask)) > > On the one hand, yes, this definitely needs the do-while wrapper. > > OTOH, hiding a conditional 'return' inside a macro is an abomination :( At > least it's only local to this file ... > > So, on the grounds that this makes it more correct if no less ugly: > > Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Queued for -next, thanks for the patch. And I fixed the checkpatch ERROR, tsk ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 1c14233..cf61262 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv, return 0; } -#define WA_REG(addr, mask, val) { \ +#define WA_REG(addr, mask, val) do { \ const int r = wa_add(dev_priv, (addr), (mask), (val)); \ if (r) \ return r; \ - } + } while(0) #define WA_SET_BIT_MASKED(addr, mask) \ WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))