[PATCH v5 0/5] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[PATCH v5 0/5] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[Sean Chang]

This series cleans up redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
across sunrpc, nfsd, and lockd, as these checks are already handled
within the dprintk macros.

Additionally, it refactors the nfs_errorf() macros into a safer
do-while(0) pattern and removes unused nfs_warnf() macros to improve
code maintainability.

v5:
- Reformat the cleanup of __maybe_unused into a formal 'Revert' patch as requested.
- Update the macro refactoring commit message to include historical context
  (commit ce8866f0913f) and use backticks for `git grep`.

v4:
- Add a missing patch to include/linux/sunrpc/debug.h to ensure dprintk()
  properly handles variable referencing via no_printk().
- Remove obsolete __maybe_unused from fs/nfsd/export.c (revert ebae102897e7)
  as suggested by Andy Shevchenko.
- Add Reviewed-by and Tested-by tags from Andy Shevchenko.

v3:
- Added nfs_errorf refactoring and removed unused nfs_warnf macros.
- Split sunrpc and nfsd changes for better clarity.

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.

Sean Chang (5):
  sunrpc: Fix dprintk type mismatch using do-while(0)
  nfsd/lockd: Remove redundant debug checks
  svcrdma: Remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
  nfs: Refactor nfs_errorf macros and remove unused ones
  Revert "nfsd: Mark variable __maybe_unused to avoid W=1 build break"

 fs/lockd/svclock.c                       |  7 ------
 fs/nfs/internal.h                        | 28 +++++++++++-------------
 fs/nfsd/export.c                         |  2 +-
 fs/nfsd/nfsfh.c                          |  8 +++----
 include/linux/sunrpc/debug.h             |  8 ++-----
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 ++++++++++-----------
 6 files changed, 30 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH v5 1/5] sunrpc: Fix dprintk type mismatch using do-while(0)

[Sean Chang]

Following David Laight's suggestion, simplify the macro definitions by removing
the unnecessary 'fmt' argument and using no_printk(VA_ARGS) directly.

