Received-SPF: pass (google.com: domain of linux-wireless+bounces-4697-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240312135557.1778379-1-quic_ramess@quicinc.com>
 <20240312135557.1778379-2-quic_ramess@quicinc.com> <4a527729-2bf8-47ce-8275-d36b76bde95c@quicinc.com>
 <3178cf8c-d328-4fe2-b3be-f5c4c908f20f@quicinc.com> <605a673e-3686-47d0-b71b-1e314fde65db@quicinc.com>
 <87il1qf571.fsf@kernel.org> <87edcef4qi.fsf@kernel.org> <f768cfec-d20d-424d-85ef-3576434aca12@quicinc.com>
In-Reply-To: <f768cfec-d20d-424d-85ef-3576434aca12@quicinc.com>
From: Jonas Gorski <jonas.gorski@gmail.com>
Date: Wed, 13 Mar 2024 20:56:48 +0100
Message-ID: <CAOiHx=kx03MKKyMcJPxYPZH=E7krZwfsDqq2xnow8qRB7D-aqQ@mail.gmail.com>
Subject: Re: [PATCH v4 01/12] wifi: ath12k: add multiple radio support in a
 single MAC HW un/register
To: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: Kalle Valo <kvalo@kernel.org>, Rameshkumar Sundaram <quic_ramess@quicinc.com>, 
	ath12k@lists.infradead.org, linux-wireless@vger.kernel.org, 
	Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Content-Type: text/plain; charset="UTF-8"

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

Best Regards,
Jonas