[PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Bob Copeland]

Review spotted a problem with the error handling in ath5k_hw_dma_stop:
a successful return from ath5k_hw_stop_tx_dma will be treated as
an error, so we always bail out of the loop after processing a single
active queue.  As a result, we may not actually stop some queues during
reset.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
These two patches fix some buglets I found when reviewing some old code.

 drivers/net/wireless/ath/ath5k/dma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
index 0064be7..e828b98 100644
--- a/drivers/net/wireless/ath/ath5k/dma.c
+++ b/drivers/net/wireless/ath/ath5k/dma.c
@@ -838,7 +838,7 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
 	for (i = 0; i < qmax; i++) {
 		err = ath5k_hw_stop_tx_dma(ah, i);
 		/* -EINVAL -> queue inactive */
-		if (err != -EINVAL)
+		if (err && err != -EINVAL)
 			return err;
 	}
 
-- 
1.7.1.1

[PATCH 2/2] ath5k: correct endianness of frame duration

[Bob Copeland]

The ath5k version of ieee80211_generic_frame_duration() returns
an __le16 for standard modes but a cpu-endian int for turbo/half/
quarter rates.  Make it always return cpu-endian values.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/pcu.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
index e5f2b96..a702817 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -86,7 +86,7 @@ int ath5k_hw_get_frame_duration(struct ath5k_hw *ah,
 	if (!ah->ah_bwmode) {
 		dur = ieee80211_generic_frame_duration(sc->hw,
 						NULL, len, rate);
-		return dur;
+		return le16_to_cpu(dur);
 	}
 
 	bitrate = rate->bitrate;
@@ -265,8 +265,6 @@ static inline void ath5k_hw_write_rate_duration(struct ath5k_hw *ah)
 		 * what rate we should choose to TX ACKs. */
 		tx_time = ath5k_hw_get_frame_duration(ah, 10, rate);
 
-		tx_time = le16_to_cpu(tx_time);
-
 		ath5k_hw_reg_write(ah, tx_time, reg);
 
 		if (!(rate->flags & IEEE80211_RATE_SHORT_PREAMBLE))
-- 
1.7.1.1

Re: [PATCH 2/2] ath5k: correct endianness of frame duration

[Bruno Randolf]

On Tue January 25 2011 13:31:44 Bob Copeland wrote:
> The ath5k version of ieee80211_generic_frame_duration() returns
> an __le16 for standard modes but a cpu-endian int for turbo/half/
> quarter rates.  Make it always return cpu-endian values.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/pcu.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/pcu.c
> b/drivers/net/wireless/ath/ath5k/pcu.c index e5f2b96..a702817 100644
> --- a/drivers/net/wireless/ath/ath5k/pcu.c
> +++ b/drivers/net/wireless/ath/ath5k/pcu.c
> @@ -86,7 +86,7 @@ int ath5k_hw_get_frame_duration(struct ath5k_hw *ah,
>  	if (!ah->ah_bwmode) {
>  		dur = ieee80211_generic_frame_duration(sc->hw,
>  						NULL, len, rate);
> -		return dur;
> +		return le16_to_cpu(dur);
>  	}
> 
>  	bitrate = rate->bitrate;
> @@ -265,8 +265,6 @@ static inline void ath5k_hw_write_rate_duration(struct
> ath5k_hw *ah) * what rate we should choose to TX ACKs. */
>  		tx_time = ath5k_hw_get_frame_duration(ah, 10, rate);
> 
> -		tx_time = le16_to_cpu(tx_time);
> -
>  		ath5k_hw_reg_write(ah, tx_time, reg);
> 
>  		if (!(rate->flags & IEEE80211_RATE_SHORT_PREAMBLE))

I wonder what effect this has in ath5k_hw_set_ifs_intervals() where 
ack_tx_time was little endian before...

But the change makes sense.

Acked-by: Bruno Randolf <br1@einfach.org>

Re: [PATCH 2/2] ath5k: correct endianness of frame duration

[Bob Copeland]

On Tue, Jan 25, 2011 at 02:23:31PM +0900, Bruno Randolf wrote:
> > The ath5k version of ieee80211_generic_frame_duration() returns
> > an __le16 for standard modes but a cpu-endian int for turbo/half/
> > quarter rates.  Make it always return cpu-endian values.
> 
> I wonder what effect this has in ath5k_hw_set_ifs_intervals() where 
> ack_tx_time was little endian before...

Good point, I didn't think about that.  My guess is EIFS would have
been much too large on big endian arches before the patch.  But then
EIFS resets after successful rx so maybe our many BE users (heh)
wouldn't have noticed except on very idle channels...

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] ath5k: correct endianness of frame duration

[Bruno Randolf]

On Tue January 25 2011 19:38:03 Bob Copeland wrote:
> On Tue, Jan 25, 2011 at 02:23:31PM +0900, Bruno Randolf wrote:
> > > The ath5k version of ieee80211_generic_frame_duration() returns
> > > an __le16 for standard modes but a cpu-endian int for turbo/half/
> > > quarter rates.  Make it always return cpu-endian values.
> > 
> > I wonder what effect this has in ath5k_hw_set_ifs_intervals() where
> > ack_tx_time was little endian before...
> 
> Good point, I didn't think about that.  My guess is EIFS would have
> been much too large on big endian arches before the patch.  But then
> EIFS resets after successful rx so maybe our many BE users (heh)
> wouldn't have noticed except on very idle channels...

Allright, just the EIFS is affected - shouldn't be a big deal. Anyhow it's 
correct now and was wrong before.

But you meant on a very busy channel, right?

I'm on a BE board ;) I can try to see what difference it makes tomorrow.

bruno

Re: [PATCH 2/2] ath5k: correct endianness of frame duration

[Bob Copeland]

On Tue, Jan 25, 2011 at 5:47 AM, Bruno Randolf <br1@einfach.org> wrote:
> Allright, just the EIFS is affected - shouldn't be a big deal. Anyhow it's
> correct now and was wrong before.
>
> But you meant on a very busy channel, right?
>
> I'm on a BE board ;) I can try to see what difference it makes tomorrow.

Err, yeah, braino.  Glad to know BE actually works :)

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] ath5k: correct endianness of frame duration

