[PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Vittorio Gambaletta (VittGam)]

If generic entries are positioned above specific ones, then the
former will be matched first and used instead of the latter.

Cc: <linux-wireless@vger.kernel.org>
Cc: <ath9k-devel@qca.qualcomm.com>
Cc: <ath9k-devel@lists.ath9k.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
---

--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -26,7 +26,6 @@
 	{ PCI_VDEVICE(ATHEROS, 0x0023) }, /* PCI   */
 	{ PCI_VDEVICE(ATHEROS, 0x0024) }, /* PCI-E */
 	{ PCI_VDEVICE(ATHEROS, 0x0027) }, /* PCI   */
-	{ PCI_VDEVICE(ATHEROS, 0x0029) }, /* PCI   */
 
 #ifdef CONFIG_ATH9K_PCOEM
 	/* Mini PCI AR9220 MB92 cards: Compex WLM200NX, Wistron DNMA-92 */
@@ -37,7 +36,7 @@
 	  .driver_data = ATH9K_PCI_LED_ACT_HI },
 #endif
 
-	{ PCI_VDEVICE(ATHEROS, 0x002A) }, /* PCI-E */
+	{ PCI_VDEVICE(ATHEROS, 0x0029) }, /* PCI   */
 
 #ifdef CONFIG_ATH9K_PCOEM
 	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_ATHEROS,
@@ -85,7 +84,11 @@
 			 0x10CF, /* Fujitsu */
 			 0x1536),
 	  .driver_data = ATH9K_PCI_D3_L1_WAR },
+#endif
 
+	{ PCI_VDEVICE(ATHEROS, 0x002A) }, /* PCI-E */
+
+#ifdef CONFIG_ATH9K_PCOEM
 	/* AR9285 card for Asus */
 	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_ATHEROS,
 			 0x002B,

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Kalle Valo]

"Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:

> If generic entries are positioned above specific ones, then the
> former will be matched first and used instead of the latter.
>
> Cc: <linux-wireless@vger.kernel.org>
> Cc: <ath9k-devel@qca.qualcomm.com>
> Cc: <ath9k-devel@lists.ath9k.org>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>

Why? What kind of bug you are fixing? You are not really describing the
problem you are trying fix.

And your email headers look weird:

To:     <kvalo@codeaurora.org>
Cc:     <linux-wireless@vger.kernel.org>
Cc:     <ath9k-devel@qca.qualcomm.com>
Cc:     <ath9k-devel@venema.h4ckr.net>
Cc:     <stable@vger.kernel.org>
Date:   Mon, 3 Oct 2016 12:00:56 +0200

What software are you using to send this? Only one CC header is valid
according to the spec (thanks to Luca for checking) even though mailers
seem to handle multiple CC headers. But for example my patchwork script
fails with this and uses only the first CC header.

-- 
Kalle Valo

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Vittorio Gambaletta (VittGam)]

Hello,

On 04/10/2016 17:46:44 CEST, Kalle Valo wrote:
> "Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:
>
>> If generic entries are positioned above specific ones, then the
>> former will be matched first and used instead of the latter.
>>
>> Cc: <linux-wireless@vger.kernel.org>
>> Cc: <ath9k-devel@qca.qualcomm.com>
>> Cc: <ath9k-devel@lists.ath9k.org>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>
> Why? What kind of bug you are fixing? You are not really describing the
> problem you are trying fix.

The active_high LED of my Wistron DNMA-92 is still being recognized as
active_low on 4.7.6 mainline.

When I was preparing my former patch to fix that, I initially added the
PCI_DEVICE_SUB section for 0x0029/0x2096 above the PCI_VDEVICE section
for 0x0029; but then I moved the former below the latter after seeing
how 0x002A sections were sorted in the file.

I must have somehow messed up with testing, because I tested the final
version of that patch before sending it, and it was apparently working;
but now it is not working on 4.7.6 mainline.

With this patch, 0x0029/0x2096 has finally got active_high LED on 4.7.6.

So, after seeing that the rest of the file is sorted this way (generic
section after the specific ones), I concluded that the 0x002A sorting
was wrong in the first place, and so is 0x0029. Then I sent this patch
to fix this.

