linux-kernel - Re: [PATCH v2 29/59] dyndbg: change __dynamic_func_call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJfuBxyHzkNOrjsKQ5TmBX8UOpJDKnxZr-t+eQ52Hfkq3BVC-Q@mail.gmail.com>
Date: Tue, 25 Mar 2025 10:23:30 -0600
From: jim.cromie@...il.com
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	amd-gfx@...ts.freedesktop.org, intel-gvt-dev@...ts.freedesktop.org, 
	intel-gfx@...ts.freedesktop.org, intel-gfx-trybot@...ts.freedesktop.org, 
	jbaron@...mai.com, gregkh@...uxfoundation.org, ukaszb@...omium.org, 
	daniel.vetter@...ll.ch, tvrtko.ursulin@...ux.intel.com, jani.nikula@...el.com, 
	ville.syrjala@...ux.intel.com
Subject: Re: [PATCH v2 29/59] dyndbg: change __dynamic_func_call_cls* macros
 into expressions

On Mon, Mar 24, 2025 at 9:19 AM Louis Chauvet <louis.chauvet@...tlin.com> wrote:
>
>
>
> Le 20/03/2025 à 19:52, Jim Cromie a écrit :
> > The Xe driver's XE_IOCTL_DBG macro calls drm_dbg() from inside an if
> > (expression).  This breaks when CONFIG_DRM_USE_DYNAMIC_DEBUG=y because
> > the invoked macro has a do-while-0 wrapper.
> >
> >     if (cond && (drm_dbg("expr-form"),1)) {
> >        ... do some more stuff
> >     }
> >
> > Fix for this usage by changing __dynamic_func_call_cls{,_no_desc}
> > macros into expressions, by replacing the do-while-0s with a ({ })
> > wrapper.  In the common usage, the trailing ';' converts the
> > expression into a statement.
> >
> >     drm_dbg("statement form");
> >
> > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> > ---
> > ---
> >   include/linux/dynamic_debug.h | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > index 8043966a0fd6..80bcaad03400 100644
> > --- a/include/linux/dynamic_debug.h
> > +++ b/include/linux/dynamic_debug.h
> > @@ -339,20 +339,20 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
> >    * (|_cls): adds in _DPRINT_CLASS_DFLT as needed
> >    * (|_no_desc):     former gets callsite descriptor as 1st arg (for prdbgs)
> >    */
> > -#define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {        \
> > -     DEFINE_DYNAMIC_DEBUG_METADATA_CLS((id), cls, fmt);      \
> > +#define __dynamic_func_call_cls(id, cls, fmt, func, ...) ({  \
> > +     DEFINE_DYNAMIC_DEBUG_METADATA_CLS(id, cls, fmt);        \
>
> Is it normal to remove the parenthesis around id? Or the other way
> around, why did you add parenthesis in PATCH 17?
>

heisen-thinking ?
noisy inputs ?
historically, checkpatch warnings on macros have given me difficulty
so I tend toward defense.
I think this one was a red-herring.

> >       if (DYNAMIC_DEBUG_BRANCH(id))                           \
> > -             func(&id, ##__VA_ARGS__);                       \
> > -} while (0)
> > +             func(&(id), ##__VA_ARGS__);                     \
> > +})
> >   #define __dynamic_func_call(id, fmt, func, ...)                             \
> >       __dynamic_func_call_cls(id, _DPRINTK_CLASS_DFLT, fmt,           \
> >                               func, ##__VA_ARGS__)
> >
> > -#define __dynamic_func_call_cls_no_desc(id, cls, fmt, func, ...) do {        \
> > +#define __dynamic_func_call_cls_no_desc(id, cls, fmt, func, ...) ({  \
> >       DEFINE_DYNAMIC_DEBUG_METADATA_CLS(id, cls, fmt);                \
>
> I expect the same constraints around id here, both with parenthesis, or
> no parenthesis at all.
>

yes, inconsistent.

> >       if (DYNAMIC_DEBUG_BRANCH(id))                                   \
> >               func(__VA_ARGS__);                                      \
> > -} while (0)
> > +})
> >   #define __dynamic_func_call_no_desc(id, fmt, func, ...)                     \
> >       __dynamic_func_call_cls_no_desc(id, _DPRINTK_CLASS_DFLT,        \
> >                                       fmt, func, ##__VA_ARGS__)
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>