[Nick Kossifidis]

2011/1/25 Bob Copeland <me@bobcopeland.com>:
> The ath5k version of ieee80211_generic_frame_duration() returns
> an __le16 for standard modes but a cpu-endian int for turbo/half/
> quarter rates.  Make it always return cpu-endian values.
>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/pcu.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
> index e5f2b96..a702817 100644
> --- a/drivers/net/wireless/ath/ath5k/pcu.c
> +++ b/drivers/net/wireless/ath/ath5k/pcu.c
> @@ -86,7 +86,7 @@ int ath5k_hw_get_frame_duration(struct ath5k_hw *ah,
>        if (!ah->ah_bwmode) {
>                dur = ieee80211_generic_frame_duration(sc->hw,
>                                                NULL, len, rate);
> -               return dur;
> +               return le16_to_cpu(dur);
>        }
>
>        bitrate = rate->bitrate;
> @@ -265,8 +265,6 @@ static inline void ath5k_hw_write_rate_duration(struct ath5k_hw *ah)
>                 * what rate we should choose to TX ACKs. */
>                tx_time = ath5k_hw_get_frame_duration(ah, 10, rate);
>
> -               tx_time = le16_to_cpu(tx_time);
> -
>                ath5k_hw_reg_write(ah, tx_time, reg);
>
>                if (!(rate->flags & IEEE80211_RATE_SHORT_PREAMBLE))
> --
> 1.7.1.1

Acked-by: Nick Kossifidis <mickflemm@gmail.com>


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Bruno Randolf]