> And your email headers look weird:
>
> To:     <kvalo@codeaurora.org>
> Cc:     <linux-wireless@vger.kernel.org>
> Cc:     <ath9k-devel@qca.qualcomm.com>
> Cc:     <ath9k-devel@venema.h4ckr.net>
> Cc:     <stable@vger.kernel.org>
> Date:   Mon, 3 Oct 2016 12:00:56 +0200
>
> What software are you using to send this? Only one CC header is valid
> according to the spec (thanks to Luca for checking) even though mailers
> seem to handle multiple CC headers. But for example my patchwork script
> fails with this and uses only the first CC header.

Sorry about this, I used a custom mailer to send the patch since I was
having problems with git-send-email...

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Kalle Valo]

"Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:

> Hello,
>
> On 04/10/2016 17:46:44 CEST, Kalle Valo wrote:
>> "Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:
>>
>>> If generic entries are positioned above specific ones, then the
>>> former will be matched first and used instead of the latter.
>>>
>>> Cc: <linux-wireless@vger.kernel.org>
>>> Cc: <ath9k-devel@qca.qualcomm.com>
>>> Cc: <ath9k-devel@lists.ath9k.org>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>>
>> Why? What kind of bug you are fixing? You are not really describing the
>> problem you are trying fix.
>
> The active_high LED of my Wistron DNMA-92 is still being recognized as
> active_low on 4.7.6 mainline.

This kind of information is important, always add that to the commit log
so that we don't need to guess. 

> When I was preparing my former patch to fix that, I initially added the
> PCI_DEVICE_SUB section for 0x0029/0x2096 above the PCI_VDEVICE section
> for 0x0029; but then I moved the former below the latter after seeing
> how 0x002A sections were sorted in the file.
>
> I must have somehow messed up with testing, because I tested the final
> version of that patch before sending it, and it was apparently working;
> but now it is not working on 4.7.6 mainline.
>
> With this patch, 0x0029/0x2096 has finally got active_high LED on
> 4.7.6.

I'm confused, are you now saying that this patch doesn't work?

> So, after seeing that the rest of the file is sorted this way (generic
> section after the specific ones), I concluded that the 0x002A sorting
> was wrong in the first place, and so is 0x0029. Then I sent this patch
> to fix this.

I can't see how changing the order in ath_pci_id_table[] would make any
difference in functionality, but I might be missing something.

>> And your email headers look weird:
>>
>> To:     <kvalo@codeaurora.org>
>> Cc:     <linux-wireless@vger.kernel.org>
>> Cc:     <ath9k-devel@qca.qualcomm.com>
>> Cc:     <ath9k-devel@venema.h4ckr.net>
>> Cc:     <stable@vger.kernel.org>
>> Date:   Mon, 3 Oct 2016 12:00:56 +0200
>>
>> What software are you using to send this? Only one CC header is valid
>> according to the spec (thanks to Luca for checking) even though mailers
>> seem to handle multiple CC headers. But for example my patchwork script
>> fails with this and uses only the first CC header.
>
> Sorry about this, I used a custom mailer to send the patch since I was
> having problems with git-send-email...

Ok, that explains it.

-- 
Kalle Valo

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Vittorio Gambaletta (VittGam)]

Hello,

On 12/10/2016 15:34:46 CEST, Kalle Valo wrote:
> "Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:
>
>> Hello,
>>
>> On 04/10/2016 17:46:44 CEST, Kalle Valo wrote:
>>> "Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:
>>>
>>>> If generic entries are positioned above specific ones, then the
>>>> former will be matched first and used instead of the latter.
>>>>
>>>> Cc: <linux-wireless@vger.kernel.org>
>>>> Cc: <ath9k-devel@qca.qualcomm.com>
>>>> Cc: <ath9k-devel@lists.ath9k.org>
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>>>
>>> Why? What kind of bug you are fixing? You are not really describing the
>>> problem you are trying fix.
>>
>> The active_high LED of my Wistron DNMA-92 is still being recognized as
>> active_low on 4.7.6 mainline.
>
> This kind of information is important, always add that to the commit log
> so that we don't need to guess.

Okay, sorry about missing that before.

