[RFC PATCH 00/13] Fix parentheses around macro parameter use in headers

[RFC PATCH 00/13] Fix parentheses around macro parameter use in headers

[Mathieu Desnoyers]

I tried to look at various list and list user headers in the kernel tree
to figure out what the proper parentheses pattern should be around macro
parameter use for new code, and it turns out that the current code base
lacks consistency.

This is not an exhaustive change of all public kernel headers, but at
least it is a start, updating those which are implementing or using
kernel lists.

The basic rules followed here are:

- Use parentheses around arguments which are rvalues, except when those
  are expressions between commas "," used as arguments to functions or
  other macros, or surrounded by brackets "[]".
- Do not use parentheses around arguments which are lvalues.

For consistency, when a macro argument is used both as an lvalue and as
an rvalue within the macro, use the parentheses rules applying to each
of the context: with parentheses for rvalue, without parentheses for
lvalue.

Mathieu Desnoyers (13):
  rcu: rcupdate.h: Fix parentheses around macro parameter use
  rculist.h: Fix parentheses around macro pointer parameter use
  rculist_nulls.h: Add parentheses around macro pointer parameter use
  rculist_bl.h: Fix parentheses around macro pointer parameter use
  list.h: Fix parentheses around macro pointer parameter use
  list_nulls.h: Fix parentheses around macro pointer parameter use
  list_bl.h: Fix parentheses around macro pointer parameter use
  llist.h: Fix parentheses around macro pointer parameter use
  klist.h: Fix parentheses around macro parameter use
  resource_ext.h: Remove useless parentheses around macro parameters
  netdevice.h: Fix parentheses around macro parameter use
  blk-mq.h: Fix parentheses around macro parameter use
  bio.h: Fix parentheses around macro parameter use

 include/linux/bio.h           | 22 +++++++-------
 include/linux/blk-mq.h        | 38 ++++++++++++------------
 include/linux/klist.h         |  8 +++---
 include/linux/list.h          | 54 +++++++++++++++++------------------
 include/linux/list_bl.h       | 12 ++++----
 include/linux/list_nulls.h    |  8 +++---
 include/linux/llist.h         | 14 ++++-----
 include/linux/netdevice.h     | 12 ++++----
 include/linux/rculist.h       | 28 +++++++++---------
 include/linux/rculist_bl.h    |  6 ++--
 include/linux/rculist_nulls.h |  4 +--
 include/linux/rcupdate.h      | 46 ++++++++++++++---------------
 include/linux/resource_ext.h  |  6 ++--
 13 files changed, 129 insertions(+), 129 deletions(-)

-- 
2.25.1

[RFC PATCH 01/13] rcu: rcupdate.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

linux/rcupdate.h macros use the *p parameter without parentheses, e.g.:

  typeof(*p)

rather than

  typeof(*(p))

The following test-case shows how it can generate confusion due to C
operator precedence being reversed compared to the expectations:

    #define m(p) \
    do { \
            __typeof__(*p) v = 0; \
    } while (0)

    void fct(unsigned long long *p1)
    {
            m(p1 + 1);      /* works */
            m(1 + p1);      /* broken */
    }

Add missing parentheses around macro parameter use in the following
pattern to ensure operator precedence behaves as expected:

- m(&p) is changed for m(&(p)).

Remove useless parentheses in the following macro argument usage
patterns:

- m((x), y) is changed for m(x, y), because comma has the lowest
  operator precedence, and thus the extra parentheses are useless.
- "(_r_a_p__v) where _r_a_p__v is a variable and not a macro parameter,
  is changed for "_r_a_p__v",

An extra whitespace is also removed in kvfree_rcu_arg_2() for
consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Zqiang <qiang1.zhang@intel.com>
---
 include/linux/rcupdate.h | 46 ++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index dcd2cf1e8326..4f26c3080953 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -215,7 +215,7 @@ void rcu_tasks_trace_qs_blkd(struct task_struct *t);
 										\
 		if (likely(!READ_ONCE((t)->trc_reader_special.b.need_qs)) &&	\
 		    likely(!___rttq_nesting)) {					\
-			rcu_trc_cmpxchg_need_qs((t), 0,	TRC_NEED_QS_CHECKED);	\
+			rcu_trc_cmpxchg_need_qs(t, 0, TRC_NEED_QS_CHECKED);	\
 		} else if (___rttq_nesting && ___rttq_nesting != INT_MIN &&	\
 			   !READ_ONCE((t)->trc_reader_special.b.blocked)) {	\
 			rcu_tasks_trace_qs_blkd(t);				\
@@ -227,7 +227,7 @@ void rcu_tasks_trace_qs_blkd(struct task_struct *t);
 
 #define rcu_tasks_qs(t, preempt)					\
 do {									\
-	rcu_tasks_classic_qs((t), (preempt));				\
+	rcu_tasks_classic_qs(t, preempt);				\
 	rcu_tasks_trace_qs(t);						\
 } while (0)
 
@@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { }
 
 #ifdef __CHECKER__
 #define rcu_check_sparse(p, space) \
-	((void)(((typeof(*p) space *)p) == p))
+	((void)(((typeof(*(p)) space *)(p)) == (p)))
 #else /* #ifdef __CHECKER__ */
 #define rcu_check_sparse(p, space)
 #endif /* #else #ifdef __CHECKER__ */
 
 #define __unrcu_pointer(p, local)					\
 ({									\
-	typeof(*p) *local = (typeof(*p) *__force)(p);			\
+	typeof(*(p)) *local = (typeof(*(p)) *__force)(p);		\
 	rcu_check_sparse(p, __rcu);					\
-	((typeof(*p) __force __kernel *)(local)); 			\
+	((typeof(*(p)) __force __kernel *)(local));			\
 })
 /**
  * unrcu_pointer - mark a pointer as not being RCU protected
@@ -452,29 +452,29 @@ static inline void rcu_preempt_sleep_check(void) { }
 
 #define __rcu_access_pointer(p, local, space) \
 ({ \
-	typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
+	typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \
 	rcu_check_sparse(p, space); \
-	((typeof(*p) __force __kernel *)(local)); \
+	((typeof(*(p)) __force __kernel *)(local)); \
 })
 #define __rcu_dereference_check(p, local, c, space) \
 ({ \
 	/* Dependency order vs. p above. */ \
-	typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
+	typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \
 	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
 	rcu_check_sparse(p, space); \
-	((typeof(*p) __force __kernel *)(local)); \
+	((typeof(*(p)) __force __kernel *)(local)); \
 })
 #define __rcu_dereference_protected(p, local, c, space) \
 ({ \
 	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \
 	rcu_check_sparse(p, space); \
-	((typeof(*p) __force __kernel *)(p)); \
+	((typeof(*(p)) __force __kernel *)(p)); \
 })
 #define __rcu_dereference_raw(p, local) \
 ({ \
 	/* Dependency order vs. p above. */ \
 	typeof(p) local = READ_ONCE(p); \
-	((typeof(*p) __force __kernel *)(local)); \
+	((typeof(*(p)) __force __kernel *)(local)); \
 })
 #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu))
 
@@ -520,10 +520,10 @@ do {									      \
 	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
 	rcu_check_sparse(p, __rcu);					      \
 									      \
-	if (__builtin_constant_p(v) && (_r_a_p__v) == (uintptr_t)NULL)	      \
-		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
+	if (__builtin_constant_p(v) && _r_a_p__v == (uintptr_t)NULL)	      \
+		WRITE_ONCE(p, (typeof(p))_r_a_p__v);			      \
 	else								      \
-		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
+		smp_store_release(&(p), RCU_INITIALIZER((typeof(p))_r_a_p__v));\
 } while (0)
 
 /**
@@ -539,8 +539,8 @@ do {									      \
  */
 #define rcu_replace_pointer(rcu_ptr, ptr, c)				\
 ({									\
-	typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c));	\
-	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	typeof(ptr) __tmp = rcu_dereference_protected(rcu_ptr, c);	\
+	rcu_assign_pointer(rcu_ptr, ptr);				\
 	__tmp;								\
 })
 
