Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 17:26 +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. What kernel coding style? checkpatch isn't the arbiter of style, if that's the only problem. The DEFINE_PCI macro was a well reasoned addition when it was added in 2008. The problem was most people were getting the definition wrong. When we converted away from CONFIG_HOTPLUG, having this DEFINE_ meant that only one place needed changing instead of hundreds for PCI tables. The reason people were getting the PCI table wrong was mostly the init section specifiers which are now gone, but it has enough underlying utility (mostly constification) that I don't think we'd want to churn the kernel hugely to make a change to struct pci_table and then have to start detecting and fixing misuses. James ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? John -- John W. LinvilleSomeday the world will need a hero, and you linvi...@tuxdriver.com might be all we have. Be ready. ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] ath5k 5GHz Receive Diversity problem
Hi, Does the reported RSSI on both antennas actually match? Or is the non-main antenna RSSI crappy? If you flip the default antenna over (btu not disable diversity) does it still behave this way? -a On 17 July 2014 23:53, brettwri...@eaton.com wrote: Thanks Adrian and Wojtek, There is a bit called AR5K_PHY_AGCCTL_OFDM_DIV_DIS which led me to thinking it should be possible. At least now I can give up and work around it. I still don't fully understand why fast diversity works for CCK (11b) rates with the same length preamble as the OFDM rates. i.e. The radio definitely must be looking at the non-default antenna for those modulations, because it receives them fine - just not OFDM. However, there is a lot I don't understand about this radio ;o) Thanks Brett -Original Message- From: adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] On Behalf Of Adrian Chadd Sent: 17 July, 2014 6:13 PM To: Wojciech Dubowik Cc: ath5k-devel@lists.ath5k.org; Wright, Brett Subject: Re: [ath5k-devel] ath5k 5GHz Receive Diversity problem Hi, Right. It takes time for the hardware to figure out the signal level on each antenna - there's a coarse gain change and then a couple of fine gain changes so it can calculate RSSI. If the packet has a short preamble or it picks it up a little too late then yeah, you won't get any fast diversity. -a On 17 July 2014 01:01, Wojciech Dubowik wojciech.dubo...@neratec.com wrote: On 07/17/2014 03:24 AM, brettwri...@eaton.com wrote: Hello List, I am having a problem getting Rx diversity to work with both ar5213 and ar5413. What I have found is that the fast diversity works great for receiving 802.11b (i.e. CCK) modulations. But fast diversity for OFDM just does not seem to work (and yes diversity is turned ON). I am using controlled conditions, i.e. cable and attenuators, and find that basically on the non-default antenna there is effectively 30dB attenuation. This means that to successfully receive an OFDM frame on the non-default antenna the signal has to be 30dBm above the noise floor! Not very useful… Basically Rx diversity for 802.11a 5GHz and pure 802.11g is a no-go. Does anyone know if this should work – or perhaps Rx diversity for OFDM is just a no-go? BTW – I’ve tried this with ath5k/mac80211 as well as madwifi and openhal, as well as the old closed source madwifi hal. They all behave the same way. Adrian – I see you have made some changes to FreeBSD related to this – do you have any idea if this **should** work? Anyone else? AFAIK at least for OFDM the packet has to be seen first on default antenna. Then rx diversity is switching to the other antenna during preamble to check for a better signal. It means that first you have to be on a proper antenna and be able to decode a packet (or at least preamble). You can receive on the other one when you pump the signal 30dB because it's a typical isolation for antenna RF switch on reference designs. In our design it's typically 50dB and then for sure you won't get rx diversity to work if you are on the wrong antenna. That's why I know ;o) Rest is handled in SW i.e. if chipset has chosen alternate antenna three times in a row then switch default to the other one. Could be also based on tx failures. Br, Wojtek Thanks Brett ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 09:28 -0700, James Bottomley wrote: On Fri, 2014-07-18 at 17:26 +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. What kernel coding style? checkpatch isn't the arbiter of style, if that's the only problem. The DEFINE_PCI macro was a well reasoned addition when it was added in 2008. The problem was most people were getting the definition wrong. When we converted away from CONFIG_HOTPLUG, having this DEFINE_ meant that only one place needed changing instead of hundreds for PCI tables. The reason people were getting the PCI table wrong was mostly the init section specifiers which are now gone, but it has enough underlying utility (mostly constification) that I don't think we'd want to churn the kernel hugely to make a change to struct pci_table and then have to start detecting and fixing misuses. James -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html I would rather fix the misuses of the macro, than remove it. Could we possibly make checkpatch smart enough to tell when the macro is misused? Thanks, Jake ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 11:17 -0700, Greg KH wrote: On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote: On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? The macro is horrid, no other bus has this type of thing just to save a few characters in typing OK, so this is the macro: #define DEFINE_PCI_DEVICE_TABLE(_table) \ const struct pci_device_id _table[] Could you explain what's so horrible? The reason it's useful today is that people forget the const (and sometimes the [] making it a true table instead of a pointer). If you use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it wrongly (good) and you automatically get the correct annotations. We have almost 1000 more uses of the non-macro version than the macro version in the kernel today: $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l 262 $ git grep const struct pci_device_id | wc -l 1254 My big complaint is that we need to be consistant, either pick one or the other and stick to it. As the macro is the least used, it's easiest to fix up, and it also is more consistant with all other kernel subsystems which do not have such a macro. I've a weak preference for consistency, but not at the expense of hundreds of patches churning the kernel to remove an innocuous macro. Churn costs time and effort. As there is no need for the __init macro mess anymore, there's no real need for the DEFINE_PCI_DEVICE_TABLE macro either. I think checkpatch will catch the use of non-const users for the id table already today, it catches lots of other uses like this already. , so why should PCI be special in this regard anymore? I think the PCI usage dwarfs most other bus types now, so you could turn the question around. However, I don't think majority voting is a good guide to best practise; lets debate the merits for their own sake. Not really dwarf, USB is close with over 700 such structures: $ git grep const struct usb_device_id | wc -l 725 And i2c is almost just as big as PCI: $ git grep const struct i2c_device_id | wc -l 1223 So again, this macro is not consistent with the majority of PCI drivers, nor with any other type of device id declaration in the kernel, which is why I feel it should be removed. And finally, the PCI documentation itself says to not use this macro, so this isn't a new thing. From Documentation/PCI/pci.txt: The ID table is an array of struct pci_device_id entries ending with an all-zero entry. Definitions with static const are generally preferred. Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided. That wording went into the file last December, when we last talked about this and everyone in that discussion agreed to remove the macro for the above reasons. Consistency matters. In this case, I don't think it does that much ... a cut and paste either way (from a macro or non-macro based driver) yields correct code. Since there's no bug here and no apparent way to misuse the macro, why bother? Anyway, it's PCI code ... let the PCI maintainer decide. However, if he does want this do it as one big bang patch via either the PCI or Trivial tree (latter because Jiří has experience doing this, but the former might be useful so the decider feels the pain ...) James ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? The macro is horrid, no other bus has this type of thing just to save a few characters in typing OK, so this is the macro: #define DEFINE_PCI_DEVICE_TABLE(_table) \ const struct pci_device_id _table[] Could you explain what's so horrible? The reason it's useful today is that people forget the const (and sometimes the [] making it a true table instead of a pointer). If you use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it wrongly (good) and you automatically get the correct annotations. , so why should PCI be special in this regard anymore? I think the PCI usage dwarfs most other bus types now, so you could turn the question around. However, I don't think majority voting is a good guide to best practise; lets debate the merits for their own sake. James ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? The macro is horrid, no other bus has this type of thing just to save a few characters in typing, so why should PCI be special in this regard anymore? thanks, greg k-h ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Sat, Jul 19, 2014 at 07:14:12AM +1000, Dave Airlie wrote: We have almost 1000 more uses of the non-macro version than the macro version in the kernel today: $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l 262 $ git grep const struct pci_device_id | wc -l 1254 did you check for non-const ones? just to see if we have any of the broken case in the tree :-) I didn't :) as for consistency, pci_dev vs usb_device :-P Read farther down the email... ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
We have almost 1000 more uses of the non-macro version than the macro version in the kernel today: $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l 262 $ git grep const struct pci_device_id | wc -l 1254 did you check for non-const ones? just to see if we have any of the broken case in the tree :-) as for consistency, pci_dev vs usb_device :-P Dave. ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