On Tue January 25 2011 13:31:43 Bob Copeland wrote:
> Review spotted a problem with the error handling in ath5k_hw_dma_stop:
> a successful return from ath5k_hw_stop_tx_dma will be treated as
> an error, so we always bail out of the loop after processing a single
> active queue.  As a result, we may not actually stop some queues during
> reset.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
> These two patches fix some buglets I found when reviewing some old code.
> 
>  drivers/net/wireless/ath/ath5k/dma.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/dma.c
> b/drivers/net/wireless/ath/ath5k/dma.c index 0064be7..e828b98 100644
> --- a/drivers/net/wireless/ath/ath5k/dma.c
> +++ b/drivers/net/wireless/ath/ath5k/dma.c
> @@ -838,7 +838,7 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
>  	for (i = 0; i < qmax; i++) {
>  		err = ath5k_hw_stop_tx_dma(ah, i);
>  		/* -EINVAL -> queue inactive */
> -		if (err != -EINVAL)
> +		if (err && err != -EINVAL)
>  			return err;
>  	}

Acked-by: Bruno Randolf <br1@einfach.org>

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Nick Kossifidis]

2011/1/25 Bob Copeland <me@bobcopeland.com>:
> Review spotted a problem with the error handling in ath5k_hw_dma_stop:
> a successful return from ath5k_hw_stop_tx_dma will be treated as
> an error, so we always bail out of the loop after processing a single
> active queue.  As a result, we may not actually stop some queues during
> reset.
>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
> These two patches fix some buglets I found when reviewing some old code.
>
>  drivers/net/wireless/ath/ath5k/dma.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
> index 0064be7..e828b98 100644
> --- a/drivers/net/wireless/ath/ath5k/dma.c
> +++ b/drivers/net/wireless/ath/ath5k/dma.c
> @@ -838,7 +838,7 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
>        for (i = 0; i < qmax; i++) {
>                err = ath5k_hw_stop_tx_dma(ah, i);
>                /* -EINVAL -> queue inactive */
> -               if (err != -EINVAL)
> +               if (err && err != -EINVAL)
>                        return err;
>        }
>
> --
> 1.7.1.1

Acked-by: Nick Kossifidis <mickflemm@gmail.com>



-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Stanislaw Gruszka]

On Mon, Jan 24, 2011 at 11:31:43PM -0500, Bob Copeland wrote:
> Review spotted a problem with the error handling in ath5k_hw_dma_stop:
> a successful return from ath5k_hw_stop_tx_dma will be treated as
> an error, so we always bail out of the loop after processing a single
> active queue.  As a result, we may not actually stop some queues during
> reset.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
> These two patches fix some buglets I found when reviewing some old code.
> 
>  drivers/net/wireless/ath/ath5k/dma.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
> index 0064be7..e828b98 100644
> --- a/drivers/net/wireless/ath/ath5k/dma.c
> +++ b/drivers/net/wireless/ath/ath5k/dma.c
> @@ -838,7 +838,7 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
>  	for (i = 0; i < qmax; i++) {
>  		err = ath5k_hw_stop_tx_dma(ah, i);
>  		/* -EINVAL -> queue inactive */
> -		if (err != -EINVAL)
> +		if (err && err != -EINVAL)
>  			return err;
>  	}

Patch is good, but does not make code fully correct. When last queue
is inactive, we return -EINVAL from ath5_hw_dma_stop(). So we need
also:

 		if (err && err != -EINVAL)
 			return err;
+		err = 0;
 	}

But perhaps, would be better just return 0 from 
ath5k_hw_stop_tx_dma() when queue is inactive.

I think that could fix "ath5k phy0: can't reset hardware (-5)"
bugs reported in a few places, so patch should go to stable
as well.

Stanislaw

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Bob Copeland]

From: Bob Copeland <me@bobcopeland.com>
Date: Mon, 24 Jan 2011 09:25:11 -0500
Subject: [PATCH] ath5k: fix error handling in ath5k_hw_dma_stop

Review spotted a problem with the error handling in ath5k_hw_dma_stop:
a successful return from ath5k_hw_stop_tx_dma will be treated as
an error, so we always bail out of the loop after processing a single
active queue.  As a result, we may not actually stop some queues during
reset.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---

> Patch is good, but does not make code fully correct. When last queue
> is inactive, we return -EINVAL from ath5_hw_dma_stop(). So we need
> also:
> 
>  		if (err && err != -EINVAL)
>  			return err;
> +		err = 0;
>  	}

> But perhaps, would be better just return 0 from 
> ath5k_hw_stop_tx_dma() when queue is inactive.

