[PATCH 1/2] ath9k: Fix tx struck state with paprd

[PATCH 1/2] ath9k: Fix tx struck state with paprd

[Vasanthakumar Thiagarajan]

Paprd needs to be done only on active chains(not for all the chains
that hw can support). The paprd training frames which are sent
for inactive chains would be hanging on the hw queue without
getting transmitted and would make the connection so unstable.
This issue happens only with the hw which supports paprd cal(ar9003).

Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
Cc: stable@kernel.org
---
 drivers/net/wireless/ath/ath9k/main.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 8b327bc..a133878 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -270,6 +270,7 @@ static void ath_paprd_activate(struct ath_softc *sc)
 {
 	struct ath_hw *ah = sc->sc_ah;
 	struct ath9k_hw_cal_data *caldata = ah->caldata;
+	struct ath_common *common = ath9k_hw_common(ah);
 	int chain;
 
 	if (!caldata || !caldata->paprd_done)
@@ -278,7 +279,7 @@ static void ath_paprd_activate(struct ath_softc *sc)
 	ath9k_ps_wakeup(sc);
 	ar9003_paprd_enable(ah, false);
 	for (chain = 0; chain < AR9300_MAX_CHAINS; chain++) {
-		if (!(ah->caps.tx_chainmask & BIT(chain)))
+		if (!(common->tx_chainmask & BIT(chain)))
 			continue;
 
 		ar9003_paprd_populate_single_table(ah, caldata, chain);
@@ -300,6 +301,7 @@ void ath_paprd_calibrate(struct work_struct *work)
 	struct ieee80211_supported_band *sband = &sc->sbands[band];
 	struct ath_tx_control txctl;
 	struct ath9k_hw_cal_data *caldata = ah->caldata;
+	struct ath_common *common = ath9k_hw_common(ah);
 	int qnum, ftype;
 	int chain_ok = 0;
 	int chain;
@@ -333,7 +335,7 @@ void ath_paprd_calibrate(struct work_struct *work)
 	ath9k_ps_wakeup(sc);
 	ar9003_paprd_init_table(ah);
 	for (chain = 0; chain < AR9300_MAX_CHAINS; chain++) {
-		if (!(ah->caps.tx_chainmask & BIT(chain)))
+		if (!(common->tx_chainmask & BIT(chain)))
 			continue;
 
 		chain_ok = 0;
-- 
1.7.0.4

[PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Vasanthakumar Thiagarajan]

Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
---
 drivers/net/wireless/ath/ath9k/hw.h   |    1 +
 drivers/net/wireless/ath/ath9k/main.c |   18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index df47f79..c1b4962 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -645,6 +645,7 @@ struct ath_hw {
 	struct ath9k_hw_capabilities caps;
 	struct ath9k_channel channels[38];
 	struct ath9k_channel *curchan;
+	struct ath9k_channel prev_paprd_chan;
 
 	union {
 		struct ar5416_eeprom_def def;
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index a133878..9150788 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -266,6 +266,20 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
 	return r;
 }
 
+static bool is_paprd_done(struct ath_hw *ah)
+{
+	struct ath9k_channel *curchan, *paprd_chan;
+
+	curchan = ah->curchan;
+	paprd_chan = &ah->prev_paprd_chan;
+
+	if ((paprd_chan->channel == curchan->channel) &&
+	    paprd_chan->chanmode == curchan->chanmode)
+		return true;
+
+	return false;
+}
+
 static void ath_paprd_activate(struct ath_softc *sc)
 {
 	struct ath_hw *ah = sc->sc_ah;
@@ -375,6 +389,8 @@ void ath_paprd_calibrate(struct work_struct *work)
 
 	if (chain_ok) {
 		caldata->paprd_done = true;
+		memcpy(&ah->prev_paprd_chan, ah->curchan,
+		       sizeof(struct ath9k_channel));
 		ath_paprd_activate(sc);
 	}
 
@@ -489,7 +505,7 @@ set_timer:
 
 	mod_timer(&common->ani.timer, jiffies + msecs_to_jiffies(cal_interval));
 	if ((sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_PAPRD) && ah->caldata) {
-		if (!ah->caldata->paprd_done)
+		if (!is_paprd_done(ah))
 			ieee80211_queue_work(sc->hw, &sc->paprd_work);
 		else
 			ath_paprd_activate(sc);
-- 
1.7.0.4

Re: [PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Felix Fietkau]

On 2010-09-21 7:54 AM, Vasanthakumar Thiagarajan wrote:
> Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> ---
>  drivers/net/wireless/ath/ath9k/hw.h   |    1 +
>  drivers/net/wireless/ath/ath9k/main.c |   18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index df47f79..c1b4962 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -645,6 +645,7 @@ struct ath_hw {
>  	struct ath9k_hw_capabilities caps;
>  	struct ath9k_channel channels[38];
>  	struct ath9k_channel *curchan;
> +	struct ath9k_channel prev_paprd_chan;
>  
>  	union {
>  		struct ar5416_eeprom_def def;
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index a133878..9150788 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -266,6 +266,20 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>  	return r;
>  }
>  
> +static bool is_paprd_done(struct ath_hw *ah)
> +{
> +	struct ath9k_channel *curchan, *paprd_chan;
> +
> +	curchan = ah->curchan;
> +	paprd_chan = &ah->prev_paprd_chan;
> +
> +	if ((paprd_chan->channel == curchan->channel) &&
> +	    paprd_chan->chanmode == curchan->chanmode)
> +		return true;
> +
> +	return false;
> +}
That seems like code duplication to me. The caldata already has the
channel number and the channel flags. ath9k_hw_reset() clears the entire
caldata whenever that changes. Because of that, ah->caldata->paprd_done
should have already been set to zero automatically after the reset
triggered by an operating channel change.
Is that part not working, or why did you write this patch?
Either way, we should not have a separate check just for paprd, it
belongs to the other calibrations.

- Felix

Re: [PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Vasanthakumar Thiagarajan]

On Tue, Sep 21, 2010 at 03:36:28PM +0530, Felix Fietkau wrote:
> On 2010-09-21 7:54 AM, Vasanthakumar Thiagarajan wrote:
> > Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> > ---
> >  drivers/net/wireless/ath/ath9k/hw.h   |    1 +
> >  drivers/net/wireless/ath/ath9k/main.c |   18 +++++++++++++++++-
> >  2 files changed, 18 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> > index df47f79..c1b4962 100644
> > --- a/drivers/net/wireless/ath/ath9k/hw.h
> > +++ b/drivers/net/wireless/ath/ath9k/hw.h
> > @@ -645,6 +645,7 @@ struct ath_hw {
> >  	struct ath9k_hw_capabilities caps;
> >  	struct ath9k_channel channels[38];
> >  	struct ath9k_channel *curchan;
> > +	struct ath9k_channel prev_paprd_chan;
> >  
> >  	union {
> >  		struct ar5416_eeprom_def def;
> > diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> > index a133878..9150788 100644
> > --- a/drivers/net/wireless/ath/ath9k/main.c
> > +++ b/drivers/net/wireless/ath/ath9k/main.c
> > @@ -266,6 +266,20 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
> >  	return r;
> >  }
> >  
> > +static bool is_paprd_done(struct ath_hw *ah)
> > +{
> > +	struct ath9k_channel *curchan, *paprd_chan;
> > +
> > +	curchan = ah->curchan;
> > +	paprd_chan = &ah->prev_paprd_chan;
> > +
> > +	if ((paprd_chan->channel == curchan->channel) &&
> > +	    paprd_chan->chanmode == curchan->chanmode)
> > +		return true;
> > +
> > +	return false;
> > +}
> That seems like code duplication to me. The caldata already has the
> channel number and the channel flags. ath9k_hw_reset() clears the entire
> caldata whenever that changes. Because of that, ah->caldata->paprd_done
> should have already been set to zero automatically after the reset
> triggered by an operating channel change.
> Is that part not working, or why did you write this patch?
> Either way, we should not have a separate check just for paprd, it
> belongs to the other calibrations.

I don't want to do paprd again whenever coming back from off-channel
(like during background scanning).

Vasanth

Re: [PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Felix Fietkau]

On 2010-09-21 12:17 PM, Vasanthakumar Thiagarajan wrote:
> On Tue, Sep 21, 2010 at 03:36:28PM +0530, Felix Fietkau wrote:
>> That seems like code duplication to me. The caldata already has the
>> channel number and the channel flags. ath9k_hw_reset() clears the entire
>> caldata whenever that changes. Because of that, ah->caldata->paprd_done
>> should have already been set to zero automatically after the reset
>> triggered by an operating channel change.
>> Is that part not working, or why did you write this patch?
>> Either way, we should not have a separate check just for paprd, it
>> belongs to the other calibrations.
> 
> I don't want to do paprd again whenever coming back from off-channel
> (like during background scanning).
That's not what it does. The caldata is only reset after *operating*
channel changes, not just after off-channel activity.
The reason this works is that for off-channel activity, the caldata
pointer is not passed to the hw reset function, so it can't reset any
data there. The intention behind that is that offchannel activity should
never trigger any long calibration activity, nor change the state of the
existing long calibration data.

- Felix

Re: [PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Vasanthakumar Thiagarajan]

On Tue, Sep 21, 2010 at 03:55:01PM +0530, Felix Fietkau wrote:
> On 2010-09-21 12:17 PM, Vasanthakumar Thiagarajan wrote:
> > On Tue, Sep 21, 2010 at 03:36:28PM +0530, Felix Fietkau wrote:
> >> That seems like code duplication to me. The caldata already has the
> >> channel number and the channel flags. ath9k_hw_reset() clears the entire
> >> caldata whenever that changes. Because of that, ah->caldata->paprd_done
> >> should have already been set to zero automatically after the reset
> >> triggered by an operating channel change.
> >> Is that part not working, or why did you write this patch?
> >> Either way, we should not have a separate check just for paprd, it
> >> belongs to the other calibrations.
> > 
> > I don't want to do paprd again whenever coming back from off-channel
> > (like during background scanning).
> That's not what it does. The caldata is only reset after *operating*
> channel changes, not just after off-channel activity.
> The reason this works is that for off-channel activity, the caldata
> pointer is not passed to the hw reset function, so it can't reset any
> data there. The intention behind that is that offchannel activity should
> never trigger any long calibration activity, nor change the state of the
> existing long calibration data.

yeah I see that. probably I completely ignored this
SC_OP_OFFCHANNNEL for sometime, thanks.


Vasanth

Re: [PATCH 2/2] ath9k: Kick start paprd calibration whenever operating channel changes

[Vasanthakumar Thiagarajan]

On Tue, Sep 21, 2010 at 11:24:47AM +0530, Vasanthakumar Thiagarajan wrote:
> Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> ---
>  drivers/net/wireless/ath/ath9k/hw.h   |    1 +
>  drivers/net/wireless/ath/ath9k/main.c |   18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index df47f79..c1b4962 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -645,6 +645,7 @@ struct ath_hw {
>  	struct ath9k_hw_capabilities caps;
>  	struct ath9k_channel channels[38];
>  	struct ath9k_channel *curchan;
> +	struct ath9k_channel prev_paprd_chan;
>  
>  	union {
>  		struct ar5416_eeprom_def def;
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index a133878..9150788 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -266,6 +266,20 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>  	return r;
>  }
>  
> +static bool is_paprd_done(struct ath_hw *ah)
> +{
> +	struct ath9k_channel *curchan, *paprd_chan;
> +
> +	curchan = ah->curchan;
> +	paprd_chan = &ah->prev_paprd_chan;
> +
> +	if ((paprd_chan->channel == curchan->channel) &&
> +	    paprd_chan->chanmode == curchan->chanmode)
> +		return true;
> +
> +	return false;
> +}
> +
>  static void ath_paprd_activate(struct ath_softc *sc)
>  {
>  	struct ath_hw *ah = sc->sc_ah;
> @@ -375,6 +389,8 @@ void ath_paprd_calibrate(struct work_struct *work)
>  
>  	if (chain_ok) {
>  		caldata->paprd_done = true;
> +		memcpy(&ah->prev_paprd_chan, ah->curchan,
> +		       sizeof(struct ath9k_channel));
>  		ath_paprd_activate(sc);
>  	}
>  
> @@ -489,7 +505,7 @@ set_timer:
>  
>  	mod_timer(&common->ani.timer, jiffies + msecs_to_jiffies(cal_interval));
>  	if ((sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_PAPRD) && ah->caldata) {
> -		if (!ah->caldata->paprd_done)
> +		if (!is_paprd_done(ah))
>  			ieee80211_queue_work(sc->hw, &sc->paprd_work);
>  		else
>  			ath_paprd_activate(sc);

Please drop this particular one as it looks redundant.

Vasanth