ath9k: Move generic entries below specific ones in ath_pci_id_table.

PATCH In review Vittorio Gambaletta (VittGam) 4 participants 9 messages Oct 3, 2016 – Nov 15, 2016

Subsystems (2)

Stable branch Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

Vittorio Gambaletta (VittGam)

author

2016-10-03 10:00 UTC #0 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

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

Cc <[email protected]>

Signed-off-by Vittorio Gambaletta <[email protected]>

---

drivers/net/wireless/ath/ath9k/pci.c +5 -2

@@ -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,

Kalle Valo

2016-10-04 15:46 UTC #1 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Quoted 8 lines · 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 <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>

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:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
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.

Signature · 1 line

Kalle Valo

Vittorio Gambaletta (VittGam)

author

2016-10-04 18:14 UTC #2 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Hello,

Quoted 5 lines · Kalle Valo

Quoted 3 lines · Vittorio Gambaletta (VittGam)
If generic entries are positioned above specific ones, then the former will be matched first and used instead of the latter.

Quoted 8 lines

Quoted 5 lines
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>
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.

Quoted 9 lines

And your email headers look weird:

To:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Date:   Mon, 3 Oct 2016 12:00:56 +0200

Quoted 4 lines

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...

Kalle Valo

2016-10-12 13:34 UTC #3 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Quoted 14 lines · Vittorio Gambaletta (VittGam)

Hello,
Quoted 11 lines · Kalle Valo
Quoted 8 lines · 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 <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>

Quoted 5 lines

Quoted 2 lines
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.

Quoted 11 lines

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?

Quoted 4 lines

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.

Quoted 9 lines

And your email headers look weird:

To:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Date:   Mon, 3 Oct 2016 12:00:56 +0200

Quoted 7 lines

Quoted 4 lines
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.

Signature · 1 line

Kalle Valo

Vittorio Gambaletta (VittGam)

author

2016-10-12 14:13 UTC #4 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Hello,

Quoted 16 lines · Kalle Valo

Quoted 14 lines · Vittorio Gambaletta (VittGam)
Hello,
Quoted 11 lines · Kalle Valo
Quoted 8 lines · 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 <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Quoted 5 lines

Quoted 5 lines
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.

Quoted 8 lines

Quoted 6 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 18 lines

Quoted 9 lines
Quoted 9 lines
And your email headers look weird:
To:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Cc:     <[email protected]>
Date:   Mon, 3 Oct 2016 12:00:56 +0200
Quoted 7 lines
Quoted 4 lines
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.

Valo

2016-10-12 15:01 UTC #5 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Quoted 8 lines · Vittorio Gambaletta (VittGam)

Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

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.

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 <[email protected]> #4.7+

Signed-off-by Vittorio Gambaletta <[email protected]>

[kvalo at qca.qualcomm.com: improve the commit log based on email discussions]

Signed-off-by Kalle Valo <[email protected]>

----------------------------------------------------------------------

Signature · 1 line

Kalle Valo

Vittorio Gambaletta (VittGam)

author

2016-10-14 09:49 UTC #6 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Hello,

Quoted 14 lines · Kalle

Quoted 12 lines · Vittorio Gambaletta (VittGam)
Quoted 7 lines
Quoted 4 lines
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.

Quoted 5 lines

Quoted 3 lines
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 :)

Quoted 2 lines

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.

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 <[email protected]> #4.7+

Signed-off-by Vittorio Gambaletta <[email protected]>

[kvalo at qca.qualcomm.com: improve the commit log based on email discussions]

Signed-off-by Kalle Valo <[email protected]>

----------------------------------------------------------------------

Cheers,
Vittorio

Valo

2016-11-15 14:49 UTC #7 lore raw

PATCH 1/1 ath9k: Move generic entries below specific ones in ath_pci_id_table.

Quoted 34 lines · Vittorio Gambaletta (VittGam)

Hello,
Quoted 2 lines · Kalle
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.

Quoted 6 lines

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

Thanks, I updated the commit with your changes.

Signature · 1 line

Kalle Valo

Kalle Valo

2016-11-15 14:54 UTC #8 lore raw

In reply to Vittorio Gambaletta (VittGam)

Quoted 34 lines · 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 <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>
Signed-off-by Kalle Valo <[email protected]>
Signed-off-by Vittorio Gambaletta <[email protected]>
Signed-off-by Kalle Valo <[email protected]>
drivers/net/wireless/ath/ath9k/pci.c +1 -2
@@ -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 */

drivers/net/wireless/ath/ath9k/pci.c line 94

Quoted 10 lines

drivers/net/wireless/ath/ath9k/pci.c +4 -0

 			 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,

Patch applied to ath-next branch of ath.git, thanks.

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

Signature · 6 lines

https://patchwork.kernel.org/patch/9360361/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches