Re: [PATCH v3] wifi: iwlwifi: pcie: fix the order of scanning iwl_dev_info_table

[Aiden Leong <aiden.leong@aibsd.com>]

[-- Attachment #1: Type: text/plain, Size: 4723 bytes --]

On Sunday, March 12, 2023 5:47:07 PM CST Greenman, Gregory wrote:
> On Fri, 2023-03-10 at 13:14 +0800, Aiden Leong wrote:
> 
> > On Wednesday, February 8, 2023 5:14:50 AM CST Aiden Leong wrote:
> > 
> > > On Wednesday, February 8, 2023 1:44:39 AM CST Greenman, Gregory wrote:
> > > 
> > > > On Fri, 2023-01-20 at 01:56 +0800, Aiden Leong wrote:
> > > > 
> > > > > 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.
> > > > > 
> > > > > Signed-off-by: Aiden Leong <aiden.leong@aibsd.com>
> > > > > ---
> > > > > 
> > > > >  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
> > > 
> > > 
> > > > > 99768d6a6032..05764eef15a7 100644
> > > > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > > > @@ -1456,7 +1456,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 &&
> > > > 
> > > > 
> > > > It failed or internal testing, so it's more complicated. To traverse
> > > > this
> > > > table
> > > 
> > > 
> > >  from the beginning to the end requires some changes to the table
> > > 
> > > 
> > > > itself and the "goto" wasn't omitted by a mistake, but for a
> > > > reason...
> > > > For the device that you have (device id 0x4DF0, sub-device id 0x0244,
> > > > right?)
> > > 
> > > 
> > >  is it enough to have the first fix (disable
> > > 
> > > 
> > > > tx_with_siso_diversity)?
> > > 
> > > 
> > > Hi Gregory,
> > > That's exactly why I put a warning in previous emails.
> > > My opinion will be a little different than yours in this situation.
> > > 1. We SHOULD traverse this table from top to bottom to keep our source
> > > tree
 as clean as possible.
> > > 2. One simple option is to reverse every config items in this table so
> > > the
> > > logic keep the same.
> > > 3. Your team(I assume Luca Coelho is your colleague) may need to
> > > provide
> > > further explaination about the `goto` line, since each change in kernel
> > > should have a reason.
> > > 4. 0x4DF0, 0x0244 is correct. The question is: Will Intel release
> > > products
> > > with same pid+subID but differenct STEP/RF_TYPE/RF_ID etc? If so,
> > > pid+subID
 won't be enough.
> > > 
> > > To sum up, there will be three patches:
> > > 1. This patch still fixes the BUG introduced by the `goto` change.
> > > 2. Patch 2 should be [PATCH 1/2] in previous email.
> > > 3. Patch 3 reverses every items in this table. Your team can fine-tune
> > > the
> > > order of each items. I won't submit this patch.
> > > 
> > > If you like my ideas, please merge patch1&2 along with another ident
> > > fix
> > > patch.
> > > 
> > > BTW, it has been a month since the first email. I'd appreciate if you
> > > reply
 soon.
> > > 
> > > Cheers,
> > > Aiden
> > 
> > 
> > Hi Gregory,
> > 
> > PING
> > 
> > You should let us know if you are not actively maintaining the community
> > part 
 of the driver. If you are only working on the close source
> > firmware, we should have someone else do the open source job.
> > We should not waste our life for months on such a small patch.
> > 
> > Have a nice day,
> > Aiden
> 
> 
> Hi,
> 
> You’re coming across as rather accusatory and demanding. I’d appreciate if
> you could 
 tone it down a bit. Regarding the table order, we’ve made a
> decision in the code way back to walk the table from the back – that may
> not match your personal expectation of “clean”, but that’s really your
> problem, not ours.
> Also, we cannot comment on future product releases in general.
> 
> If you’re willing to work with us to fix the issue you’re encountering
> within the
 framework of how the driver is written now, I can give you a
> patch with more logs to understand why your second patch doesn't fix the
> issue.
> 
> Regards,
> Gregory

So I'm the bad guy now?
Fine.
That's funny!

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]