Re: [PATCH v2] sunrpc: simplify dprintk() macros and cleanup redundant debug guards

[Sean Chang]

On Fri, Mar 13, 2026 at 12:14 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> On Thu, Mar 12, 2026 at 11:54:15PM +0800, Sean Chang wrote:
> > On Thu, Mar 12, 2026 at 5:47 AM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxx> wrote:
> > > On Tue, Mar 03, 2026 at 10:07:25PM +0800, Sean Chang wrote:
> > > > Following David Laight's suggestion, simplify the macro definitions by
> > > > removing the unnecessary 'fmt' argument and using no_printk(__VA_ARGS__)
> > > > directly.
> > > >
> > > > Verification with .lst files under -O2 confirms that the compiler
> > > > successfully performs "dead code elimination". Even when variables
> > > > (like char buf[] in nfsfh.c) or static helper functions (like
> > > > nlmdbg_cookie2a() in svclock.c) are declared without #ifdef, they are
> > > > completely optimized out (no stack allocation, no symbol references in
> > > > the final executable) as they are only referenced within no_printk().
> > >
> > > Does this patch fixes also 202603110038.P6d14oxa-lkp@xxxxxxxxx?
> >
> > Regarding the LKP report:
> > I have reproduced the Sparse warning (void vs int mismatch) locally.
> > To resolve this, I'll use the do-while(0) form in v3 to ensure the macro
> > always evaluates to void:
> > -# define dfprintk(fac, ...)            no_printk(__VA_ARGS__)
> > -# define dfprintk_rcu(fac, ...)        no_printk(__VA_ARGS__)
> > +# define dfprintk(fac, ...)            do { no_printk(__VA_ARGS__); } while (0)
> > +# define dfprintk_rcu(fac, ...)        do { no_printk(__VA_ARGS__); } while (0)
>
> Wouldn't be better to drop ({}) in nfs_error*() macros?
>

Hi Andy,
I understand that the ({}) wrapper might look redundant at first
glance. However,
even if I remove it, the return type remains an issue because no_printk() (which
dprintk() expands to) already contains its own ({}) statement expression.

To resolve this without refactoring errorf(), which hasn't been
touched in years,
I believe modifying dfprintk() is the better path.

One alternative is to explicitly force the return type to void like this:
({ dprintk(fmt "\n", ##__VA_ARGS__); (void)0; })

While this ensures the return type remains void (consistent with the
behavior before
dprintk() was redefined), it is admittedly clunky. We could wrap
(void)0 in a macro like
#define nothing_to_do ((void)0), but that adds unnecessary complexity.

Therefore, I still prefer Option 1, as it restores the original
behavior from before the
recent commits and maintains the cleanest code structure for this subsystem.
What do you think?

On Fri, Mar 13, 2026 at 1:53 AM Chuck Lever <cel@xxxxxxxxxx> wrote:
> >
> > Hi Chuck,
> > To make the review and merging process easier across different
> > subsystems, I will
> > send v3 as a 3-patch series:
> > [PATCH v3 1/3] sunrpc: Fix dprintk type mismatch using do-while(0)
> >     include/linux/sunrpc/debug.h
> > [PATCH v3 2/3] nfsd/lockd: Remove redundant debug checks
> >     fs/nfsd/nfsfh.c, fs/lockd/svclock.c
> > [PATCH v3 3/3] sunrpc/xprtrdma: Simplify variable declarations and debug checks
> >     net/sunrpc/xprtrdma/svc_rdma_transport.c
>
> I can take all three of those, if that's what you'd like. Just let
> me know when the review is complete.
>

Hi Chuck,
That would be great, thank you! I will send out the v3 series once the
discussion with Andy is settled.

Best Regards,
Sean