Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8c28baa8-0945-fd77-3d1d-92c99c7bbbd1@efficios.com>
Date:   Thu, 4 May 2023 10:54:09 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [RFC PATCH 4/4] llist.h: Fix parentheses around macro pointer
 parameter use
Content-Language: en-US
To:     "Huang, Ying" <ying.huang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20230504012914.1797355-1-mathieu.desnoyers@efficios.com>
 <20230504012914.1797355-4-mathieu.desnoyers@efficios.com>
 <87pm7gd4l5.fsf@yhuang6-desk2.ccr.corp.intel.com>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <87pm7gd4l5.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023-05-04 01:54, Huang, Ying wrote:
> Hi, Mathieu,
> 
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>>
>> 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.
> 
> I don't think it's necessary to add parentheses here.  As you said,
> "pos" is used as an lvalue.

I agree that it's not strictly necessary to add the parentheses around
"pos" in typeof(*pos) when pos is also used as an lvalue within a macro,
but if we look at what happened in list.h, we can see why having a consistent
pattern is good to eliminate issues as the code evolves.

When code from list_for_each_entry_continue was lifted into
list_prepare_entry(), we had a situation where "pos" was initially used
as an lvalue in the original macro, but not in list_prepare_entry(), for
which the parentheses were relevant.

This example is from the pre-git era, in tglx's history tree:

commit a3500b9e955d47891e57587c30006de83a3591f5
Author: Linus Torvalds <torvalds@home.osdl.org>
Date:   Wed Feb 11 21:00:34 2004 -0800

     Fix "bus_for_each_dev()" and "bus_for_each_drv()", which did not
     correctly handle the "restart from this device/driver" case, and
     caused oopses with ieee1394.
     
     This just uses "list_for_each_entry_continue()" instead.
     
     Add helper macro to make usage of "list_for_each_entry_continue()"
     a bit more readable.

[...]

+/**
+ * list_prepare_entry - prepare a pos entry for use as a start point in
+ *                     list_for_each_entry_continue
+ * @pos:       the type * to use as a start point
+ * @head:      the head of the list
+ * @member:    the name of the list_struct within the struct.
+ */
+#define list_prepare_entry(pos, head, member) \
+       ((pos) ? : list_entry(head, typeof(*pos), member))

So even though the fact that "pos" is used as an lvalue specifically in
llist_for_each_entry_safe() makes it so that the parentheses are not
strictly required around "pos" in typeof(*pos), I argue that we should
still add those for consistency.

> 
>> Remove useless parentheses around use of macro parameter (node) in the
>> following pattern:
>>
>>    llist_entry((node), typeof(*pos), member)
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
> 
> This change looks good for me.

Thanks,

Mathieu

> 
> Best Regards,
> Huang, Ying
> 
>> 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 | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>> index 85bda2d02d65..45d358c15d0d 100644
>> --- a/include/linux/llist.h
>> +++ b/include/linux/llist.h
>> @@ -173,9 +173,9 @@ 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); \
>> +		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
>>   	     pos = n)
>>   
>>   /**

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