@@ -571,7 +571,7 @@ do {									      \
  * down multi-linked structures after a grace period has elapsed.  However,
  * rcu_dereference_protected() is normally preferred for this use case.
  */
-#define rcu_access_pointer(p) __rcu_access_pointer((p), __UNIQUE_ID(rcu), __rcu)
+#define rcu_access_pointer(p) __rcu_access_pointer(p, __UNIQUE_ID(rcu), __rcu)
 
 /**
  * rcu_dereference_check() - rcu_dereference with debug checking
@@ -607,7 +607,7 @@ do {									      \
  * annotated as __rcu.
  */
 #define rcu_dereference_check(p, c) \
-	__rcu_dereference_check((p), __UNIQUE_ID(rcu), \
+	__rcu_dereference_check(p, __UNIQUE_ID(rcu), \
 				(c) || rcu_read_lock_held(), __rcu)
 
 /**
@@ -623,7 +623,7 @@ do {									      \
  * rcu_read_lock() but also rcu_read_lock_bh() into account.
  */
 #define rcu_dereference_bh_check(p, c) \
-	__rcu_dereference_check((p), __UNIQUE_ID(rcu), \
+	__rcu_dereference_check(p, __UNIQUE_ID(rcu), \
 				(c) || rcu_read_lock_bh_held(), __rcu)
 
 /**
@@ -639,7 +639,7 @@ do {									      \
  * only rcu_read_lock() but also rcu_read_lock_sched() into account.
  */
 #define rcu_dereference_sched_check(p, c) \
-	__rcu_dereference_check((p), __UNIQUE_ID(rcu), \
+	__rcu_dereference_check(p, __UNIQUE_ID(rcu), \
 				(c) || rcu_read_lock_sched_held(), \
 				__rcu)
 
@@ -651,7 +651,7 @@ do {									      \
  * rcu_read_lock_held().
  */
 #define rcu_dereference_raw_check(p) \
-	__rcu_dereference_check((p), __UNIQUE_ID(rcu), 1, __rcu)
+	__rcu_dereference_check(p, __UNIQUE_ID(rcu), 1, __rcu)
 
 /**
  * rcu_dereference_protected() - fetch RCU pointer when updates prevented
@@ -670,7 +670,7 @@ do {									      \
  * but very ugly failures.
  */
 #define rcu_dereference_protected(p, c) \
-	__rcu_dereference_protected((p), __UNIQUE_ID(rcu), (c), __rcu)
+	__rcu_dereference_protected(p, __UNIQUE_ID(rcu), c, __rcu)
 
 
 /**
@@ -1021,7 +1021,7 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
 #define KVFREE_GET_MACRO(_1, _2, NAME, ...) NAME
 #define kvfree_rcu_arg_2(ptr, rhf)					\
 do {									\
-	typeof (ptr) ___p = (ptr);					\
+	typeof(ptr) ___p = (ptr);					\
 									\
 	if (___p) {									\
 		BUILD_BUG_ON(!__is_kvfree_rcu_offset(offsetof(typeof(*(ptr)), rhf)));	\
-- 
2.25.1

[RFC PATCH 02/13] rculist.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member

This corrects the following usage pattern where operator precedence is
unexpected:

  LIST_HEAD(testlist);

  struct test {
          struct list_head node;
          int a;
  };

  // pos->member issue
  void f(void)
  {
          struct test *t1;
          struct test **t2 = &t1;

          list_for_each_entry_rcu((*t2), &testlist, node) {       /* works */
                  //...
          }
          list_for_each_entry_rcu(*t2, &testlist, node) { /* broken */
                  //...
          }
  }

The typeof(*pos) lack of parentheses around "pos" is not an issue per se
in the specific macros modified here because "pos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Zqiang <qiang1.zhang@intel.com>
Cc: rcu@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/rculist.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index d29740be4833..d27aeff5447d 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -388,9 +388,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  */
 #define list_for_each_entry_rcu(pos, head, member, cond...)		\
 	for (__list_check_rcu(dummy, ## cond, 0),			\
-	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
-		&pos->member != (head);					\
-		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
+	     pos = list_entry_rcu((head)->next, typeof(*(pos)), member);\
+		&(pos)->member != (head);				\
+		pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_srcu	-	iterate over rcu list of given type
@@ -407,9 +407,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  */
 #define list_for_each_entry_srcu(pos, head, member, cond)		\
 	for (__list_check_srcu(cond),					\
-	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
-		&pos->member != (head);					\
-		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
+	     pos = list_entry_rcu((head)->next, typeof(*(pos)), member);\
+		&(pos)->member != (head);				\
+		pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_entry_lockless - get the struct for this entry
@@ -441,9 +441,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * but never deleted.
  */
 #define list_for_each_entry_lockless(pos, head, member) \
-	for (pos = list_entry_lockless((head)->next, typeof(*pos), member); \
-	     &pos->member != (head); \
-	     pos = list_entry_lockless(pos->member.next, typeof(*pos), member))
+	for (pos = list_entry_lockless((head)->next, typeof(*(pos)), member); \
+	     &(pos)->member != (head); \
+	     pos = list_entry_lockless((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue_rcu - continue iteration over list of given type
@@ -464,9 +464,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * position.
  */
 #define list_for_each_entry_continue_rcu(pos, head, member) 		\
-	for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \
-	     &pos->member != (head);	\
-	     pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
+	for (pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member); \
+	     &(pos)->member != (head);	\
+	     pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_from_rcu - iterate over a list from current point
@@ -486,8 +486,8 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * after the given position.
  */
 #define list_for_each_entry_from_rcu(pos, head, member)			\
-	for (; &(pos)->member != (head);					\
-		pos = list_entry_rcu(pos->member.next, typeof(*(pos)), member))
+	for (; &(pos)->member != (head);				\
+		pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * hlist_del_rcu - deletes entry from hash list without re-initialization
-- 
2.25.1

[RFC PATCH 03/13] rculist_nulls.h: Add parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add parentheses around use of macro argument "tpos" in those patterns to
ensure operator precedence behaves as expected:

- typeof(*tpos)

The typeof(*tpos) lack of parentheses around "tpos" is not an issue per se
in the specific macros modified here because "tpos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency with other list iteration code across
the kernel tree.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Zqiang <qiang1.zhang@intel.com>
Cc: rcu@vger.kernel.org
---
 include/linux/rculist_nulls.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index ba4c00dd8005..6e56dd2daecf 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -168,7 +168,7 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
 	for (({barrier();}),							\
 	     pos = rcu_dereference_raw(hlist_nulls_first_rcu(head));		\
 		(!is_a_nulls(pos)) &&						\
-		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1; }); \
+		({ tpos = hlist_nulls_entry(pos, typeof(*(tpos)), member); 1; }); \
 		pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos)))
 
 /**
@@ -183,7 +183,7 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
 	for (({barrier();}),							\
 	     pos = rcu_dereference_raw(hlist_nulls_first_rcu(head));		\
 		(!is_a_nulls(pos)) &&						\
-		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member);	\
+		({ tpos = hlist_nulls_entry(pos, typeof(*(tpos)), member);	\
 		   pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos)); 1; });)
 #endif
 #endif
-- 
2.25.1

[RFC PATCH 04/13] rculist_bl.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "tpos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*tpos)
- pos->next
- x && y is changed for (x) && (y).

The typeof(*tpos) lack of parentheses around "tpos" is not an issue per se
in the specific macros modified here because "tpos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Zqiang <qiang1.zhang@intel.com>
Cc: rcu@vger.kernel.org
---
 include/linux/rculist_bl.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/rculist_bl.h b/include/linux/rculist_bl.h
index 0b952d06eb0b..798c0a03bf5c 100644
--- a/include/linux/rculist_bl.h
+++ b/include/linux/rculist_bl.h
@@ -94,8 +94,8 @@ static inline void hlist_bl_add_head_rcu(struct hlist_bl_node *n,
  */
 #define hlist_bl_for_each_entry_rcu(tpos, pos, head, member)		\
 	for (pos = hlist_bl_first_rcu(head);				\
-		pos &&							\
-		({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \
-		pos = rcu_dereference_raw(pos->next))
+		(pos) &&						\
+		({ tpos = hlist_bl_entry(pos, typeof(*(tpos)), member); 1; }); \
+		pos = rcu_dereference_raw((pos)->next))
 
 #endif
-- 
2.25.1

[RFC PATCH 05/13] list.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency,
- x && y is changed for (x) && (y).

Remove useless parentheses around use of macro parameter (head) in the
following pattern:

- list_is_head(pos, (head))

Because comma is the lowest priority operator already, so the extra pair
of parentheses is redundant.

This corrects the following usage pattern where operator precedence is
unexpected:

  LIST_HEAD(testlist);

  struct test {
          struct list_head node;
          int a;
  };

  // pos->member issue
  void f(void)
  {
          struct test *t1;
          struct test **t2 = &t1;

          list_for_each_entry((*t2), &testlist, node) {   /* works */
                  //...
          }
          list_for_each_entry(*t2, &testlist, node) {     /* broken */
                  //...
          }
  }

  // typeof(*pos) issue
  void f2(void)
  {
          struct test *t1 = NULL, *t2;

          t2 = list_prepare_entry((0 + t1), &testlist, node);     /* works */
          t2 = list_prepare_entry(0 + t1, &testlist, node);       /* broken */
  }

Note that the macros in which "pos" is also used as an lvalue probably
don't suffer from the lack of parentheses around "pos" in typeof(*pos),
but add those nevertheless to keep everything consistent.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Ricardo Martinez <ricardo.martinez@linux.intel.com>
---
 include/linux/list.h | 54 ++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index f10344dbad4d..d197b1a6f411 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -603,7 +603,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each(pos, head) \
-	for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (head)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_rcu - Iterate over a list in an RCU-safe fashion
@@ -612,8 +612,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_rcu(pos, head)		  \
 	for (pos = rcu_dereference((head)->next); \
-	     !list_is_head(pos, (head)); \
-	     pos = rcu_dereference(pos->next))
+	     !list_is_head(pos, head); \
+	     pos = rcu_dereference((pos)->next))
 
 /**
  * list_for_each_continue - continue iteration over a list
@@ -623,7 +623,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * Continue to iterate over a list, continuing after the current position.
  */
 #define list_for_each_continue(pos, head) \
-	for (pos = pos->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (pos)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_prev	-	iterate over a list backwards
@@ -631,7 +631,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev(pos, head) \
-	for (pos = (head)->prev; !list_is_head(pos, (head)); pos = pos->prev)
+	for (pos = (head)->prev; !list_is_head(pos, head); pos = (pos)->prev)
 
 /**
  * list_for_each_safe - iterate over a list safe against removal of list entry
@@ -640,9 +640,9 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_safe(pos, n, head) \
-	for (pos = (head)->next, n = pos->next; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->next)
+	for (pos = (head)->next, n = (pos)->next; \
+	     !list_is_head(pos, head); \
+	     pos = (n), n = (pos)->next)
 
 /**
  * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
@@ -651,9 +651,9 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev_safe(pos, n, head) \
-	for (pos = (head)->prev, n = pos->prev; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->prev)
+	for (pos = (head)->prev, n = (pos)->prev; \
+	     !list_is_head(pos, head); \
+	     pos = (n), n = (pos)->prev)
 
 /**
  * list_count_nodes - count nodes in the list
@@ -677,7 +677,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_entry_is_head(pos, head, member)				\
-	(&pos->member == (head))
+	(&(pos)->member == (head))
 
 /**
  * list_for_each_entry	-	iterate over list of given type
@@ -686,7 +686,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry(pos, head, member)				\
-	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member);			\
 	     pos = list_next_entry(pos, member))
 
@@ -697,7 +697,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_reverse(pos, head, member)			\
-	for (pos = list_last_entry(head, typeof(*pos), member);		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member); 			\
 	     pos = list_prev_entry(pos, member))
 
@@ -710,7 +710,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * Prepares a pos entry for use as a start point in list_for_each_entry_continue().
  */
 #define list_prepare_entry(pos, head, member) \
-	((pos) ? : list_entry(head, typeof(*pos), member))
+	((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -773,10 +773,10 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_safe(pos, n, head, member)			\
-	for (pos = list_first_entry(head, typeof(*pos), member),	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member),	\
 		n = list_next_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_continue - continue list iteration safe against removal
@@ -792,7 +792,7 @@ static inline size_t list_count_nodes(struct list_head *head)
 	for (pos = list_next_entry(pos, member), 				\
 		n = list_next_entry(pos, member);				\
 	     !list_entry_is_head(pos, head, member);				\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_from - iterate over list from current point safe against removal
@@ -807,7 +807,7 @@ static inline size_t list_count_nodes(struct list_head *head)
 #define list_for_each_entry_safe_from(pos, n, head, member) 			\
 	for (n = list_next_entry(pos, member);					\
 	     !list_entry_is_head(pos, head, member);				\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_reverse - iterate backwards over list safe against removal
@@ -820,10 +820,10 @@ static inline size_t list_count_nodes(struct list_head *head)
  * of list entry.
  */
 #define list_for_each_entry_safe_reverse(pos, n, head, member)		\
-	for (pos = list_last_entry(head, typeof(*pos), member),		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member),	\
 		n = list_prev_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
-	     pos = n, n = list_prev_entry(n, member))
+	     pos = (n), n = list_prev_entry(n, member))
 
 /**
  * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
@@ -1033,11 +1033,11 @@ static inline void hlist_move_list(struct hlist_head *old,
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head) \
-	for (pos = (head)->first; pos ; pos = pos->next)
+	for (pos = (head)->first; pos ; pos = (pos)->next)
 
 #define hlist_for_each_safe(pos, n, head) \
-	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
-	     pos = n)
+	for (pos = (head)->first; (pos) && ({ n = (pos)->next; 1; }); \
+	     pos = (n))
 
 #define hlist_entry_safe(ptr, type, member) \
 	({ typeof(ptr) ____ptr = (ptr); \
@@ -1082,8 +1082,8 @@ static inline void hlist_move_list(struct hlist_head *old,
  * @member:	the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry_safe(pos, n, head, member) 		\
-	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
-	     pos && ({ n = pos->member.next; 1; });			\
-	     pos = hlist_entry_safe(n, typeof(*pos), member))
+	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
+	     (pos) && ({ n = (pos)->member.next; 1; });			\
+	     pos = hlist_entry_safe(n, typeof(*(pos)), member))
 
 #endif
-- 
2.25.1

Re: [RFC PATCH 05/13] list.h: Fix parentheses around macro pointer parameter use

[Andy Shevchenko]

On Thu, May 04, 2023 at 04:05:19PM -0400, Mathieu Desnoyers wrote:
> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
> 
> - typeof(*pos)
> - pos->member
> - "x = y" is changed for "x = (y)", because "y" can be an expression
>   containing a comma if it is the result of the expansion of a macro such
>   as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>   precedence. This use-case is far-fetched, but we have to choose one
>   way or the other (with or without parentheses) for consistency,
> - x && y is changed for (x) && (y).
> 
> Remove useless parentheses around use of macro parameter (head) in the
> following pattern:
> 
> - list_is_head(pos, (head))
> 
> Because comma is the lowest priority operator already, so the extra pair
> of parentheses is redundant.

But strictly speaking it might be something like

	list_...(..., (a, b))

where (a, b) is the head. No?

> This corrects the following usage pattern where operator precedence is
> unexpected:
> 
>   LIST_HEAD(testlist);
> 
>   struct test {
>           struct list_head node;
>           int a;
>   };
> 
>   // pos->member issue
>   void f(void)
>   {
>           struct test *t1;
>           struct test **t2 = &t1;
> 
>           list_for_each_entry((*t2), &testlist, node) {   /* works */
>                   //...
>           }
>           list_for_each_entry(*t2, &testlist, node) {     /* broken */
>                   //...

Me still in doubt. But it's up to maintainers.

>           }
>   }
> 
>   // typeof(*pos) issue
>   void f2(void)
>   {
>           struct test *t1 = NULL, *t2;
> 
>           t2 = list_prepare_entry((0 + t1), &testlist, node);     /* works */
>           t2 = list_prepare_entry(0 + t1, &testlist, node);       /* broken */
>   }
> 
> Note that the macros in which "pos" is also used as an lvalue probably
> don't suffer from the lack of parentheses around "pos" in typeof(*pos),
> but add those nevertheless to keep everything consistent.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 05/13] list.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

On 2023-05-08 08:16, Andy Shevchenko wrote:
> On Thu, May 04, 2023 at 04:05:19PM -0400, Mathieu Desnoyers wrote:
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>> - "x = y" is changed for "x = (y)", because "y" can be an expression
>>    containing a comma if it is the result of the expansion of a macro such
>>    as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>>    precedence. This use-case is far-fetched, but we have to choose one
>>    way or the other (with or without parentheses) for consistency,
>> - x && y is changed for (x) && (y).
>>
>> Remove useless parentheses around use of macro parameter (head) in the
>> following pattern:
>>
>> - list_is_head(pos, (head))
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
> 
> But strictly speaking it might be something like
> 
> 	list_...(..., (a, b))
> 
> where (a, b) is the head. No?

The following case still works after removing the extra parentheses 
around "head" because the parentheses are present where the macro is used:

LIST_HEAD(testlist);

int f2(void)
{
         return 1;
}

void f(void)
{
    struct list_head *pos;

    list_for_each(pos, (f2(), &testlist)) {
            //...
    }
}

The only use I found that would break is as follows:

LIST_HEAD(testlist);

int f2(void)
{
         return 1;
}

#define eval(...)       __VA_ARGS__

void f(void)
{
    struct list_head *pos;

    list_for_each(pos, eval(f2(), &testlist)) {
            //...
    }
}

Because "eval()" will evaluate "f(), &testlist" with comma and all, 
without enclosing parentheses.

So the question is: do we want to support this kind-of-odd macro 
evaluation, considering that it requires adding parentheses around 
pretty much all macro parameters when used as expressions between commas?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 05/13] list.h: Fix parentheses around macro pointer parameter use

[Andy Shevchenko]

On Mon, May 08, 2023 at 09:46:40AM -0400, Mathieu Desnoyers wrote:
> On 2023-05-08 08:16, Andy Shevchenko wrote:

...

> The only use I found that would break is as follows:
> 
> LIST_HEAD(testlist);
> 
> int f2(void)
> {
>         return 1;
> }
> 
> #define eval(...)       __VA_ARGS__
> 
> void f(void)
> {
>    struct list_head *pos;
> 
>    list_for_each(pos, eval(f2(), &testlist)) {
>            //...
>    }
> }
> 
> Because "eval()" will evaluate "f(), &testlist" with comma and all, without
> enclosing parentheses.
> 
> So the question is: do we want to support this kind-of-odd macro evaluation,
> considering that it requires adding parentheses around pretty much all macro
> parameters when used as expressions between commas?

Similar question can be asked for your initial motivation to support indirect
pointers. I found the double pointer as weird as this macro case. But it can be
only me. Hence I left this to the more experienced developers to express their
opinions.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 05/13] list.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

On 2023-05-12 07:02, Andy Shevchenko wrote:
> On Mon, May 08, 2023 at 09:46:40AM -0400, Mathieu Desnoyers wrote:
>> On 2023-05-08 08:16, Andy Shevchenko wrote:
> 
> ...
> 
>> The only use I found that would break is as follows:
>>
>> LIST_HEAD(testlist);
>>
>> int f2(void)
>> {
>>          return 1;
>> }
>>
>> #define eval(...)       __VA_ARGS__
>>
>> void f(void)
>> {
>>     struct list_head *pos;
>>
>>     list_for_each(pos, eval(f2(), &testlist)) {
>>             //...
>>     }
>> }
>>
>> Because "eval()" will evaluate "f(), &testlist" with comma and all, without
>> enclosing parentheses.
>>
>> So the question is: do we want to support this kind-of-odd macro evaluation,
>> considering that it requires adding parentheses around pretty much all macro
>> parameters when used as expressions between commas?
> 
> Similar question can be asked for your initial motivation to support indirect
> pointers. I found the double pointer as weird as this macro case. But it can be
> only me. Hence I left this to the more experienced developers to express their
> opinions.
> 

The main motivation behind my changes is to make macro code consistent, 
and to eliminate classes of issues that can arise from unexpected 
operator precedence around use of macro arguments that lack parentheses.

The examples I provide in the commit messages are just instances showing 
how the lack of parentheses can lead to unexpected effects due to 
operator precedence.

My end goal is not to "support" specific use-cases. My goal is to 
eliminate inconsistency, increase robustness of the kernel macros, and 
lessen the cognitive burden that comes with using and maintaining those 
macros.

I hope that we can spend less time figuring out operator precedence 
corner-cases and more brain power thinking about and documenting things 
that really matter like memory barriers and synchronization.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

[RFC PATCH 06/13] list_nulls.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*tpos)
- pos->next

The typeof(*tpos) lack of parentheses around "tpos" is not an issue per se
in the specific macros modified here because "tpos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Dumazet <edumazet@google.com>
---
 include/linux/list_nulls.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/list_nulls.h b/include/linux/list_nulls.h
index fa6e8471bd22..74ffde881e44 100644
--- a/include/linux/list_nulls.h
+++ b/include/linux/list_nulls.h
@@ -127,8 +127,8 @@ static inline void hlist_nulls_del(struct hlist_nulls_node *n)
 #define hlist_nulls_for_each_entry(tpos, pos, head, member)		       \
 	for (pos = (head)->first;					       \
 	     (!is_a_nulls(pos)) &&					       \
-		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+		({ tpos = hlist_nulls_entry(pos, typeof(*(tpos)), member); 1;}); \
+	     pos = (pos)->next)
 
 /**
  * hlist_nulls_for_each_entry_from - iterate over a hlist continuing from current point
@@ -139,7 +139,7 @@ static inline void hlist_nulls_del(struct hlist_nulls_node *n)
  */
 #define hlist_nulls_for_each_entry_from(tpos, pos, member)	\
 	for (; (!is_a_nulls(pos)) && 				\
-		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+		({ tpos = hlist_nulls_entry(pos, typeof(*(tpos)), member); 1;}); \
+	     pos = (pos)->next)
 
 #endif
-- 
2.25.1

[RFC PATCH 07/13] list_bl.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "tpos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*tpos)
- pos->next
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.
- x && y is changed for (x) && (y).

The typeof(*tpos) lack of parentheses around "tpos" is not an issue per se
in the specific macros modified here because "tpos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nick Piggin <npiggin@kernel.dk>
---
 include/linux/list_bl.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/list_bl.h b/include/linux/list_bl.h
index ae1b541446c9..450cca15496b 100644
--- a/include/linux/list_bl.h
+++ b/include/linux/list_bl.h
@@ -168,9 +168,9 @@ static inline bool hlist_bl_is_locked(struct hlist_bl_head *b)
  */
 #define hlist_bl_for_each_entry(tpos, pos, head, member)		\
 	for (pos = hlist_bl_first(head);				\
-	     pos &&							\
-		({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+	     (pos) &&							\
+		({ tpos = hlist_bl_entry(pos, typeof(*(tpos)), member); 1;}); \
+	     pos = (pos)->next)
 
 /**
  * hlist_bl_for_each_entry_safe - iterate over list of given type safe against removal of list entry
@@ -182,8 +182,8 @@ static inline bool hlist_bl_is_locked(struct hlist_bl_head *b)
  */
 #define hlist_bl_for_each_entry_safe(tpos, pos, n, head, member)	 \
 	for (pos = hlist_bl_first(head);				 \
-	     pos && ({ n = pos->next; 1; }) && 				 \
-		({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = n)
+	     (pos) && ({ n = (pos)->next; 1; }) &&			 \
+		({ tpos = hlist_bl_entry(pos, typeof(*(tpos)), member); 1;}); \
+	     pos = (n))
 
 #endif
-- 
2.25.1

[RFC PATCH 08/13] llist.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.

The typeof(*pos) lack of parentheses around "pos" is not an issue per se
in the specific macros modified here because "pos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

Remove useless parentheses around use of macro parameter (node) in the
following patterns:

- llist_entry((node), typeof(*(pos)), member) is changed for
  llist_entry(node, typeof(*(pos)), member),
- "(pos) = (node)" is changed for "pos = (node)".

Because comma is the lowest priority operator already, so the extra pair
of parentheses is redundant.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/llist.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 85bda2d02d65..4181b34b557f 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -114,7 +114,7 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each(pos, node)			\
-	for ((pos) = (node); pos; (pos) = (pos)->next)
+	for (pos = (node); pos; pos = (pos)->next)
 
 /**
  * llist_for_each_safe - iterate over some deleted entries of a lock-less list
@@ -133,7 +133,7 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each_safe(pos, n, node)			\
-	for ((pos) = (node); (pos) && ((n) = (pos)->next, true); (pos) = (n))
+	for (pos = (node); (pos) && (n = (pos)->next, true); pos = (n))
 
 /**
  * llist_for_each_entry - iterate over some deleted entries of lock-less list of given type
@@ -151,9 +151,9 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each_entry(pos, node, member)				\
-	for ((pos) = llist_entry((node), typeof(*(pos)), member);	\
+	for (pos = llist_entry(node, typeof(*(pos)), member);	\
 	     member_address_is_nonnull(pos, member);			\
-	     (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))
+	     pos = llist_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * llist_for_each_entry_safe - iterate over some deleted entries of lock-less list of given type
@@ -173,10 +173,10 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each_entry_safe(pos, n, node, member)			       \
-	for (pos = llist_entry((node), typeof(*pos), member);		       \
+	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
 	     member_address_is_nonnull(pos, member) &&			       \
-	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
-	     pos = n)
+		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
+	     pos = (n))
 
 /**
  * llist_empty - tests whether a lock-less list is empty
-- 
2.25.1

[RFC PATCH 09/13] klist.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

Add missing parentheses around "_name" parameter use in KLIST_INIT().
It keeps list macros consistent, and prevents unexpected operator
precedence situations, for example:

  struct klist a[1] = { KLIST_INIT(*a, NULL, NULL) };

Where the "." operator within KLIST_INIT() is evaluated before the "*"
operator.

Add missing parentheses around macro parameter use in the following
patterns to ensure operator precedence behaves as expected:

- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/klist.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/klist.h b/include/linux/klist.h
index b0f238f20dbb..d7e0612ca4ff 100644
--- a/include/linux/klist.h
+++ b/include/linux/klist.h
@@ -23,10 +23,10 @@ struct klist {
 } __attribute__ ((aligned (sizeof(void *))));
 
 #define KLIST_INIT(_name, _get, _put)					\
-	{ .k_lock	= __SPIN_LOCK_UNLOCKED(_name.k_lock),		\
-	  .k_list	= LIST_HEAD_INIT(_name.k_list),			\
-	  .get		= _get,						\
-	  .put		= _put, }
+	{ .k_lock	= __SPIN_LOCK_UNLOCKED((_name).k_lock),		\
+	  .k_list	= LIST_HEAD_INIT((_name).k_list),		\
+	  .get		= (_get),					\
+	  .put		= (_put), }
 
 #define DEFINE_KLIST(_name, _get, _put)					\
 	struct klist _name = KLIST_INIT(_name, _get, _put)
-- 
2.25.1

[RFC PATCH 10/13] resource_ext.h: Remove useless parentheses around macro parameters

[Mathieu Desnoyers]

Parentheses around macro parameters which are surrounded by commas is
a scenario where those added parentheses are useless, because the comma
is the operator with the lowest precedence.

Remove those useless parentheses to make list iteration code consistent
across kernel headers.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jiang Liu <jiang.liu@linux.intel.com>
---
 include/linux/resource_ext.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
index ff0339df56af..f4a3c0040886 100644
--- a/include/linux/resource_ext.h
+++ b/include/linux/resource_ext.h
@@ -60,11 +60,11 @@ resource_list_destroy_entry(struct resource_entry *entry)
 	resource_list_free_entry(entry);
 }
 
-#define resource_list_for_each_entry(entry, list)	\
-	list_for_each_entry((entry), (list), node)
+#define resource_list_for_each_entry(entry, list)		\
+	list_for_each_entry(entry, list, node)
 
 #define resource_list_for_each_entry_safe(entry, tmp, list)	\
-	list_for_each_entry_safe((entry), (tmp), (list), node)
+	list_for_each_entry_safe(entry, tmp, list, node)
 
 static inline struct resource_entry *
 resource_list_first_type(struct list_head *list, unsigned long type)
-- 
2.25.1

[RFC PATCH 11/13] netdevice.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

Add missing parentheses around macro parameter use in the following
pattern:

- "x - 1" changed for "(x) - 1",
- "x->member" changed for "(x)->member".

to ensure operator precedence behaves as expected.

Remove useless parentheses around macro parameter use in the following
pattern:

- m((x), y) changed for m(x, y), because comma has the lowest operator
  precedence, which makes the extra comma useless.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
---
 include/linux/netdevice.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 08fbd4622ccf..5a357262a45b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -283,7 +283,7 @@ struct hh_cache {
 	/* cached hardware header; allow for machine alignment needs.        */
 #define HH_DATA_MOD	16
 #define HH_DATA_OFF(__len) \
-	(HH_DATA_MOD - (((__len - 1) & (HH_DATA_MOD - 1)) + 1))
+	(HH_DATA_MOD - ((((__len) - 1) & (HH_DATA_MOD - 1)) + 1))
 #define HH_DATA_ALIGN(__len) \
 	(((__len)+(HH_DATA_MOD-1))&~(HH_DATA_MOD - 1))
 	unsigned long	hh_data[HH_DATA_ALIGN(LL_MAX_HEADER) / sizeof(long)];
@@ -4459,7 +4459,7 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
 }
 
 #define HARD_TX_LOCK(dev, txq, cpu) {			\
-	if ((dev->features & NETIF_F_LLTX) == 0) {	\
+	if (((dev)->features & NETIF_F_LLTX) == 0) {	\
 		__netif_tx_lock(txq, cpu);		\
 	} else {					\
 		__netif_tx_acquire(txq);		\
@@ -4467,12 +4467,12 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
 }
 
 #define HARD_TX_TRYLOCK(dev, txq)			\
-	(((dev->features & NETIF_F_LLTX) == 0) ?	\
+	((((dev)->features & NETIF_F_LLTX) == 0) ?	\
 		__netif_tx_trylock(txq) :		\
 		__netif_tx_acquire(txq))
 
 #define HARD_TX_UNLOCK(dev, txq) {			\
-	if ((dev->features & NETIF_F_LLTX) == 0) {	\
+	if (((dev)->features & NETIF_F_LLTX) == 0) {	\
 		__netif_tx_unlock(txq);			\
 	} else {					\
 		__netif_tx_release(txq);		\
@@ -4534,7 +4534,7 @@ static inline void netif_addr_unlock_bh(struct net_device *dev)
  * rcu_read_lock held.
  */
 #define for_each_dev_addr(dev, ha) \
-		list_for_each_entry_rcu(ha, &dev->dev_addrs.list, list)
+		list_for_each_entry_rcu(ha, &(dev)->dev_addrs.list, list)
 
 /* These functions live elsewhere (drivers/net/net_init.c, but related) */
 
@@ -5237,6 +5237,6 @@ extern struct net_device *blackhole_netdev;
 /* Note: Avoid these macros in fast path, prefer per-cpu or per-queue counters. */
 #define DEV_STATS_INC(DEV, FIELD) atomic_long_inc(&(DEV)->stats.__##FIELD)
 #define DEV_STATS_ADD(DEV, FIELD, VAL) 	\
-		atomic_long_add((VAL), &(DEV)->stats.__##FIELD)
+		atomic_long_add(VAL, &(DEV)->stats.__##FIELD)
 
 #endif	/* _LINUX_NETDEVICE_H */
-- 
2.25.1

Re: [RFC PATCH 11/13] netdevice.h: Fix parentheses around macro parameter use

[Jakub Kicinski]

On Thu,  4 May 2023 16:05:25 -0400 Mathieu Desnoyers wrote:
> Add missing parentheses around macro parameter use in the following
> pattern:
> 
> - "x - 1" changed for "(x) - 1",
> - "x->member" changed for "(x)->member".
> 
> to ensure operator precedence behaves as expected.
> 
> Remove useless parentheses around macro parameter use in the following
> pattern:
> 
> - m((x), y) changed for m(x, y), because comma has the lowest operator
>   precedence, which makes the extra comma useless.

Sure, why not. Can we take it via netdev, tho?
I can't have any dependencies, right?

Re: [RFC PATCH 11/13] netdevice.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

On 2023-05-05 14:44, Jakub Kicinski wrote:
> On Thu,  4 May 2023 16:05:25 -0400 Mathieu Desnoyers wrote:
>> Add missing parentheses around macro parameter use in the following
>> pattern:
>>
>> - "x - 1" changed for "(x) - 1",
>> - "x->member" changed for "(x)->member".
>>
>> to ensure operator precedence behaves as expected.
>>
>> Remove useless parentheses around macro parameter use in the following
>> pattern:
>>
>> - m((x), y) changed for m(x, y), because comma has the lowest operator
>>    precedence, which makes the extra comma useless.
> 
> Sure, why not. Can we take it via netdev, tho?
> I can't have any dependencies, right?

I'll note your Acked-by in my patch for the next round. When I post 
without RFC tag it will be ready for merging. There is still some 
high-level feedback I want to gather on the overall series before I 
remove the RFC tag.

There are no dependencies, each patch in this series only target a 
single header, and there are no dependencies across those patches.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

[RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

Fix the following macro parameter usage patterns in blk-mq.h for
consistency, ensuring that operator precedence is respected:

Added parentheses:

- x->member is changed for (x)->member,
- x.member is changed for (x).member,
- flags >> BLK_MQ_F_ALLOC_POLICY_START_BIT is changed for
  (flags) >> BLK_MQ_F_ALLOC_POLICY_START_BIT.
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.

Removed parentheses:

- m((x)) is changed for m(x) (the extra parentheses are useless),
- m(x, (y), (z)) is changed for m(x, y, z), because comma is the lowest
  priority operator, and thus the extra parentheses are useless,
- v[(x)] is changed for v[x], because the extra parentheses are useless
  given that [] already surrounds an expression,
- "(i) = 0" is changed for "i = 0", because "i" is an lvalue, which
  makes the extra parentheses useless.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
---
 include/linux/blk-mq.h | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 06caacd77ed6..4de6ad92530c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -223,13 +223,13 @@ static inline unsigned short req_get_ioprio(struct request *req)
 
 #define rq_list_add(listptr, rq)	do {		\
 	(rq)->rq_next = *(listptr);			\
-	*(listptr) = rq;				\
+	*(listptr) = (rq);				\
 } while (0)
 
 #define rq_list_add_tail(lastpptr, rq)	do {		\
 	(rq)->rq_next = NULL;				\
-	**(lastpptr) = rq;				\
-	*(lastpptr) = &rq->rq_next;			\
+	**(lastpptr) = (rq);				\
+	*(lastpptr) = &(rq)->rq_next;			\
 } while (0)
 
 #define rq_list_pop(listptr)				\
@@ -251,11 +251,11 @@ static inline unsigned short req_get_ioprio(struct request *req)
 })
 
 #define rq_list_for_each(listptr, pos)			\
-	for (pos = rq_list_peek((listptr)); pos; pos = rq_list_next(pos))
+	for (pos = rq_list_peek(listptr); pos; pos = rq_list_next(pos))
 
 #define rq_list_for_each_safe(listptr, pos, nxt)			\
-	for (pos = rq_list_peek((listptr)), nxt = rq_list_next(pos);	\
-		pos; pos = nxt, nxt = pos ? rq_list_next(pos) : NULL)
+	for (pos = rq_list_peek(listptr), nxt = rq_list_next(pos);	\
+		pos; pos = (nxt), nxt = (pos) ? rq_list_next(pos) : NULL)
 
 #define rq_list_next(rq)	(rq)->rq_next
 #define rq_list_empty(list)	((list) == (struct request *) NULL)
@@ -692,10 +692,10 @@ enum {
 	BLK_MQ_CPU_WORK_BATCH	= 8,
 };
 #define BLK_MQ_FLAG_TO_ALLOC_POLICY(flags) \
-	((flags >> BLK_MQ_F_ALLOC_POLICY_START_BIT) & \
+	(((flags) >> BLK_MQ_F_ALLOC_POLICY_START_BIT) & \
 		((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1))
 #define BLK_ALLOC_POLICY_TO_MQ_FLAG(policy) \
-	((policy & ((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1)) \
+	(((policy) & ((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1)) \
 		<< BLK_MQ_F_ALLOC_POLICY_START_BIT)
 
 #define BLK_MQ_NO_HCTX_IDX	(-1U)
@@ -948,11 +948,11 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
 }
 
 #define queue_for_each_hw_ctx(q, hctx, i)				\
-	xa_for_each(&(q)->hctx_table, (i), (hctx))
+	xa_for_each(&(q)->hctx_table, i, hctx)
 
 #define hctx_for_each_ctx(hctx, ctx, i)					\
-	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
-	     ({ ctx = (hctx)->ctxs[(i)]; 1; }); (i)++)
+	for (i = 0; (i) < (hctx)->nr_ctx &&				\
+	     ({ ctx = (hctx)->ctxs[i]; 1; }); (i)++)
 
 static inline void blk_mq_cleanup_rq(struct request *rq)
 {
@@ -1013,20 +1013,20 @@ struct req_iterator {
 };
 
 #define __rq_for_each_bio(_bio, rq)	\
-	if ((rq->bio))			\
-		for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next)
+	if ((rq)->bio)			\
+		for (_bio = (rq)->bio; _bio; _bio = (_bio)->bi_next)
 
 #define rq_for_each_segment(bvl, _rq, _iter)			\
-	__rq_for_each_bio(_iter.bio, _rq)			\
-		bio_for_each_segment(bvl, _iter.bio, _iter.iter)
+	__rq_for_each_bio((_iter).bio, _rq)			\
+		bio_for_each_segment(bvl, (_iter).bio, (_iter).iter)
 
 #define rq_for_each_bvec(bvl, _rq, _iter)			\
-	__rq_for_each_bio(_iter.bio, _rq)			\
-		bio_for_each_bvec(bvl, _iter.bio, _iter.iter)
+	__rq_for_each_bio((_iter).bio, _rq)			\
+		bio_for_each_bvec(bvl, (_iter).bio, (_iter).iter)
 
 #define rq_iter_last(bvec, _iter)				\
-		(_iter.bio->bi_next == NULL &&			\
-		 bio_iter_last(bvec, _iter.iter))
+		((_iter).bio->bi_next == NULL &&		\
+		 bio_iter_last(bvec, (_iter).iter))
 
 /*
  * blk_rq_pos()			: the current sector
-- 
2.25.1

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

On 2023-05-04 16:05, Mathieu Desnoyers wrote:
> Fix the following macro parameter usage patterns in blk-mq.h for
> consistency, ensuring that operator precedence is respected:
> 
> Added parentheses:
[...]
> - "x = y" is changed for "x = (y)", because "y" can be an expression
>    containing a comma if it is the result of the expansion of a macro such
>    as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>    precedence. This use-case is far-fetched, but we have to choose one
>    way or the other (with or without parentheses) for consistency.

[...]

>   include/linux/blk-mq.h | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 06caacd77ed6..4de6ad92530c 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -223,13 +223,13 @@ static inline unsigned short req_get_ioprio(struct request *req)
>   
>   #define rq_list_add(listptr, rq)	do {		\
>   	(rq)->rq_next = *(listptr);			\
> -	*(listptr) = rq;				\
> +	*(listptr) = (rq);				\
>   } while (0)
> 

Linus,

Which way do we want to go with respect to the rvalue of the assignment 
operator "=" in a macro ? (with or without parentheses)

In short:

#define m(x) do { z = (x); } while (0)

or

#define m(x) do { z = x; } while (0)

?

Given that "=" has the lowest operator precedence just above comma, and 
its associativity is right-to-left, I suspect the only use that would 
break it without the extra parentheses around "x" is:

#define eval(...) __VA_ARGS__
#define m(x) do { z = x; } while (0)

m(eval(1, abc))

Which generates the following C code after preprocessing:

do { z = 1, abc; } while (0)

which ends up expanding the comma within the rvalue. But this use-case 
is a bit far-fetched, so I don't know if we want to require the 
parentheses or not.

And if we decide that we do want to require the parentheses around the 
"x" parameter in the "=" rvalue, then this means we have to consider 
whether we want to require parentheses around the macro arguments used 
as function/macro arguments, e.g.:

#define eval(...) __VA_ARGS__
#define m(x)    f(x)

m(eval(1, abc));

Which generates the following C code after preprocessing:

f(1, abc);

If we want to be consistent, I suspect we want to require the same for 
both use-cases ("=" rvalue and function/macro parameters).

Thanks,

Mathieu



-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Linus Torvalds]

On Fri, May 5, 2023 at 6:56 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Which way do we want to go with respect to the rvalue of the assignment
> operator "=" in a macro ? (with or without parentheses)
>
> In short:
>
> #define m(x) do { z = (x); } while (0)
>
> or
>
> #define m(x) do { z = x; } while (0)

I suspect that the first one is preferred, just as a "don't even have
to think about it" thing.

In general, despite my suggestion of maybe using upper-case to show
odd syntax (and I may have suggested it, but I really don't like how
it looks, so I'm not at all convinced it's a good idea), to a
first-order approximation the rule should be:

 - always use parentheses around macros

 - EXCEPT:
     - when used purely as arguments to functions or other macros
     - when there is some syntax reason why it's not ok to add parens

The "arguments to functions/macros" is because the comma separator
between arguments isn't even a operator (ie it is *not* a
comma-expression, it's multiple expressions separated by commas).
There is no "operator precedence" subtlety.

So we have a lot of macros that are just wrappers around functions (or
other macros), and in that situation you do *not* then add more
parentheses, and doing something like

    #define update_screen(x) redraw_screen(x, 0)

is fine, and might even be preferred syntax because putting
parentheses around 'x' not only doesn't buy you anything, but just
makes things uglier.

And the "syntax reasons" can be due to the usual things: we not only
have that 'pass member name around' issue, but we have things like
string expansion etc, where adding parentheses anywhere to things like

    #define __stringify_1(x...)     #x
    #define __stringify(x...)       __stringify_1(x)

would obviously simply not work (or look at our "SYSCALL_DEFINEx()"
games for more complex examples with many layers of token pasting
etc).

But in general I would suggest against "this is the lowest priority
operator" kind of games. Nobody remembers the exact operator
precedence so well that they don't have to think about it.

So for example, we have

    #define scr_writew(val, addr) (*(addr) = (val))

to pick another VT example, and I think that's right both for 'addr'
(that requires the parentheses) and for 'val' (that might not require
it, but let's not make people think about it).

                  Linus

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

On 2023-05-05 14:40, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 6:56 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Which way do we want to go with respect to the rvalue of the assignment
>> operator "=" in a macro ? (with or without parentheses)
>>
>> In short:
>>
>> #define m(x) do { z = (x); } while (0)
>>
>> or
>>
>> #define m(x) do { z = x; } while (0)
> 
> I suspect that the first one is preferred, just as a "don't even have
> to think about it" thing.
> 
> In general, despite my suggestion of maybe using upper-case to show
> odd syntax (and I may have suggested it, but I really don't like how
> it looks, so I'm not at all convinced it's a good idea), to a
> first-order approximation the rule should be:
> 
>   - always use parentheses around macros
> 
>   - EXCEPT:
>       - when used purely as arguments to functions or other macros
>       - when there is some syntax reason why it's not ok to add parens

I would add to this list of exceptions cases where the argument is used 
as an expression within brackets, e.g.

#define m(x) myvar[x]

Because the content within the brackets is already an expression.

The other exception I would add is when a parameter is used as an 
lvalue, as:

#define m(x) do { x = 2; } while (0)

because I cannot find a case where it would cause unexpected operator 
precedence.

Are you OK with those 2 additional exceptions ?

> 
> The "arguments to functions/macros" is because the comma separator
> between arguments isn't even a operator (ie it is *not* a
> comma-expression, it's multiple expressions separated by commas).
> There is no "operator precedence" subtlety.

Good point.

> 
> So we have a lot of macros that are just wrappers around functions (or
> other macros), and in that situation you do *not* then add more
> parentheses, and doing something like
> 
>      #define update_screen(x) redraw_screen(x, 0)
> 
> is fine, and might even be preferred syntax because putting
> parentheses around 'x' not only doesn't buy you anything, but just
> makes things uglier.
> 
> And the "syntax reasons" can be due to the usual things: we not only
> have that 'pass member name around' issue, but we have things like
> string expansion etc, where adding parentheses anywhere to things like
> 
>      #define __stringify_1(x...)     #x
>      #define __stringify(x...)       __stringify_1(x)
> 
> would obviously simply not work (or look at our "SYSCALL_DEFINEx()"
> games for more complex examples with many layers of token pasting
> etc).
> 
> But in general I would suggest against "this is the lowest priority
> operator" kind of games. Nobody remembers the exact operator
> precedence so well that they don't have to think about it.
> 
> So for example, we have
> 
>      #define scr_writew(val, addr) (*(addr) = (val))
> 
> to pick another VT example, and I think that's right both for 'addr'
> (that requires the parentheses) and for 'val' (that might not require
> it, but let's not make people think about it).

Indeed, brain power and reviewer time is a scarce resource. It's a shame 
to waste it on figuring out operator priority again and again.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Linus Torvalds]

On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> I would add to this list of exceptions cases where the argument is used
> as an expression within brackets, e.g.
>
> #define m(x) myvar[x]

Yeah, that makes sense not because of any operator precedence rules,
but simply because brackets end up syntactically nesting just like
parentheses themselves do.

IOW, while you can mess up that nesting by having non-nested brackets
in the argument, that's equally true of any added parentheses too.

> The other exception I would add is when a parameter is used as an
> lvalue, as:
>
> #define m(x) do { x = 2; } while (0)

I really don't understand why you think '=' is so special. It's very
much not special.

It happens to have the lowest precedence, sure, but the keyword is "happens".

I think you are confused by the non-C languages that make assignment
be not an expression operator, but a statement.

So I think you are technically correct in that the parentheses aren't
_needed_, but the above is still the same case that in many other
situations parentheses aren't technically *needed*, but not having to
think about it is better than having to do so.

           Linus

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

On 2023-05-05 15:54, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:

[...]

>> The other exception I would add is when a parameter is used as an
>> lvalue, as:
>>
>> #define m(x) do { x = 2; } while (0)
> 
> I really don't understand why you think '=' is so special. It's very
> much not special.
> 
> It happens to have the lowest precedence, sure, but the keyword is "happens".
> 
> I think you are confused by the non-C languages that make assignment
> be not an expression operator, but a statement.

The reason why I think the lvalue of a "=" operator can be argued to be 
"special" is because it is simply invalid to apply many of the C 
operators to an lvalue (e.g. +, -, /, ...), which leads me to think that 
there are no valid lvalue parameters which can cause unexpected operator 
precedence.

That being said, just having to *think* about it is wasted brain power, 
so I am in favor of just adding the parentheses for lvalues as well.

> So I think you are technically correct in that the parentheses aren't
> _needed_, but the above is still the same case that in many other
> situations parentheses aren't technically *needed*, but not having to
> think about it is better than having to do so.

Yes, so no exception for the lvalue of an assignment, therefore giving:

#define m(x) do { (x) = 2; } while (0)

If we are OK with this, I will go ahead and update my patch set accordingly.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Linus Torvalds]

On Fri, May 5, 2023 at 1:08 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> The reason why I think the lvalue of a "=" operator can be argued to be
> "special" is because it is simply invalid to apply many of the C
> operators to an lvalue (e.g. +, -, /, ...),

Mathieu, you are simply objectively wrong.

See here:

  #define m1(x) (x = 2)
  #define m2(x) ((x) = 2)

and then try using the argument "a = b" to those macros.

Guess which one flags it as an error ("lvalue required") and which one does not?

m2 is the only "good" one. Yes, m1 works in 99% of all cases in
practice, but if you want a safer macro, you *will* add the
parentheses.

So *STOP*ARGUING* based on an incorrect "lowest precedence" basis.
Even for the "lowest precedence" case, you have the *same* precedence.

The fact is, assignment is not in any way special operation in macros,
and does not deserve - and should absolutely not have - any special
"doesn't need parentheses around argument" rules.

           Linus

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

On 2023-05-05 16:22, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 1:08 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> The reason why I think the lvalue of a "=" operator can be argued to be
>> "special" is because it is simply invalid to apply many of the C
>> operators to an lvalue (e.g. +, -, /, ...),
> 
> Mathieu, you are simply objectively wrong.
> 
> See here:
> 
>    #define m1(x) (x = 2)
>    #define m2(x) ((x) = 2)
> 
> and then try using the argument "a = b" to those macros.
> 
> Guess which one flags it as an error ("lvalue required") and which one does not?

I'm glad you are proving me wrong. So it was just a lack of imagination 
on my end.

> 
> m2 is the only "good" one. Yes, m1 works in 99% of all cases in
> practice, but if you want a safer macro, you *will* add the
> parentheses.
> 
> So *STOP*ARGUING* based on an incorrect "lowest precedence" basis.
> Even for the "lowest precedence" case, you have the *same* precedence.

Yes, your example clearly shows it.

> The fact is, assignment is not in any way special operation in macros,
> and does not deserve - and should absolutely not have - any special
> "doesn't need parentheses around argument" rules.

Good point. You are right. So that strongly supports the parentheses 
around use of parameters as lvalues. One less special-case to care 
about, which is great.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

I've attempted to capture the resulting rules based on our discussion to add this to
coding-style.rst. Please let me know if anything is wrong:

(to be added in section 12) Macros, Enums and RTL)

Always use parentheses around macro arguments, except when:

- they are used as a full expression:
   - as an initializer,
   - as an expression statement,
   - as the controlling expression of a selection statement (``if`` or ``switch``),
   - as the controlling expression of a ``while`` or ``do`` statement,
   - as any of the expressions of a ``for`` statement,
   - as the expression in a return statement,
- they are used as expression within an array subscript operator "[]",
- they are used as arguments to functions or other macros,
- there is some syntax reason why adding the parentheses would not work.

(note: I'm unsure about requiring or not the parentheses around initializers.
Based on C11 section "6.8 Statement and blocks", initializers that are not part of
a compound literal are full expressions, which makes the extra parentheses useless.)

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

RE: [RFC PATCH 12/13] blk-mq.h: Fix parentheses around macro parameter use

[David Laight]

From: Linus Torvalds
> Sent: 05 May 2023 20:55
....
> > The other exception I would add is when a parameter is used as an
> > lvalue, as:
> >
> > #define m(x) do { x = 2; } while (0)
> 
> I really don't understand why you think '=' is so special. It's very
> much not special.
> 
> It happens to have the lowest precedence, sure, but the keyword is "happens".

And consider what happens if you try:
	m(a ? b : c)

Personally I'd avoid using parameters as lvalues if at all possible.
It is much better to have:
	#define m(x) do { *(x) = 2; } while (0)
and require the caller do m(&foo) to make it obvious the value is changed.
(Apart from loop definitions...)

Things like the C++ 'int &arg' make it hard to scan read/search
code for places where variables get changed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[RFC PATCH 13/13] bio.h: Fix parentheses around macro parameter use

[Mathieu Desnoyers]

Add missing parentheses around macro parameter use in the following
patterns to ensure operator precedence behaves as expected:

- "x++" is changed for "(x)++",
- x->member is changed for (x)->member,
- &x is changed for &(x).
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.

Remove useless parentheses in the following macro argument usage
patterns:

- m((x)) is changed for m(x),
- m((x), y) is changed for m(x, y) because comma has lowest operator
  precedence, making the extra parentheses useless,

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Omar Sandoval <osandov@fb.com>
Cc: linux-block@vger.kernel.org
---
 include/linux/bio.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index b3e7529ff55e..f09aea290511 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -20,7 +20,7 @@ static inline unsigned int bio_max_segs(unsigned int nr_segs)
 }
 
 #define bio_prio(bio)			(bio)->bi_ioprio
-#define bio_set_prio(bio, prio)		((bio)->bi_ioprio = prio)
+#define bio_set_prio(bio, prio)		((bio)->bi_ioprio = (prio))
 
 #define bio_iter_iovec(bio, iter)				\
 	bvec_iter_bvec((bio)->bi_io_vec, (iter))
@@ -37,7 +37,7 @@ static inline unsigned int bio_max_segs(unsigned int nr_segs)
 #define bio_iovec(bio)		bio_iter_iovec((bio), (bio)->bi_iter)
 
 #define bvec_iter_sectors(iter)	((iter).bi_size >> 9)
-#define bvec_iter_end_sector(iter) ((iter).bi_sector + bvec_iter_sectors((iter)))
+#define bvec_iter_end_sector(iter) ((iter).bi_sector + bvec_iter_sectors(iter))
 
 #define bio_sectors(bio)	bvec_iter_sectors((bio)->bi_iter)
 #define bio_end_sector(bio)	bvec_iter_end_sector((bio)->bi_iter)
@@ -93,7 +93,7 @@ static inline bool bio_next_segment(const struct bio *bio,
  * before it got to the driver and the driver won't own all of it
  */
 #define bio_for_each_segment_all(bvl, bio, iter) \
-	for (bvl = bvec_init_iter_all(&iter); bio_next_segment((bio), &iter); )
+	for (bvl = bvec_init_iter_all(&(iter)); bio_next_segment(bio, &(iter)); )
 
 static inline void bio_advance_iter(const struct bio *bio,
 				    struct bvec_iter *iter, unsigned int bytes)
@@ -145,17 +145,17 @@ static inline void bio_advance(struct bio *bio, unsigned int nbytes)
 #define __bio_for_each_segment(bvl, bio, iter, start)			\
 	for (iter = (start);						\
 	     (iter).bi_size &&						\
-		((bvl = bio_iter_iovec((bio), (iter))), 1);		\
-	     bio_advance_iter_single((bio), &(iter), (bvl).bv_len))
+		((bvl = bio_iter_iovec(bio, iter)), 1);			\
+	     bio_advance_iter_single(bio, &(iter), (bvl).bv_len))
 
 #define bio_for_each_segment(bvl, bio, iter)				\
 	__bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter)
 
-#define __bio_for_each_bvec(bvl, bio, iter, start)		\
+#define __bio_for_each_bvec(bvl, bio, iter, start)			\
 	for (iter = (start);						\
 	     (iter).bi_size &&						\
-		((bvl = mp_bvec_iter_bvec((bio)->bi_io_vec, (iter))), 1); \
-	     bio_advance_iter_single((bio), &(iter), (bvl).bv_len))
+		((bvl = mp_bvec_iter_bvec((bio)->bi_io_vec, iter)), 1);	\
+	     bio_advance_iter_single(bio, &(iter), (bvl).bv_len))
 
 /* iterate over multi-page bvec */
 #define bio_for_each_bvec(bvl, bio, iter)			\
@@ -167,7 +167,7 @@ static inline void bio_advance(struct bio *bio, unsigned int nbytes)
  */
 #define bio_for_each_bvec_all(bvl, bio, i)		\
 	for (i = 0, bvl = bio_first_bvec_all(bio);	\
-	     i < (bio)->bi_vcnt; i++, bvl++)
+	     i < (bio)->bi_vcnt; (i)++, (bvl)++)
 
 #define bio_iter_last(bvec, iter) ((iter).bi_size == (bvec).bv_len)
 
@@ -548,7 +548,7 @@ static inline void bio_list_init(struct bio_list *bl)
 #define BIO_EMPTY_LIST	{ NULL, NULL }
 
 #define bio_list_for_each(bio, bl) \
-	for (bio = (bl)->head; bio; bio = bio->bi_next)
+	for (bio = (bl)->head; bio; bio = (bio)->bi_next)
 
 static inline unsigned bio_list_size(const struct bio_list *bl)
 {
@@ -702,7 +702,7 @@ static inline bool bioset_initialized(struct bio_set *bs)
 
 #define bio_for_each_integrity_vec(_bvl, _bio, _iter)			\
 	for_each_bio(_bio)						\
-		bip_for_each_vec(_bvl, _bio->bi_integrity, _iter)
+		bip_for_each_vec(_bvl, (_bio)->bi_integrity, _iter)
 
 extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
 extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
-- 
2.25.1