[ath9k-devel] [RFC PATCH 1/2] ath9k: rename symbols in enum ath9k_internal_frame_type to avoid confusion

[ath9k-devel] [RFC PATCH 1/2] ath9k: rename symbols in enum ath9k_internal_frame_type to avoid confusion

[Pavel Roskin]

Symbols starting with "ATH9K_INT" are also used for interrupt mask.

Signed-off-by: Pavel Roskin <proski@gnu.org>
---
 drivers/net/wireless/ath/ath9k/rc.h      |    6 +++---
 drivers/net/wireless/ath/ath9k/virtual.c |    2 +-
 drivers/net/wireless/ath/ath9k/xmit.c    |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rc.h b/drivers/net/wireless/ath/ath9k/rc.h
index 36083dd..3d8d40c 100644
--- a/drivers/net/wireless/ath/ath9k/rc.h
+++ b/drivers/net/wireless/ath/ath9k/rc.h
@@ -176,9 +176,9 @@ struct ath_rate_priv {
 #define ATH_TX_INFO_UNDERRUN		(1 << 4)
 
 enum ath9k_internal_frame_type {
-	ATH9K_NOT_INTERNAL,
-	ATH9K_INT_PAUSE,
-	ATH9K_INT_UNPAUSE
+	ATH9K_IFT_NOT_INTERNAL,
+	ATH9K_IFT_PAUSE,
+	ATH9K_IFT_UNPAUSE
 };
 
 int ath_rate_control_register(void);
diff --git a/drivers/net/wireless/ath/ath9k/virtual.c b/drivers/net/wireless/ath/ath9k/virtual.c
index a43fbf8..e95aaa3 100644
--- a/drivers/net/wireless/ath/ath9k/virtual.c
+++ b/drivers/net/wireless/ath/ath9k/virtual.c
@@ -218,7 +218,7 @@ static int ath9k_send_nullfunc(struct ath_wiphy *aphy,
 
 	memset(&txctl, 0, sizeof(struct ath_tx_control));
 	txctl.txq = &sc->tx.txq[sc->tx.hwq_map[ATH9K_WME_AC_VO]];
-	txctl.frame_type = ps ? ATH9K_INT_PAUSE : ATH9K_INT_UNPAUSE;
+	txctl.frame_type = ps ? ATH9K_IFT_PAUSE : ATH9K_IFT_UNPAUSE;
 
 	if (ath_tx_start(aphy->hw, skb, &txctl) != 0)
 		goto exit;
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index a3b6cf2..7e1fad4 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1568,12 +1568,12 @@ static int ath_tx_setup_buffer(struct ieee80211_hw *hw, struct ath_buf *bf,
 
 	tx_info->pad[0] = 0;
 	switch (txctl->frame_type) {
-	case ATH9K_NOT_INTERNAL:
+	case ATH9K_IFT_NOT_INTERNAL:
 		break;
-	case ATH9K_INT_PAUSE:
+	case ATH9K_IFT_PAUSE:
 		tx_info->pad[0] |= ATH_TX_INFO_FRAME_TYPE_PAUSE;
 		/* fall through */
-	case ATH9K_INT_UNPAUSE:
+	case ATH9K_IFT_UNPAUSE:
 		tx_info->pad[0] |= ATH_TX_INFO_FRAME_TYPE_INTERNAL;
 		break;
 	}

[ath9k-devel] [RFC PATCH 2/2] ath9k: fix mixing of interrupt masks, remove ah->mask_reg

[Pavel Roskin]

ah->mask_reg is used inconsistently.  ath9k_hw_init_chain_masks() uses
it to cache the value it writes to AR_IMR.  But once
ath9k_hw_set_interrupts() is called, ah->mask_reg starts caching the
interrupt mask in the format used in sc->imask, which differs in several
bits.

Use sc->imask instead of ah->mask_reg as omask ath9k_hw_set_interrupts()
and ath9k_hw_updatetxtriglevel().  Remove ah->mask_reg, as it becomes
write only.  Use a local variable in ath9k_hw_init_chain_masks().

Signed-off-by: Pavel Roskin <proski@gnu.org>
Reported-by: Julia Lawall <julia@diku.dk>
---

Adding ath9k.h to hw.c and mac.c could raise some eyebrows, but the
alternative is moving imask from sc to ah, which would be more
intrusive.

I think sc and ah should be merged eventually, so all files that know
the ah structure will know sc as well.  There is no gain from hiding sc
from the low-level code.

 drivers/net/wireless/ath/ath9k/hw.c  |   19 +++++++++++--------
 drivers/net/wireless/ath/ath9k/hw.h  |    1 -
 drivers/net/wireless/ath/ath9k/mac.c |    5 ++++-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 77db932..791aeb9 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <asm/unaligned.h>
 
+#include "ath9k.h"
 #include "hw.h"
 #include "rc.h"
 #include "initvals.h"
@@ -1120,23 +1121,25 @@ static void ath9k_hw_init_chain_masks(struct ath_hw *ah)
 static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
 					  enum nl80211_iftype opmode)
 {
-	ah->mask_reg = AR_IMR_TXERR |
+	u32 imr_reg;
+
+	imr_reg = AR_IMR_TXERR |
 		AR_IMR_TXURN |
 		AR_IMR_RXERR |
 		AR_IMR_RXORN |
 		AR_IMR_BCNMISC;
 
 	if (ah->config.rx_intr_mitigation)
-		ah->mask_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
+		imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
 	else
-		ah->mask_reg |= AR_IMR_RXOK;
+		imr_reg |= AR_IMR_RXOK;
 
-	ah->mask_reg |= AR_IMR_TXOK;
+	imr_reg |= AR_IMR_TXOK;
 
 	if (opmode == NL80211_IFTYPE_AP)
-		ah->mask_reg |= AR_IMR_MIB;
+		imr_reg |= AR_IMR_MIB;
 
-	REG_WRITE(ah, AR_IMR, ah->mask_reg);
+	REG_WRITE(ah, AR_IMR, imr_reg);
 	ah->imrs2_reg |= AR_IMR_S2_GTT;
 	REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg);
 
@@ -2839,10 +2842,11 @@ EXPORT_SYMBOL(ath9k_hw_getisr);
 
 enum ath9k_int ath9k_hw_set_interrupts(struct ath_hw *ah, enum ath9k_int ints)
 {
-	u32 omask = ah->mask_reg;
 	u32 mask, mask2;
 	struct ath9k_hw_capabilities *pCap = &ah->caps;
 	struct ath_common *common = ath9k_hw_common(ah);
+	struct ath_softc *sc = (struct ath_softc *)common->priv;
+	u32 omask = sc->imask;
 
 	ath_print(common, ATH_DBG_INTERRUPT, "0x%x => 0x%x\n", omask, ints);
 
@@ -2911,7 +2915,6 @@ enum ath9k_int ath9k_hw_set_interrupts(struct ath_hw *ah, enum ath9k_int ints)
 			   AR_IMR_S2_TSFOOR | AR_IMR_S2_GTT | AR_IMR_S2_CST);
 	ah->imrs2_reg |= mask2;
 	REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg);
-	ah->mask_reg = ints;
 
 	if (!(pCap->hw_caps & ATH9K_HW_CAP_AUTOSLEEP)) {
 		if (ints & ATH9K_INT_TIM_TIMER)
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 6b03e16..906aaee 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -478,7 +478,6 @@ struct ath_hw {
 	struct ath9k_tx_queue_info txq[ATH9K_NUM_TX_QUEUES];
 
 	int16_t curchan_rad_index;
-	u32 mask_reg;
 	u32 imrs2_reg;
 	u32 txok_interrupt_mask;
 	u32 txerr_interrupt_mask;
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 7af823a..67da800 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -15,6 +15,7 @@
  */
 
 #include "hw.h"
+#include "ath9k.h"
 
 static void ath9k_hw_set_txq_interrupts(struct ath_hw *ah,
 					struct ath9k_tx_queue_info *qi)
@@ -99,13 +100,15 @@ EXPORT_SYMBOL(ath9k_hw_numtxpending);
  */
 bool ath9k_hw_updatetxtriglevel(struct ath_hw *ah, bool bIncTrigLevel)
 {
+	struct ath_common *common = ath9k_hw_common(ah);
+	struct ath_softc *sc = (struct ath_softc *)common->priv;
 	u32 txcfg, curLevel, newLevel;
 	enum ath9k_int omask;
 
 	if (ah->tx_trig_level >= ah->config.max_txtrig_level)
 		return false;
 
-	omask = ath9k_hw_set_interrupts(ah, ah->mask_reg & ~ATH9K_INT_GLOBAL);
+	omask = ath9k_hw_set_interrupts(ah, sc->imask & ~ATH9K_INT_GLOBAL);
 
 	txcfg = REG_READ(ah, AR_TXCFG);
 	curLevel = MS(txcfg, AR_FTRIG);

[ath9k-devel] [RFC PATCH 2/2] ath9k: fix mixing of interrupt masks, remove ah->mask_reg

[Luis R. Rodriguez]

On Mon, Mar 29, 2010 at 08:39:58PM -0700, Pavel Roskin wrote:
> ah->mask_reg is used inconsistently.  ath9k_hw_init_chain_masks() uses
> it to cache the value it writes to AR_IMR.  But once
> ath9k_hw_set_interrupts() is called, ah->mask_reg starts caching the
> interrupt mask in the format used in sc->imask, which differs in several
> bits.
> 
> Use sc->imask instead of ah->mask_reg as omask ath9k_hw_set_interrupts()
> and ath9k_hw_updatetxtriglevel().  Remove ah->mask_reg, as it becomes
> write only.  Use a local variable in ath9k_hw_init_chain_masks().
> 
> Signed-off-by: Pavel Roskin <proski@gnu.org>
> Reported-by: Julia Lawall <julia@diku.dk>
> ---
> 
> Adding ath9k.h to hw.c and mac.c could raise some eyebrows, but the
> alternative is moving imask from sc to ah, which would be more
> intrusive.
> 
> I think sc and ah should be merged eventually, so all files that know
> the ah structure will know sc as well.  There is no gain from hiding sc
> from the low-level code.
> 
>  drivers/net/wireless/ath/ath9k/hw.c  |   19 +++++++++++--------
>  drivers/net/wireless/ath/ath9k/hw.h  |    1 -
>  drivers/net/wireless/ath/ath9k/mac.c |    5 ++++-
>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
> index 77db932..791aeb9 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.c
> +++ b/drivers/net/wireless/ath/ath9k/hw.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <asm/unaligned.h>
>  
> +#include "ath9k.h"
>  #include "hw.h"
>  #include "rc.h"
>  #include "initvals.h"
> @@ -1120,23 +1121,25 @@ static void ath9k_hw_init_chain_masks(struct ath_hw *ah)
>  static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
>  					  enum nl80211_iftype opmode)
>  {
> -	ah->mask_reg = AR_IMR_TXERR |
> +	u32 imr_reg;
> +
> +	imr_reg = AR_IMR_TXERR |
>  		AR_IMR_TXURN |
>  		AR_IMR_RXERR |
>  		AR_IMR_RXORN |
>  		AR_IMR_BCNMISC;
>  
>  	if (ah->config.rx_intr_mitigation)
> -		ah->mask_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
> +		imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
>  	else
> -		ah->mask_reg |= AR_IMR_RXOK;
> +		imr_reg |= AR_IMR_RXOK;
>  
> -	ah->mask_reg |= AR_IMR_TXOK;
> +	imr_reg |= AR_IMR_TXOK;
>  
>  	if (opmode == NL80211_IFTYPE_AP)
> -		ah->mask_reg |= AR_IMR_MIB;
> +		imr_reg |= AR_IMR_MIB;
>  
> -	REG_WRITE(ah, AR_IMR, ah->mask_reg);
> +	REG_WRITE(ah, AR_IMR, imr_reg);
>  	ah->imrs2_reg |= AR_IMR_S2_GTT;
>  	REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg);
>  
> @@ -2839,10 +2842,11 @@ EXPORT_SYMBOL(ath9k_hw_getisr);
>  
>  enum ath9k_int ath9k_hw_set_interrupts(struct ath_hw *ah, enum ath9k_int ints)
>  {
> -	u32 omask = ah->mask_reg;
>  	u32 mask, mask2;
>  	struct ath9k_hw_capabilities *pCap = &ah->caps;
>  	struct ath_common *common = ath9k_hw_common(ah);
> +	struct ath_softc *sc = (struct ath_softc *)common->priv;

This is exactly what I was trying to prevent, lets please try to figure out
another way, NACK for now.

Consider

struct ath9k_htc_priv *priv = (struct ath9k_htc_priv *) common->priv;

  Luis

[ath9k-devel] [RFC PATCH 1/2] ath9k: rename symbols in enum ath9k_internal_frame_type to avoid confusion

[Luis R. Rodriguez]

On Mon, Mar 29, 2010 at 08:39:18PM -0700, Pavel Roskin wrote:
> Symbols starting with "ATH9K_INT" are also used for interrupt mask.
> 
> Signed-off-by: Pavel Roskin <proski@gnu.org>

Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

[ath9k-devel] [RFC PATCH 2/2] ath9k: fix mixing of interrupt masks, remove ah->mask_reg

[Pavel Roskin]

On Tue, 2010-03-30 at 09:52 -0700, Luis R. Rodriguez wrote:

> This is exactly what I was trying to prevent, lets please try to figure out
> another way, NACK for now.
> 
> Consider
> 
> struct ath9k_htc_priv *priv = (struct ath9k_htc_priv *) common->priv;

OK, then let's move imask from sc to ah.  It looks like ah->mask_reg was
misused exactly because sc->imask wasn't available in some files.

-- 
Regards,
Pavel Roskin