Re: [PATCH v4 01/12] wifi: ath12k: add multiple radio support in a single MAC HW un/register

From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
To: Jonas Gorski <jonas.gorski@gmail.com>,
	Jeff Johnson <quic_jjohnson@quicinc.com>,
	Kalle Valo <kvalo@kernel.org>
Cc: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
	Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Subject: Re: [PATCH v4 01/12] wifi: ath12k: add multiple radio support in a single MAC HW un/register
Date: Fri, 15 Mar 2024 01:08:56 +0530	[thread overview]
Message-ID: <f542dbeb-debf-4335-9d49-4d12510f8bb4@quicinc.com> (raw)
In-Reply-To: <CAOiHx=kx03MKKyMcJPxYPZH=E7krZwfsDqq2xnow8qRB7D-aqQ@mail.gmail.com>

On 3/14/2024 1:26 AM, Jonas Gorski wrote:
> On Wed, 13 Mar 2024 at 20:18, Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>>
>> On 3/13/2024 9:58 AM, Kalle Valo wrote:
>>> Kalle Valo <kvalo@kernel.org> writes:
>>>
>>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>>>
>>>>> On 3/13/2024 5:57 AM, Rameshkumar Sundaram wrote:
>>>>>> On 3/13/2024 3:23 AM, Jeff Johnson wrote:
>>>>>>> and guess we have to figure out how to suppress the ath12k-check issues with
>>>>>>> this macro
>>>>>> ath12k-check complains about the reuse of ah and index arguments which
>>>>>> may get evaluated multiple times if its an arithmetic expression, But
>>>>>> areas where we use the macro in our code aren't doing so.
>>>>>> Do you have any suggestions here ? or shall we go back and use this
>>>>>> for-loop inline.
>>>>>
>>>>> The macro makes sense -- we'll need to update the overrides in ath12k-check.
>>>>
>>>> IIRC it is possible to avoid variable reuse in macros with typeof()
>>>> operator (or something like that). I can't remember the details right
>>>> now but I think there are examples in the kernel code.
>>>
>>> Here's the GCC documentation with an example:
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc/Typeof.html
>>>
Thanks Kalle for the references, as Jeff mentioned below, we need to 
reuse the arguments since we write to ar and index arguments on each 
iteration.

Defining local vars using typeof() without limiting their scope (since 
we are defining a for_each iterator{}) leads other issues like 
redefinition of variables in functions where we use this macro more than 
once :(

Also even if we somehow manage to convince check-patch, we'll still end 
up evaluating index and ar arguments in every iteration of loop.
This just gives an impression to check-patch that the macro is unsafe 
(although logically its not).
Experts, what is the standard we should follow here. Please suggest.

>>
>> the problem here is that the macro actually writes to those arguments multiple
>> times, so we actually need to reuse the arguments
>>
>> the macro as defined exactly matches the semantics of other for_each macros in
>> the kernel, i.e. see in include/linux/list.h:
>> #define list_for_each(pos, head) \
>>          for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
>>
>> what I don't understand is why list_for_each() doesn't trigger the same
>> checkpatch issues. even stranger is that if I copy that macro into ath12k then
>> I do see the same checkpatch issues:
>> CHECK: Macro argument reuse 'pos' - possible side-effects?
>> #998: FILE: drivers/net/wireless/ath/ath12k/core.h:998:
>> +#define list_for_each(pos, head) \
>> +       for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
>>
>> CHECK: Macro argument reuse 'head' - possible side-effects?
>> #998: FILE: drivers/net/wireless/ath/ath12k/core.h:998:
>> +#define list_for_each(pos, head) \
>> +       for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
>>
>> So I'm really confused since I don't see anything in checkpatch.pl that would
>> cause the behavior to change between macros in include/linux/list.h vs macros
>> in drivers/net/wireless/ath/ath12k/core.h
> 
> The definition of the macro causes the complaint, not the usage of it.
> If you run checkpatch.pl on include/linux/list.h, you'll get the same
> output:
> 
> $ ./scripts/checkpatch.pl --strict --file include/linux/list.h
> ...
> CHECK: Macro argument reuse 'pos' - possible side-effects?
> #686: FILE: include/linux/list.h:686:
> +#define list_for_each(pos, head) \
> + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
> 
> CHECK: Macro argument reuse 'head' - possible side-effects?
> #686: FILE: include/linux/list.h:686:
> +#define list_for_each(pos, head) \
> + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
> ...
Thanks Jonas and Jeff for your insights!!