> >--- a/drivers/gpu/drm/xe/xe_macros.h
> >+++ b/drivers/gpu/drm/xe/xe_macros.h
> >@@ -11,8 +11,12 @@
> > #define XE_WARN_ON WARN_ON
> >
> > #define XE_IOCTL_DBG(xe, cond) \
> >- ((cond) && (drm_dbg(&(xe)->drm, \
> >- "Ioctl argument check failed at %s:%d: %s", \
> >- __FILE__, __LINE__, #cond), 1))
> >+({ \
> >+ if ((cond)) \
> >+ drm_dbg(&(xe)->drm, \
> >+ "Ioctl argument check failed at %s:%d: %s", \
> >+ __FILE__, __LINE__, #cond); \
> >+ (cond); \
>
> there's a double cond evaluation here and given any expression can be
> given to XE_IOCTL_DBG(), this doens't look very safe. I think this would
> be safer as:
>
> #define XE_IOCTL_DBG(xe, cond) ({ \
> int cond__ = !!(cond); \
> if (cond__) \
> drm_dbg(&(xe)->drm, \
> "Ioctl argument check failed at %s:%d: %s", \
> __FILE__, __LINE__, #cond); \
> cond__; \
> })
>
> as it then evaluates cond just once. Also the generated code seems to be
> sane compared to what we had before too.
>
Yes, if (cond) has operator like ++, it will be a bug. I miss that...
I will revise a patch again by referring to your review, thanks.
> And I also needed this to build-test:
>
> | diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> | index 08cfea04e22bd..82585d442f017 100644
> | --- a/drivers/gpu/drm/drm_print.c
> | +++ b/drivers/gpu/drm/drm_print.c
> | @@ -215,9 +215,8 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
> | {
> | const struct drm_device *drm = p->arg;
> | const struct device *dev = drm ? drm->dev : NULL;
> | - enum drm_debug_category category = p->category;
> |
> | - if (!__drm_debug_enabled(category))
> | + if (!__drm_debug_enabled(p->category))
> | return;
> |
> | __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf);
>
> as otherwise it complains category is unused.
>
I also submitted a seperate patch to fix '__drm_debug_enabled' macro,
from '#define __drm_debug_enabled(category) true'
to '#define __drm_debug_enabled(category) ({ void(category); true; })'
This removes the build error caused by the unused 'category', too.
Anyway, it can be build. I tested both cases.
I realize now that these two patches should have been submitted as a
patch series
I'm sorry for my mistakes.
Thanks,
Gyeyoung Baek
> Lucas De Marchi