>> When I was preparing my former patch to fix that, I initially added the
>> PCI_DEVICE_SUB section for 0x0029/0x2096 above the PCI_VDEVICE section
>> for 0x0029; but then I moved the former below the latter after seeing
>> how 0x002A sections were sorted in the file.
>>
>> I must have somehow messed up with testing, because I tested the final
>> version of that patch before sending it, and it was apparently working;
>> but now it is not working on 4.7.6 mainline.
>>
>> With this patch, 0x0029/0x2096 has finally got active_high LED on
>> 4.7.6.
>
> I'm confused, are you now saying that this patch doesn't work?

No: the previous patch (which is already merged and added the LED driver_data
for my card) doesn't work. This one does work.

>> So, after seeing that the rest of the file is sorted this way (generic
>> section after the specific ones), I concluded that the 0x002A sorting
>> was wrong in the first place, and so is 0x0029. Then I sent this patch
>> to fix this.
>
> I can't see how changing the order in ath_pci_id_table[] would make any
> difference in functionality, but I might be missing something.

It does: I've looked through the relevant code, and found that PCI device
matching from that table is done sequentially in pci_match_id() from
drivers/pci/pci-driver.c.

So if a generic PCI_VDEVICE entry (that has both subvendor and subdevice IDs
set to PCI_ANY_ID) is put before a more specific one (PCI_DEVICE_SUB), then
the generic PCI_VDEVICE entry will match first and will be used.

>>> And your email headers look weird:
>>>
>>> To:     <kvalo@codeaurora.org>
>>> Cc:     <linux-wireless@vger.kernel.org>
>>> Cc:     <ath9k-devel@qca.qualcomm.com>
>>> Cc:     <ath9k-devel@venema.h4ckr.net>
>>> Cc:     <stable@vger.kernel.org>
>>> Date:   Mon, 3 Oct 2016 12:00:56 +0200
>>>
>>> What software are you using to send this? Only one CC header is valid
>>> according to the spec (thanks to Luca for checking) even though mailers
>>> seem to handle multiple CC headers. But for example my patchwork script
>>> fails with this and uses only the first CC header.
>>
>> Sorry about this, I used a custom mailer to send the patch since I was
>> having problems with git-send-email...
>
> Ok, that explains it.

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Valo, Kalle]

"Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:

>>> So, after seeing that the rest of the file is sorted this way (generic
>>> section after the specific ones), I concluded that the 0x002A sorting
>>> was wrong in the first place, and so is 0x0029. Then I sent this patch
>>> to fix this.
>>
>> I can't see how changing the order in ath_pci_id_table[] would make any
>> difference in functionality, but I might be missing something.
>
> It does: I've looked through the relevant code, and found that PCI device
> matching from that table is done sequentially in pci_match_id() from
> drivers/pci/pci-driver.c.
>
> So if a generic PCI_VDEVICE entry (that has both subvendor and subdevice IDs
> set to PCI_ANY_ID) is put before a more specific one (PCI_DEVICE_SUB), then
> the generic PCI_VDEVICE entry will match first and will be used.

Ah, now it makes sense. Thanks for patiently explaining this to me :)

So to tell the full story I'll change the commit log to something like
below. Does it look ok to you?

----------------------------------------------------------------------
ath9k: Really fix LED polarity for some Mini PCI AR9220 MB92 cards.

The active_high LED of my Wistron DNMA-92 is still being recognized as
active_low on 4.7.6 mainline. When I was preparing my former commit
0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92
cards.") to fix that I must have somehow messed up with testing, because
I tested the final version of that patch before sending it, and it was
apparently working; but now it is not working on 4.7.6 mainline.

I initially added the PCI_DEVICE_SUB section for 0x0029/0x2096 above the
PCI_VDEVICE section for 0x0029; but then I moved the former below the
latter after seeing how 0x002A sections were sorted in the file.

So if a generic PCI_VDEVICE entry (that has both subvendor and subdevice
IDs set to PCI_ANY_ID) is put before a more specific one
(PCI_DEVICE_SUB), then the generic PCI_VDEVICE entry will match first
and will be used.

With this patch, 0x0029/0x2096 has finally got active_high LED on 4.7.6.

Fixes: 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92 cards.")
Cc: <stable@vger.kernel.org> #4.7+
Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
[kvalo@qca.qualcomm.com: improve the commit log based on email discussions]
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
----------------------------------------------------------------------

-- 
Kalle Valo

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Vittorio Gambaletta (VittGam)]

Hello,

