Re: [ath5k-devel] [PATCH v2 00/12] ath5k: A few updates + cleanups + kerneldoc
On Fri, Nov 25, 2011 at 7:40 PM, Nick Kossifidis mickfl...@gmail.com wrote: This series of patches includes some stability fixes related to interrupt handling, an updated version of a calibration-related patch I posted some time ago but never submited here and a new module parameter to disable GPIO based rfkill switch (we had a few bug reports on that recently). It also adds kernel doc on all ath5k_hw_* functions and structs (needs some more work but it should be fine, I reviewed the output, spell-ckecked it etc) and a few minor fixes and optimizations. v2: Adress comments from Adrian, Felix, Jiri and Bob. Thanks a lot for the review ! Nick Kossifidis (12): ath5k: Switch from read-and-clear to write-to-clear method when handling PISR/SISR registers ath5k: Add TXNOFRM to INT_TX_ALL ath5k: Cleanups v1 ath5k: Calibration re-work ath5k: Use usleep_range where possible ath5k: Cleanups v2 + add kerneldoc on all hw functions ath5k: We always do full calibration on AR5210 ath5k: Add a module parameter to disable hw rf kill switch ath5k: MRR support and 2GHz radio override belong in ah_capabilities ath5k: ath5k_ani_period_restart only touches struct ath5k_ani_state ath5k: Renumber hw queue ids ath5k: Optimize ath5k_cw_validate drivers/net/wireless/ath/ath5k/ahb.c | 4 +- drivers/net/wireless/ath/ath5k/ani.c | 91 ++-- drivers/net/wireless/ath/ath5k/ani.h | 32 +- drivers/net/wireless/ath/ath5k/ath5k.h | 569 +-- drivers/net/wireless/ath/ath5k/attach.c | 16 +- drivers/net/wireless/ath/ath5k/base.c | 287 +++ drivers/net/wireless/ath/ath5k/caps.c | 27 +- drivers/net/wireless/ath/ath5k/desc.c | 217 ++-- drivers/net/wireless/ath/ath5k/desc.h | 124 +++-- drivers/net/wireless/ath/ath5k/dma.c | 370 - drivers/net/wireless/ath/ath5k/gpio.c | 81 +++- drivers/net/wireless/ath/ath5k/initvals.c | 75 ++- drivers/net/wireless/ath/ath5k/pci.c | 2 +- drivers/net/wireless/ath/ath5k/pcu.c | 222 +--- drivers/net/wireless/ath/ath5k/phy.c | 853 +++-- drivers/net/wireless/ath/ath5k/qcu.c | 143 -- drivers/net/wireless/ath/ath5k/reg.h | 27 +- drivers/net/wireless/ath/ath5k/reset.c | 230 ++-- drivers/net/wireless/ath/ath5k/rfbuffer.h | 59 ++- drivers/net/wireless/ath/ath5k/rfgain.h | 22 +- 20 files changed, 2465 insertions(+), 986 deletions(-) -- 1.7.8.rc3 Unfortunately, patchwork.kernel.org is still down, thus I have extracted your patchset from your emails. I have git-am-ed them in my local linux-next (next-2025) GIT. ( Patchset attached ). I have only seen this little typo by having a quick view on the single patch: [ drivers/net/wireless/ath/ath5k/pcu.c ] + * -Different operating modes: AP, STA, IBSS --- Space before Different Any special thing you want to be tested? How can I easily generate the (kernel) docs for ath5k (I mean I don't want to generate a complete kernel-doc)? - Sedat - [1] https://patchwork.kernel.org/project/linux-wireless/list/ ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH v2 01/12] ath5k: Switch from read-and-clear to write-to-clear method when handling PISR/SISR registers
2011/11/26 Adrian Chadd adr...@freebsd.org: Hi, Review #2: One thing that I discovered whilst debugging some silly races in FreeBSD was that the update of the txq active mask wasn't done atomically. Ie, when you check this: +++ b/drivers/net/wireless/ath/ath5k/base.c @@ -1689,7 +1689,7 @@ ath5k_tasklet_tx(unsigned long data) struct ath5k_hw *ah = (void *)data; for (i = 0; i AR5K_NUM_TX_QUEUES; i++) - if (ah-txqs[i].setup (ah-ah_txq_isr BIT(i))) + if (ah-txqs[i].setup (ah-ah_txq_isr_txok_all BIT(i))) ath5k_tx_processq(ah, ah-txqs[i]); The way that the original code did it was: * update the HAL TX mask bits inside the HAL getisr routine, so it would just update the HAL idea of txqactive; * Then, ath_hal_gettxintrtxqs() would pass back to the driver the set of txq bits currently active, and reset those that are active (with a mask, so you can read only a few) * the ath_intr() routine would simply schedule a tasklet call when a TX interrupt came in; * .. and then the TX completion tasklet would call ath_hal_gettxintrtxqs() for each task bit to see if it needed servicing. I'm not aware of ath_hal_gettxintrqs etc, this was the patch I wrote on this... http://www.mail-archive.com/ath5k-devel@lists.ath5k.org/msg01312.html Now, in previous generations of hardware/kernel, this may not have raced - ath_intr() may have been a fast interrupt handler and thus would occur atomically on a single CPU - so you don't have any races. But these days, ath_intr() is just a software interrupt and can occur in parallel with another CPU executing the completion handler. So since there was no locking around the TXQ bit updates and clear, it was very possible that a very busy TX device would miss TX queue notifications, as the CPU running the TX completion would read-and-clear the TXQ active bits in the HAL, whilst another CPU was running ath_intr() which was trying to do a read/modify/write of those same bits. I _think_ felix fixed this in ath9k by hiding this stuff behind the PCU lock. I've also fixed it in FreeBSD. I think it's worth re-evaluating what is going on in ath5k and maybe add some similar locking. Oh wait. Where are you clearing these bits? It doesn't look like you're even clearing the ah_txq_isr bits in the most recent ath5k tree I have here. This means you're likely not missing events; but you're likely always scanning those queues. Making this code a bit pointless. :) Yup we don't clean them but that's not a bug, we still only check active queues that produced interrupts, not everything. Anyway we have txq-lock already so it's not a big deal to implement this I'll post a patch on top of this patchset. Anyway, I'd tidy this all up in a subsequent patchset. Get the interrupt changes in, then figure out how to properly lock the interrupt TXQ bit set path (the isr) and the clear path (once you've tested whether the TXQ needed servicing and then cleared the bit.) HTH, adrian Thanks again for your comments ;-) -- GPG ID: 0xEE878588 As you read this post global entropy rises. Have Fun ;-) Nick ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
Re: [ath5k-devel] [PATCH v2 01/12] ath5k: Switch from read-and-clear to write-to-clear method when handling PISR/SISR registers
On 28 November 2011 03:59, Nick Kossifidis mickfl...@gmail.com wrote: Oh wait. Where are you clearing these bits? It doesn't look like you're even clearing the ah_txq_isr bits in the most recent ath5k tree I have here. This means you're likely not missing events; but you're likely always scanning those queues. Making this code a bit pointless. :) Yup we don't clean them but that's not a bug, we still only check active queues that produced interrupts, not everything. Anyway we have txq-lock already so it's not a big deal to implement this I'll post a patch on top of this patchset. Right. but you're likely only using one active TXQ (and then CAB and beacon queues, which you may not be actually checking the completion status of) it won't matter. It just means if people start using 1 hardware TXQ, you're going to check the completion status on all of them. That's not a big deal, it just means you'll spend some CPU cycles and memory accesses checking queues you don't need to. You can't protect that interrupt field behind the TXQ lock either, since you'll have multiple reader/writers from outside the TXQ. Hence why I put it behind my PCU lock (which I use to protect shared PCU state, rather than PCU access like how felix did with ath9k.) Adrian ___ ath5k-devel mailing list ath5k-devel@lists.ath5k.org https://lists.ath5k.org/mailman/listinfo/ath5k-devel
