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.
Signed-off-by: Aiden Leong <[email protected]>
---
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 &&
--
2.39.0
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 <[email protected]>
> ---
> 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 &&
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 <[email protected]>
>> ---
>> 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 &&
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 <[email protected]>
> > > ---
> > > 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.
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 <[email protected]>
>>>> ---
>>>> 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