On 12/10/2016 17:01:08 CEST, Valo, Kalle wrote:
> "Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:
>
>>>> So, after seeing that the rest of the file is sorted this way (generic
>>>> section after the specific ones), I concluded that the 0x002A sorting
>>>> was wrong in the first place, and so is 0x0029. Then I sent this patch
>>>> to fix this.
>>>
>>> I can't see how changing the order in ath_pci_id_table[] would make any
>>> difference in functionality, but I might be missing something.
>>
>> It does: I've looked through the relevant code, and found that PCI device
>> matching from that table is done sequentially in pci_match_id() from
>> drivers/pci/pci-driver.c.
>>
>> So if a generic PCI_VDEVICE entry (that has both subvendor and subdevice IDs
>> set to PCI_ANY_ID) is put before a more specific one (PCI_DEVICE_SUB), then
>> the generic PCI_VDEVICE entry will match first and will be used.
>
> Ah, now it makes sense. Thanks for patiently explaining this to me :)

You're welcome :)

> So to tell the full story I'll change the commit log to something like
> below. Does it look ok to you?

Yes; but I'd change "So" to "This turned out to be wrong", and add a note
about changing the order for 0x002A too:

----------------------------------------------------------------------
ath9k: Really fix LED polarity for some Mini PCI AR9220 MB92 cards.

The active_high LED of my Wistron DNMA-92 is still being recognized as
active_low on 4.7.6 mainline. When I was preparing my former commit
0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92
cards.") to fix that I must have somehow messed up with testing, because
I tested the final version of that patch before sending it, and it was
apparently working; but now it is not working on 4.7.6 mainline.

I initially added the PCI_DEVICE_SUB section for 0x0029/0x2096 above the
PCI_VDEVICE section for 0x0029; but then I moved the former below the
latter after seeing how 0x002A sections were sorted in the file.

This turned out to be wrong: if a generic PCI_VDEVICE entry (that has
both subvendor and subdevice IDs set to PCI_ANY_ID) is put before a more
specific one (PCI_DEVICE_SUB), then the generic PCI_VDEVICE entry will
match first and will be used.

With this patch, 0x0029/0x2096 has finally got active_high LED on 4.7.6.

While I'm at it, let's fix 0x002A too by also moving its generic definition
below its specific ones.

Fixes: 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92 cards.")
Cc: <stable@vger.kernel.org> #4.7+
Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
[kvalo@qca.qualcomm.com: improve the commit log based on email discussions]
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
----------------------------------------------------------------------

Cheers,
Vittorio

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Valo, Kalle]

"Vittorio Gambaletta (VittGam)" <linux-wireless@vittgam.net> writes:

> Hello,
>
> On 12/10/2016 17:01:08 CEST, Valo, Kalle wrote:
>
>> So to tell the full story I'll change the commit log to something like
>> below. Does it look ok to you?
>
> Yes; but I'd change "So" to "This turned out to be wrong", and add a note
> about changing the order for 0x002A too:
>
> ----------------------------------------------------------------------
> ath9k: Really fix LED polarity for some Mini PCI AR9220 MB92 cards.
>
> The active_high LED of my Wistron DNMA-92 is still being recognized as
> active_low on 4.7.6 mainline. When I was preparing my former commit
> 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92
> cards.") to fix that I must have somehow messed up with testing, because
> I tested the final version of that patch before sending it, and it was
> apparently working; but now it is not working on 4.7.6 mainline.
>
> I initially added the PCI_DEVICE_SUB section for 0x0029/0x2096 above the
> PCI_VDEVICE section for 0x0029; but then I moved the former below the
> latter after seeing how 0x002A sections were sorted in the file.
>
> This turned out to be wrong: if a generic PCI_VDEVICE entry (that has
> both subvendor and subdevice IDs set to PCI_ANY_ID) is put before a more
> specific one (PCI_DEVICE_SUB), then the generic PCI_VDEVICE entry will
> match first and will be used.
>
> With this patch, 0x0029/0x2096 has finally got active_high LED on 4.7.6.
>
> While I'm at it, let's fix 0x002A too by also moving its generic definition
> below its specific ones.
>
> Fixes: 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92 cards.")
> Cc: <stable@vger.kernel.org> #4.7+
> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
> [kvalo@qca.qualcomm.com: improve the commit log based on email discussions]
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> ----------------------------------------------------------------------

Thanks, I updated the commit with your changes.

-- 
Kalle Valo