On Wed, Feb 01, 2012 at 11:35:13AM -0800, Ben Greear wrote:
> On 02/01/2012 08:05 AM, Rajkumar Manoharan wrote:
>
> Some more comments below.....
>
>
> >+static bool ath9k_check_dcu_chain_state(u32 dma_dbg, int max_limit,
> >+ int *hang_state, int *hang_pos)
> >+{
> >+ static u32 hang_sign[] = {5, 6, 9};
> >+ u32 chain_state, dcs_pos, i;
> >+
> >+ for (dcs_pos = 0; dcs_pos< max_limit; dcs_pos++) {
> >+ chain_state = (dma_dbg>> (5 * dcs_pos))& 0x1f;
> >+ for (i = 0; i< 3; i++) {
> >+ if (chain_state == hang_sign[i]) {
> >+ *hang_state = chain_state;
> >+ *hang_pos = dcs_pos;
> >+ return true;
> >+ }
> >+ }
> >+ }
> >+ return false;
> >+}
>
> Perhaps you could add some comments to the code above to describe
> what the '5, 6, 9' and other constants mean?
>
sure. these are the dcu_chain_state values. will update the variable name.
> >+#define DCU_COMPLETE_STATE 1
> >+#define NUM_STATUS_READS 50
> >+static bool ath9k_detect_mac_hang(struct ath_hw *ah)
> >+{
> >+ u32 chain_state, comp_state, dcs_reg = AR_DMADBG_4;
> >+ u32 i, hang_pos, hang_state;
> >+
> >+ comp_state = REG_READ(ah, AR_DMADBG_6);
> >+
> >+ if ((comp_state& 0x3) != DCU_COMPLETE_STATE) {
> >+ ath_dbg(ath9k_hw_common(ah), RESET,
> >+ "MAC Hang signature not found at DCU complete\n");
> >+ return false;
> >+ }
>
> Same with the 0x3 (maybe #define what those bits mean and or them together instead
> of using the 0x3?)
>
ok.
> >+
> >+ chain_state = REG_READ(ah, dcs_reg);
> >+ if (ath9k_check_dcu_chain_state(chain_state, 6,&hang_state,&hang_pos))
> >+ goto hang_check_iter;
>
> And why did you choose a '6' here?
>
number of dcu chain states consisted in that dma_dbg register. I'll update in the
function prototype.
> >+
> >+ dcs_reg = AR_DMADBG_5;
> >+ chain_state = REG_READ(ah, dcs_reg);
> >+ if (ath9k_check_dcu_chain_state(chain_state, 4,&hang_state,&hang_pos))
> >+ goto hang_check_iter;
>
> And the '4'?
>
same as above.
> >+void ath_start_rx_poll(struct ath_softc *sc, const u32 msec)
> >+{
> >+ if (!AR_SREV_9300_20_OR_LATER(sc->sc_ah))
> >+ return;
> >+
> >+ if (!(sc->sc_flags& SC_OP_PRIM_STA_VIF))
> >+ return;
> >+
> >+ mod_timer(&sc->rx_poll_timer, jiffies + msecs_to_jiffies(msec));
> >+}
>
> Since the rx-poll timer isn't started when SC_OP_PRIM_STA_VIF is
> not set, should you always call the ath_start_rx_poll method
> even if ani is disabled in the ath9k_bss_iter method since
> the PRIM_STA_VIF flag is set earlier in that code)?
>
Goog catch. it should be out of ani check.
> > static int ath9k_add_interface(struct ieee80211_hw *hw,
> > struct ieee80211_vif *vif)
> >@@ -1948,6 +2086,8 @@ static void ath9k_bss_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
> > if (!common->disable_ani) {
> > sc->sc_flags |= SC_OP_ANI_RUN;
> > ath_start_ani(common);
> >+ sc->rx.stop_rx_poll = false;
> >+ ath_start_rx_poll(sc, 300);
Thanks for your comments.
--
Rajkumar
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
