| Message ID | 20230102024038.2915-1-aiden.leong@aibsd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Miri Korenblit |
| Headers | show |
| Series | [1/2] iwlwifi: pcie: add support for AX101NGW | expand |
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 &&
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 &&
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.
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
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 &&
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 <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(-)