To resolve a Sparse warning (void vs int mismatch) when dfprintk is used in
conditional statements, wrap the non-debug definition in a do-while(0) block.
This ensures the macro always evaluates to a void expression.

Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 include/linux/sunrpc/debug.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

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
 
 /*
-- 
2.34.1

[PATCH v5 2/5] nfsd/lockd: Remove redundant debug checks

[Sean Chang]

Remove unnecessary IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards and #ifdefs
in nfsfh.c and svclock.c.

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>
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/lockd/svclock.c | 7 -------
 fs/nfsd/nfsfh.c    | 8 +++-----
 2 files changed, 3 insertions(+), 12 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;
 	}
 
-- 
2.34.1

[PATCH v5 3/5] svcrdma: Remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards

[Sean Chang]

Remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards in
svc_rdma_accept(). Since dprintk() already evaluates to a no-op
(via no_printk) when debugging is disabled, these explicit guards
are unnecessary.

Verification with .lst files under -O2 confirms that the compiler
successfully performs "dead code elimination". Even when variables
(like 'sap' in this case) are declared outside of #ifdef, they are
completely optimized out (no stack allocation, no symbol references
in the final executable) as they are only referenced within dprintk().

Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 +++++++++++-------------
 1 file changed, 11 insertions(+), 14 deletions(-)

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

[PATCH v5 4/5] nfs: Refactor nfs_errorf macros and remove unused ones

[Sean Chang]

Refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
pattern for safer macro expansion and kernel style compliance.

Additionally, remove nfs_warnf() and nfs_fwarnf() as `git grep`
confirms they have no callers in the current tree. Furthermore,
these functions have remained unused since the introduction in
commit ce8866f0913f ("NFS: Attach supplementary error information
to fs_context.").

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603110038.P6d14oxa-lkp@intel.com/
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfs/internal.h | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 63e09dfc27a8..59ab43542390 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -161,13 +161,19 @@ enum nfs_lock_status {
 	NFS_LOCK_NOLOCK		= 2,
 };
 
-#define nfs_errorf(fc, fmt, ...) ((fc)->log.log ?		\
-	errorf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dprintk(fmt "\n", ## __VA_ARGS__); }))
-
-#define nfs_ferrorf(fc, fac, fmt, ...) ((fc)->log.log ?		\
-	errorf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__); }))
+#define nfs_errorf(fc, fmt, ...) do { \
+	if ((fc)->log.log) \
+		errorf(fc, fmt, ## __VA_ARGS__); \
+	else \
+		dprintk(fmt "\n", ## __VA_ARGS__); \
+} while (0)
+
+#define nfs_ferrorf(fc, fac, fmt, ...) do { \
+	if ((fc)->log.log) \
+		errorf(fc, fmt, ## __VA_ARGS__); \
+	else \
+		dfprintk(fac, fmt "\n", ## __VA_ARGS__); \
+} while (0)
 
 #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?		\
 	invalf(fc, fmt, ## __VA_ARGS__) :			\
@@ -177,14 +183,6 @@ enum nfs_lock_status {
 	invalf(fc, fmt, ## __VA_ARGS__) :			\
 	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
 
-#define nfs_warnf(fc, fmt, ...) ((fc)->log.log ?		\
-	warnf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dprintk(fmt "\n", ## __VA_ARGS__); }))
-
-#define nfs_fwarnf(fc, fac, fmt, ...) ((fc)->log.log ?		\
-	warnf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__); }))
-
 static inline struct nfs_fs_context *nfs_fc2context(const struct fs_context *fc)
 {
 	return fc->fs_private;
-- 
2.34.1

[PATCH v5 5/5] Revert "nfsd: Mark variable __maybe_unused to avoid W=1 build break"

[Sean Chang]

This reverts commit ebae102897e760e9e6bc625f701dd666b2163bd1.

The __maybe_unused attributes are no longer needed because dprintk()
now uses no_printk(), which ensures variables are referenced by the
compiler even when debugging is disabled.

Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfsd/export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 8fdbba7cad96..8116e5bcbe00 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1025,7 +1025,7 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
 {
 	struct svc_export	*exp;
 	struct path		path;
-	struct inode		*inode __maybe_unused;
+	struct inode		*inode;
 	struct svc_fh		fh;
 	int			err;
 	struct nfsd_net		*nn = net_generic(net, nfsd_net_id);
-- 
2.34.1

Re: [PATCH v5 1/5] sunrpc: Fix dprintk type mismatch using do-while(0)

[Chuck Lever]

On 3/21/26 10:15 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.

Generally we prefer a commit message to open with an explanation of why
the change is needed. Your first paragraph instead opens with what was
done ("Following David Laight's suggestion, simplify ...") rather than
why the change is necessary. The Sparse warning motivation is buried in
the second paragraph. Consider leading with a problem statement in this
and subsequent patches in this series.


> To resolve a Sparse warning (void vs int mismatch) when dfprintk is used in
> conditional statements, wrap the non-debug definition in a do-while(0) block.
> This ensures the macro always evaluates to a void expression.

The non-debug definitions in the diff below are:

> -# 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__)

These are not wrapped in do { ... } while (0), and no_printk()
evaluates to int (0), not void. The do-while(0) wrapping that
was discussed on the list and fixes the Sparse warning appears
to be in a later patch in this series (the nfs_errorf
refactoring), not in this one.

Should the commit message second paragraph be removed or revised
to reflect what this patch actually does?


> Suggested-by: David Laight <david.laight.linux@gmail.com>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
>  include/linux/sunrpc/debug.h | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> 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
>  
>  /*


-- 
Chuck Lever

Re: [PATCH v5 4/5] nfs: Refactor nfs_errorf macros and remove unused ones

[Chuck Lever]

On 3/21/26 10:15 AM, Sean Chang wrote:
> Refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
> pattern for safer macro expansion and kernel style compliance.
> 
> Additionally, remove nfs_warnf() and nfs_fwarnf() as `git grep`
> confirms they have no callers in the current tree. Furthermore,
> these functions have remained unused since the introduction in
> commit ce8866f0913f ("NFS: Attach supplementary error information
> to fs_context.").
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603110038.P6d14oxa-lkp@intel.com/
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
>  fs/nfs/internal.h | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)

I need an Acked-by: from the NFS client maintainers on this one.


-- 
Chuck Lever

Re: [PATCH v5 5/5] Revert "nfsd: Mark variable __maybe_unused to avoid W=1 build break"

[Chuck Lever]

On 3/21/26 10:15 AM, Sean Chang wrote:
> This reverts commit ebae102897e760e9e6bc625f701dd666b2163bd1.
> 
> The __maybe_unused attributes are no longer needed because dprintk()
> now uses no_printk(), which ensures variables are referenced by the
> compiler even when debugging is disabled.

Some commit message improvements are needed:

This revert is safe only because ("sunrpc: Fix dprintk type mismatch
using do-while(0)") already changed the non-debug dfprintk path to use
no_printk(__VA_ARGS__). The commit message doesn't reference that
enabling commit by SHA or subject. If this revert is cherry-picked or
backported without that pre-requisite, the W=1 build warning returns
silently.

The commit message says "dprintk() now uses no_printk()", but this is
true only for the !CONFIG_SUNRPC_DEBUG path. When CONFIG_SUNRPC_DEBUG is
enabled, dfprintk expands inode directly via __sunrpc_printk, not
through no_printk.


> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
>  fs/nfsd/export.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 8fdbba7cad96..8116e5bcbe00 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1025,7 +1025,7 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
>  {
>  	struct svc_export	*exp;
>  	struct path		path;
> -	struct inode		*inode __maybe_unused;
> +	struct inode		*inode;
>  	struct svc_fh		fh;
>  	int			err;
>  	struct nfsd_net		*nn = net_generic(net, nfsd_net_id);


-- 
Chuck Lever

Re: [PATCH v5 0/5] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[Jeff Layton]

On Sat, 2026-03-21 at 22:15 +0800, Sean Chang wrote:
> This series cleans up redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
> across sunrpc, nfsd, and lockd, as these checks are already handled
> within the dprintk macros.
> 
> Additionally, it refactors the nfs_errorf() macros into a safer
> do-while(0) pattern and removes unused nfs_warnf() macros to improve
> code maintainability.
> 
> v5:
> - Reformat the cleanup of __maybe_unused into a formal 'Revert' patch as requested.
> - Update the macro refactoring commit message to include historical context
>   (commit ce8866f0913f) and use backticks for `git grep`.
> 
> v4:
> - Add a missing patch to include/linux/sunrpc/debug.h to ensure dprintk()
>   properly handles variable referencing via no_printk().
> - Remove obsolete __maybe_unused from fs/nfsd/export.c (revert ebae102897e7)
>   as suggested by Andy Shevchenko.
> - Add Reviewed-by and Tested-by tags from Andy Shevchenko.
> 
> v3:
> - Added nfs_errorf refactoring and removed unused nfs_warnf macros.
> - Split sunrpc and nfsd changes for better clarity.
> 
> 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.
> 
> Sean Chang (5):
>   sunrpc: Fix dprintk type mismatch using do-while(0)
>   nfsd/lockd: Remove redundant debug checks
>   svcrdma: Remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
>   nfs: Refactor nfs_errorf macros and remove unused ones
>   Revert "nfsd: Mark variable __maybe_unused to avoid W=1 build break"
> 
>  fs/lockd/svclock.c                       |  7 ------
>  fs/nfs/internal.h                        | 28 +++++++++++-------------
>  fs/nfsd/export.c                         |  2 +-
>  fs/nfsd/nfsfh.c                          |  8 +++----
>  include/linux/sunrpc/debug.h             |  8 ++-----
>  net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 ++++++++++-----------
>  6 files changed, 30 insertions(+), 48 deletions(-)

This all looks pretty sane to me.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v5 1/5] sunrpc: Fix dprintk type mismatch using do-while(0)

[Sean Chang]

On Sun, Mar 22, 2026 at 12:38 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 3/21/26 10:15 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.
>
> Generally we prefer a commit message to open with an explanation of why
> the change is needed. Your first paragraph instead opens with what was
> done ("Following David Laight's suggestion, simplify ...") rather than
> why the change is necessary. The Sparse warning motivation is buried in
> the second paragraph. Consider leading with a problem statement in this
> and subsequent patches in this series.
>
>
> > To resolve a Sparse warning (void vs int mismatch) when dfprintk is used in
> > conditional statements, wrap the non-debug definition in a do-while(0) block.
> > This ensures the macro always evaluates to a void expression.
>
> The non-debug definitions in the diff below are:
>
> > -# 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__)
>
> These are not wrapped in do { ... } while (0), and no_printk()
> evaluates to int (0), not void. The do-while(0) wrapping that
> was discussed on the list and fixes the Sparse warning appears
> to be in a later patch in this series (the nfs_errorf
> refactoring), not in this one.
>
> Should the commit message second paragraph be removed or revised
> to reflect what this patch actually does?
>

Hi Chuck,
I notice that the commit messages are wrong because the current situation
is not the same as when I first sent it, so I will fix this message to the
correct one.

It may be like:
When RPC debugging is enabled, the dfprintk macros include redundant
else-branches that call no_printk(). Since no_printk() is a no-op
designed specifically for compile-time type checking, it is unnecessary
to invoke it explicitly when the ifdebug(fac) condition is not met
within a debug-enabled build.

Drop the explicit 'fmt' argument to allow the compiler to handle all
arguments directly through __VA_ARGS__. Since no_printk()
internally performs the same format string validation, passing the
entire argument list is more efficient and reduces macro complexity.

Best Regards,
Sean

Re: [PATCH v5 4/5] nfs: Refactor nfs_errorf macros and remove unused ones

[Sean Chang]

On Sun, Mar 22, 2026 at 12:38 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 3/21/26 10:15 AM, Sean Chang wrote:
> > Refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
> > pattern for safer macro expansion and kernel style compliance.
> >
> > Additionally, remove nfs_warnf() and nfs_fwarnf() as `git grep`
> > confirms they have no callers in the current tree. Furthermore,
> > these functions have remained unused since the introduction in
> > commit ce8866f0913f ("NFS: Attach supplementary error information
> > to fs_context.").
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202603110038.P6d14oxa-lkp@intel.com/
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> > ---
> >  fs/nfs/internal.h | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
>
> I need an Acked-by: from the NFS client maintainers on this one.
>

Hi Trond, Anna,

Could you please take a look at this refactoring patch for fs/nfs/internal.h?

This patch addresses a Sparse warning reported by the kernel test robot
where a ternary operation in the macros resulted in inconsistent types
(void vs int).
I have refactored the macros to use the standard do-while(0) pattern.

Andy Shevchenko has already reviewed and tested this, and Chuck is
looking for an Acked-by from the NFS client side to proceed with merging.

I'd appreciate your feedback or an Acked-by if this looks good to you.

Best Regards,
Sean

Re: [PATCH v5 5/5] Revert "nfsd: Mark variable __maybe_unused to avoid W=1 build break"

[Sean Chang]

On Sun, Mar 22, 2026 at 12:38 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 3/21/26 10:15 AM, Sean Chang wrote:
> > This reverts commit ebae102897e760e9e6bc625f701dd666b2163bd1.
> >
> > The __maybe_unused attributes are no longer needed because dprintk()
> > now uses no_printk(), which ensures variables are referenced by the
> > compiler even when debugging is disabled.
>
> Some commit message improvements are needed:
>
> This revert is safe only because ("sunrpc: Fix dprintk type mismatch
> using do-while(0)") already changed the non-debug dfprintk path to use
> no_printk(__VA_ARGS__). The commit message doesn't reference that
> enabling commit by SHA or subject. If this revert is cherry-picked or
> backported without that pre-requisite, the W=1 build warning returns
> silently.
>
> The commit message says "dprintk() now uses no_printk()", but this is
> true only for the !CONFIG_SUNRPC_DEBUG path. When CONFIG_SUNRPC_DEBUG is
> enabled, dfprintk expands inode directly via __sunrpc_printk, not
> through no_printk.
>

Hi Chunk,

Thanks for pointing out these issues. I will update the commit message
to be more precise and clearly state the dependencies.

The corrected version will be:
The __maybe_unused attributes are no longer needed for the
!CONFIG_SUNRPC_DEBUG case. This revert depends on a prerequisite
change in this series: "sunrpc: Fix dprintk type mismatch using do-while(0)"

That change updated the non-debug dfprintk path to use no_printk(),
which ensures that arguments are always referenced by the compiler
for type checking, even when debugging is disabled.