[ath9k-devel] [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id

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

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: <***@codeaurora.org>
Cc: <linux-***@vger.kernel.org>
Cc: <ath9k-***@qca.qualcomm.com>
Cc: <ath9k-***@venema.h4ckr.net>
Cc: <***@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

Vittorio Gambaletta (VittGam)

2016-10-04 18:14:10 UTC

Hello,

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

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.

Post by Kalle Valo
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...

Kalle Valo

2016-10-12 13:34:46 UTC

Post by Vittorio Gambaletta (VittGam)
Hello,

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

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.

Post by Vittorio Gambaletta (VittGam)
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?

Post by Vittorio Gambaletta (VittGam)
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.

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

Vittorio Gambaletta (VittGam)

2016-10-12 14:13:38 UTC

Hello,

Post by Vittorio Gambaletta (VittGam)
Hello,

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

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.

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.

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.

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

2016-10-12 15:01:08 UTC

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: <***@vger.kernel.org> #4.7+
Signed-off-by: Vittorio Gambaletta <***@vittgam.net>
[***@qca.qualcomm.com: improve the commit log based on email discussions]
Signed-off-by: Kalle Valo <***@qca.qualcomm.com>
----------------------------------------------------------------------

--
Kalle Valo

Vittorio Gambaletta (VittGam)

2016-10-14 09:49:40 UTC

Hello,

Post by Valo, Kalle

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 :)

Post by Valo, 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.

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

Cheers,
Vittorio

Valo, Kalle

2016-11-15 14:49:16 UTC