Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <becf35b8-df98-7e19-bf26-1a288cd0ddd0@aibsd.com>
Date:   Thu, 5 Jan 2023 15:02:42 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 2/2] iwlwifi: pcie: add support for AX101NGW
Content-Language: en-US
To:     "Greenman, Gregory" <gregory.greenman@intel.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     "kvalo@kernel.org" <kvalo@kernel.org>,
        "edumazet@google.com" <edumazet@google.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>
References: <20230102024038.2915-1-aiden.leong@aibsd.com>
 <00a7a47051ead3032f383868248d89e56d3617f4.camel@intel.com>
 <708fad8b-0419-ae7f-8586-7da58b50d925@aibsd.com>
 <ae38f167ef5f16cac767db4b7a20984731e8b874.camel@intel.com>
From:   Aiden Leong <aiden.leong@aibsd.com>
In-Reply-To: <ae38f167ef5f16cac767db4b7a20984731e8b874.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2023/1/5 14:21, Greenman, Gregory wrote:
> On Mon, 2023-01-02 at 22:00 +0800, Aiden Leong wrote:
>> 1. Please have a look at the second commit:
>> https://lore.kernel.org/linux-wireless/iwlwifi.20200323151304.ec4f463bde60.I14e9146a99621ff11ce50bc746a4b88af508fee0@changeid/
>>
>> Find the `goto found;` statement.
>>
>> The logic was to find the FIRST match then break with `goto found`. The
>> refactor code removed the `goto` statement which is incorrect.
>>
>>
>> 2. Let's go back to the first commit:
>> https://lore.kernel.org/linux-wireless/iwlwifi.20211024165252.abd85e1391cb.I7681fe90735044cc1c59f120e8591b7ac125535d@changeid/
>>
>>   > We don't want to change the semantics ("most generic entry must come
>> first")
>>
>> That `semantics` was mislead by the previous commit.
>>
>>
>> On 2023/1/2 21:35, Greenman, Gregory wrote:
>>> On Mon, 2023-01-02 at 10:40 +0800, Aiden Leong wrote:
>>>> Revert:
>>>> commit 3f7320428fa4 ("iwlwifi: pcie: simplify iwl_pci_find_dev_info()")
>>>>
>>>> A bug was introduced by:
>>>> commit 32ed101aa140 ("iwlwifi: convert all Qu with Jf devices to the new
>>>> config table"),
>>>> where a goto statement was removed.
>>> Not sure I undestand what problem reversing the "for" loop solves.
>>>
>>>> Signed-off-by: Aiden Leong <aiden.leong@aibsd.com>
>>>> ---
>>>> Notice:
>>>> Please run further tests before merging. I'm NOT familiar with device
>>>> drivers.
>>>> ---
>>>>    drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
>>>> index a46df1320372..5d74adbd49cf 100644
>>>> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
>>>> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
>>>> @@ -1461,7 +1461,7 @@ iwl_pci_find_dev_info(u16 device, u16 subsystem_device,
>>>>           if (!num_devices)
>>>>                   return NULL;
>>>>    
>>>> -       for (i = num_devices - 1; i >= 0; i--) {
>>>> +       for (i = 0; i < num_devices; i++) {
>>>>                   const struct iwl_dev_info *dev_info = &iwl_dev_info_table[i];
>>>>    
>>>>                   if (dev_info->device != (u16)IWL_CFG_ANY &&
> So, the fix itself is correct (thanks for fixing it!).
> The commit message should be a bit changed, the "goto" removal is not relevant
> in the current code since now this for loop is in a separate function. Also, all
> wifi commits should have prefix "wifi: " to separate them from the rest of netdev.
> Tell me if you want to resend this commit or I can fix it by myself.
>
I'm a kernel newbie. Thank you for your patience.

I'd like to resend the commit so I can learn more about how to do it right.

The title of the patchset will be "wifi: iwlwifi: pcie: add support for 
AX101NGW",

and the commit message will be "Fix a bug introduced by: commit 
32ed101aa140 ("iwlwifi: convert all Qu with Jf devices to the new config 
table"), so now we pick the FIRST matching config."

Would it be all right?


Have a nice day!

Aiden Leong