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

Rameshkumar Sundaram
2024-03-19T15:52:18Z

On 3/19/2024 12:06 AM, Kalle Valo wrote:
> Rameshkumar Sundaram <quic_ramess@quicinc.com> writes:
>
>> 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.
>
> Yeah, typeof() won't help here as we can't create a local variable. Or
> at least I can't come up way to do that safely, ideas very welcome.
>
> I think it's just best to ignore the checkpatch warning for now, unless
> better proposals come up. ath12k-check has functionality to ignore
> specific warnings (see checkpatch_filter array), I can add this warning
> to the array.
>
Sure, Thanks Kalle, I'll send v5 with Jeff's comments addressed.