How about this version instead?  Although returning 0 in the inactive
queue case would be ok, I'd like to keep the diff small since this
seems to fix some reset problems people are now seeing in Linus's tree
and if so I'd like to get it in 2.6.38.

> I think that could fix "ath5k phy0: can't reset hardware (-5)"
> bugs reported in a few places, so patch should go to stable
> as well.

I thought this was older code but it actually landed after 2.6.37
so no need for stable CC.

As a side note, we still quit when the first error is encountered.
Maybe we should try stopping all of the queues anyway if one fails?
On the other hand we could spend a lot of time polling the registers
if the card is truly hosed, so I think this is best for now.

 drivers/net/wireless/ath/ath5k/dma.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
index 0064be7..21091c2 100644
--- a/drivers/net/wireless/ath/ath5k/dma.c
+++ b/drivers/net/wireless/ath/ath5k/dma.c
@@ -838,9 +838,9 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
 	for (i = 0; i < qmax; i++) {
 		err = ath5k_hw_stop_tx_dma(ah, i);
 		/* -EINVAL -> queue inactive */
-		if (err != -EINVAL)
+		if (err && err != -EINVAL)
 			return err;
 	}
 
-	return err;
+	return 0;
 }
-- 
1.7.1.1


-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Stanislaw Gruszka]

On Tue, Jan 25, 2011 at 08:50:34PM -0500, Bob Copeland wrote:
> From: Bob Copeland <me@bobcopeland.com>
> Date: Mon, 24 Jan 2011 09:25:11 -0500
> Subject: [PATCH] ath5k: fix error handling in ath5k_hw_dma_stop
> 
> Review spotted a problem with the error handling in ath5k_hw_dma_stop:
> a successful return from ath5k_hw_stop_tx_dma will be treated as
> an error, so we always bail out of the loop after processing a single
> active queue.  As a result, we may not actually stop some queues during
> reset.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
> 
> > Patch is good, but does not make code fully correct. When last queue
> > is inactive, we return -EINVAL from ath5_hw_dma_stop(). So we need
> > also:
> > 
> >  		if (err && err != -EINVAL)
> >  			return err;
> > +		err = 0;
> >  	}
> 
> > But perhaps, would be better just return 0 from 
> > ath5k_hw_stop_tx_dma() when queue is inactive.
> 
> How about this version instead? 
It's better :)

> diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
> index 0064be7..21091c2 100644
> --- a/drivers/net/wireless/ath/ath5k/dma.c
> +++ b/drivers/net/wireless/ath/ath5k/dma.c
> @@ -838,9 +838,9 @@ int ath5k_hw_dma_stop(struct ath5k_hw *ah)
>  	for (i = 0; i < qmax; i++) {
>  		err = ath5k_hw_stop_tx_dma(ah, i);
>  		/* -EINVAL -> queue inactive */
> -		if (err != -EINVAL)
> +		if (err && err != -EINVAL)
>  			return err;
>  	}
>  
> -	return err;
> +	return 0;
>  }

Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Nick Kossifidis]

2011/1/26 Bob Copeland <me@bobcopeland.com>:
>
> As a side note, we still quit when the first error is encountered.
> Maybe we should try stopping all of the queues anyway if one fails?
> On the other hand we could spend a lot of time polling the registers
> if the card is truly hosed, so I think this is best for now.
>

It's better to continue with hw reset if a queue fails to stop, it
usually means the dma unit needs reset.



-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 1/2] ath5k: fix error handling in ath5k_hw_dma_stop

[Bob Copeland]

On Wed, Jan 26, 2011 at 12:06 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2011/1/26 Bob Copeland <me@bobcopeland.com>:
>>
>> As a side note, we still quit when the first error is encountered.
>> Maybe we should try stopping all of the queues anyway if one fails?
>> On the other hand we could spend a lot of time polling the registers
>> if the card is truly hosed, so I think this is best for now.
>
> It's better to continue with hw reset if a queue fails to stop, it
> usually means the dma unit needs reset.

Yeah, I thought that may be the case.  So the code as it stands plus
this patch from the thread will do what we want -- continue reset as
soon as any queue exhibits problems.

-- 
Bob Copeland %% www.bobcopeland.com