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

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

[Sean Chang]

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().

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
v2:
 - Follow reversed xmas tree order for variables in svc_rdma_transport.c
   as requested by Andy Shevchenko.
 - Polish commit message: use dprintk() and remove redundant file list.
 - Correct the technical claim about dprintk() type checking.

 fs/lockd/svclock.c                       |  7 -------
 fs/nfsd/nfsfh.c                          |  8 +++-----
 include/linux/sunrpc/debug.h             |  8 ++------
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 +++++++++++-------------
 4 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index ee23f5802af1..9b978a087b3c 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -47,7 +47,6 @@ static const struct rpc_call_ops nlmsvc_grant_ops;
 static LIST_HEAD(nlm_blocked);
 static DEFINE_SPINLOCK(nlm_blocked_lock);
 
-#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
 {
 	/*
@@ -74,12 +73,6 @@ static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
 
 	return buf;
 }
-#else
-static inline const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
-{
-	return "???";
-}
-#endif
 
 /*
  * Insert a blocked lock into the global list
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 68b629fbaaeb..91514326d1b4 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -105,12 +105,10 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
 {
 	/* Check if the request originated from a secure port. */
 	if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) {
-		if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
-			char buf[RPC_MAX_ADDRBUFLEN];
+		char buf[RPC_MAX_ADDRBUFLEN];
 
-			dprintk("nfsd: request from insecure port %s!\n",
-			        svc_print_addr(rqstp, buf, sizeof(buf)));
-		}
+		dprintk("nfsd: request from insecure port %s!\n",
+			svc_print_addr(rqstp, buf, sizeof(buf)));
 		return nfserr_perm;
 	}
 
diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
index ab61bed2f7af..f6f2a106eeaf 100644
--- a/include/linux/sunrpc/debug.h
+++ b/include/linux/sunrpc/debug.h
@@ -38,8 +38,6 @@ extern unsigned int		nlm_debug;
 do {									\
 	ifdebug(fac)							\
 		__sunrpc_printk(fmt, ##__VA_ARGS__);			\
-	else								\
-		no_printk(fmt, ##__VA_ARGS__);				\
 } while (0)
 
 # define dfprintk_rcu(fac, fmt, ...)					\
@@ -48,15 +46,13 @@ do {									\
 		rcu_read_lock();					\
 		__sunrpc_printk(fmt, ##__VA_ARGS__);			\
 		rcu_read_unlock();					\
-	} else {							\
-		no_printk(fmt, ##__VA_ARGS__);				\
 	}								\
 } while (0)
 
 #else
 # define ifdebug(fac)		if (0)
-# define dfprintk(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
-# define dfprintk_rcu(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
+# define dfprintk(fac, ...)		no_printk(__VA_ARGS__)
+# define dfprintk_rcu(fac, ...)	no_printk(__VA_ARGS__)
 #endif
 
 /*
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index f2d72181a6fe..0759444bda50 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -413,6 +413,7 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 	struct rpcrdma_connect_private pmsg;
 	struct ib_qp_init_attr qp_attr;
 	struct ib_device *dev;
+	struct sockaddr *sap;
 	int ret = 0;
 
 	listen_rdma = container_of(xprt, struct svcxprt_rdma, sc_xprt);
@@ -559,20 +560,16 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 		goto errout;
 	}
 
-	if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
-		struct sockaddr *sap;
-
-		dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
-		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
-		dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
-		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
-		dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
-		dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
-		dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
-		dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
-		dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
-		dprintk("    ord             : %d\n", conn_param.initiator_depth);
-	}
+	dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
+	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
+	dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
+	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
+	dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
+	dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
+	dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
+	dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
+	dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
+	dprintk("    ord             : %d\n", conn_param.initiator_depth);
 
 	return &newxprt->sc_xprt;
 
-- 
2.34.1

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

[Andy Shevchenko]

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@intel.com?

-- 
With Best Regards,
Andy Shevchenko

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

[Chuck Lever]

On Tue, Mar 3, 2026, at 9:07 AM, 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().
>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Suggested-by: David Laight <david.laight.linux@gmail.com>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
> v2:
>  - Follow reversed xmas tree order for variables in svc_rdma_transport.c
>    as requested by Andy Shevchenko.
>  - Polish commit message: use dprintk() and remove redundant file list.
>  - Correct the technical claim about dprintk() type checking.
>
>  fs/lockd/svclock.c                       |  7 -------
>  fs/nfsd/nfsfh.c                          |  8 +++-----
>  include/linux/sunrpc/debug.h             |  8 ++------
>  net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 +++++++++++-------------
>  4 files changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index ee23f5802af1..9b978a087b3c 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -47,7 +47,6 @@ static const struct rpc_call_ops nlmsvc_grant_ops;
>  static LIST_HEAD(nlm_blocked);
>  static DEFINE_SPINLOCK(nlm_blocked_lock);
> 
> -#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>  static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
>  {
>  	/*
> @@ -74,12 +73,6 @@ static const char *nlmdbg_cookie2a(const struct 
> nlm_cookie *cookie)
> 
>  	return buf;
>  }
> -#else
> -static inline const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
> -{
> -	return "???";
> -}
> -#endif
> 
>  /*
>   * Insert a blocked lock into the global list
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 68b629fbaaeb..91514326d1b4 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -105,12 +105,10 @@ static __be32 nfsd_setuser_and_check_port(struct 
> svc_rqst *rqstp,
>  {
>  	/* Check if the request originated from a secure port. */
>  	if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) {
> -		if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
> -			char buf[RPC_MAX_ADDRBUFLEN];
> +		char buf[RPC_MAX_ADDRBUFLEN];
> 
> -			dprintk("nfsd: request from insecure port %s!\n",
> -			        svc_print_addr(rqstp, buf, sizeof(buf)));
> -		}
> +		dprintk("nfsd: request from insecure port %s!\n",
> +			svc_print_addr(rqstp, buf, sizeof(buf)));
>  		return nfserr_perm;
>  	}
> 
> diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
> index ab61bed2f7af..f6f2a106eeaf 100644
> --- a/include/linux/sunrpc/debug.h
> +++ b/include/linux/sunrpc/debug.h
> @@ -38,8 +38,6 @@ extern unsigned int		nlm_debug;
>  do {									\
>  	ifdebug(fac)							\
>  		__sunrpc_printk(fmt, ##__VA_ARGS__);			\
> -	else								\
> -		no_printk(fmt, ##__VA_ARGS__);				\
>  } while (0)
> 
>  # define dfprintk_rcu(fac, fmt, ...)					\
> @@ -48,15 +46,13 @@ do {									\
>  		rcu_read_lock();					\
>  		__sunrpc_printk(fmt, ##__VA_ARGS__);			\
>  		rcu_read_unlock();					\
> -	} else {							\
> -		no_printk(fmt, ##__VA_ARGS__);				\
>  	}								\
>  } while (0)
> 
>  #else
>  # define ifdebug(fac)		if (0)
> -# define dfprintk(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
> -# define dfprintk_rcu(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
> +# define dfprintk(fac, ...)		no_printk(__VA_ARGS__)
> +# define dfprintk_rcu(fac, ...)	no_printk(__VA_ARGS__)
>  #endif
> 
>  /*
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c 
> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index f2d72181a6fe..0759444bda50 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -413,6 +413,7 @@ static struct svc_xprt *svc_rdma_accept(struct 
> svc_xprt *xprt)
>  	struct rpcrdma_connect_private pmsg;
>  	struct ib_qp_init_attr qp_attr;
>  	struct ib_device *dev;
> +	struct sockaddr *sap;
>  	int ret = 0;
> 
>  	listen_rdma = container_of(xprt, struct svcxprt_rdma, sc_xprt);
> @@ -559,20 +560,16 @@ static struct svc_xprt *svc_rdma_accept(struct 
> svc_xprt *xprt)
>  		goto errout;
>  	}
> 
> -	if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
> -		struct sockaddr *sap;
> -
> -		dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
> -		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
> -		dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
> -		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
> -		dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
> -		dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
> -		dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
> -		dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
> -		dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
> -		dprintk("    ord             : %d\n", conn_param.initiator_depth);
> -	}
> +	dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
> +	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
> +	dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
> +	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
> +	dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
> +	dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
> +	dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
> +	dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
> +	dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
> +	dprintk("    ord             : %d\n", conn_param.initiator_depth);
> 
>  	return &newxprt->sc_xprt;
> 
> -- 
> 2.34.1

Has a subsystem tree been chosen through which to merge these two patches?


-- 
Chuck Lever

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

[Sean Chang]

On Thu, Mar 12, 2026 at 5:47 AM Andy Shevchenko
<andriy.shevchenko@intel.com> 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@intel.com?
>

Hi Andy,
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)


On Thu, Mar 12, 2026 at 9:20 PM Chuck Lever <cel@kernel.org> wrote:
>
> Has a subsystem tree been chosen through which to merge these two patches?
>

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

Best Regards,
Sean

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

[Andy Shevchenko]

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@intel.com> 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@intel.com?
> 
> 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?

-- 
With Best Regards,
Andy Shevchenko

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

[Chuck Lever]

On Thu, Mar 12, 2026, at 11:54 AM, Sean Chang wrote:
> On Thu, Mar 12, 2026 at 9:20 PM Chuck Lever <cel@kernel.org> wrote:
>>
>> Has a subsystem tree been chosen through which to merge these two patches?
>>
>
> 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.


-- 
Chuck Lever

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@intel.com> 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@intel.com> 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@intel.com?
> >
> > 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@kernel.org> 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

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

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 09:04:07PM +0800, Sean Chang wrote:
> On Fri, Mar 13, 2026 at 12:14 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> 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@intel.com> wrote:
> > > > On Tue, Mar 03, 2026 at 10:07:25PM +0800, Sean Chang wrote:

...

> > > > Does this patch fixes also 202603110038.P6d14oxa-lkp@intel.com?
> > >
> > > 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?
> 
> 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?

Personally I found the ({}) in nfs_error*() redundant and the point of those
functions not being touched in years doesn't work for internal APIs. Do you
know the reason why it wasn't touched? Perhaps there was nothing to do simply
with that until dprintk() issue reveals some (legacy?) stuff to improve.

I would not go with (void)0 approach, but I also think that dropping unneeded
(if confirmed) expression wrapping is a good thing to do.

-- 
With Best Regards,
Andy Shevchenko

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

[Sean Chang]

On Tue, Mar 17, 2026 at 10:20 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> > > > > Does this patch fixes also 202603110038.P6d14oxa-lkp@intel.com?
> > > >
> > > > 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?
> >
> > 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?
>
> Personally I found the ({}) in nfs_error*() redundant and the point of those
> functions not being touched in years doesn't work for internal APIs. Do you
> know the reason why it wasn't touched? Perhaps there was nothing to do simply
> with that until dprintk() issue reveals some (legacy?) stuff to improve.
>
> I would not go with (void)0 approach, but I also think that dropping unneeded
> (if confirmed) expression wrapping is a good thing to do.
>

Hi Andy,
Thanks for your comment, how about removing the ({}) and ternary operator to
prevent sparse throw incompatible types. And use the do{} while(0) to prevent
dangling-else problem.

#define nfs_errorf(fc, fmt, ...) do { \
    if ((fc)->log.log) \
        errorf(fc, fmt, ## __VA_ARGS__); \
    else \
        dprintk(fmt "\n", ## __VA_ARGS__); \
} while (0)

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

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 12:21:41AM +0800, Sean Chang wrote:
> On Tue, Mar 17, 2026 at 10:20 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > > > > > Does this patch fixes also 202603110038.P6d14oxa-lkp@intel.com?
> > > > >
> > > > > 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?
> > >
> > > 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?
> >
> > Personally I found the ({}) in nfs_error*() redundant and the point of those
> > functions not being touched in years doesn't work for internal APIs. Do you
> > know the reason why it wasn't touched? Perhaps there was nothing to do simply
> > with that until dprintk() issue reveals some (legacy?) stuff to improve.
> >
> > I would not go with (void)0 approach, but I also think that dropping unneeded
> > (if confirmed) expression wrapping is a good thing to do.

> Thanks for your comment, how about removing the ({}) and ternary operator to
> prevent sparse throw incompatible types. And use the do{} while(0) to prevent
> dangling-else problem.

LGTM.

> #define nfs_errorf(fc, fmt, ...) do { \
>     if ((fc)->log.log) \
>         errorf(fc, fmt, ## __VA_ARGS__); \
>     else \
>         dprintk(fmt "\n", ## __VA_ARGS__); \
> } while (0)

-- 
With Best Regards,
Andy Shevchenko