[PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Daniel Wagner]

It is not allowed to use NETDEV_TX_BUSY in tx path anymore.

Signed-off-by: Daniel Wagner <wagi@monom.org>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00mac.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
index 77af1df..64a70c6 100644
--- a/drivers/net/wireless/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
@@ -46,7 +46,7 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
 	skb = dev_alloc_skb(size + rt2x00dev->hw->extra_tx_headroom);
 	if (!skb) {
 		WARNING(rt2x00dev, "Failed to create RTS/CTS frame.\n");
-		return NETDEV_TX_BUSY;
+		return -1;
 	}
 
 	skb_reserve(skb, rt2x00dev->hw->extra_tx_headroom);
@@ -84,7 +84,7 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
 
 	if (rt2x00queue_write_tx_frame(queue, skb)) {
 		WARNING(rt2x00dev, "Failed to send RTS/CTS frame.\n");
-		return NETDEV_TX_BUSY;
+		return -1;
 	}
 
 	return NETDEV_TX_OK;
@@ -143,12 +143,14 @@ int rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	    !rt2x00dev->ops->hw->set_rts_threshold) {
 		if (rt2x00queue_available(queue) <= 1) {
 			ieee80211_stop_queue(rt2x00dev->hw, qid);
-			return NETDEV_TX_BUSY;
+			dev_kfree_skb_any(skb);
+			return NETDEV_TX_OK;
 		}
 
 		if (rt2x00mac_tx_rts_cts(rt2x00dev, queue, skb)) {
 			ieee80211_stop_queue(rt2x00dev->hw, qid);
-			return NETDEV_TX_BUSY;
+			dev_kfree_skb_any(skb);
+			return NETDEV_TX_OK;
 		}
 	}
 
@@ -166,7 +168,8 @@ int rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 
 	if (rt2x00queue_write_tx_frame(queue, skb)) {
 		ieee80211_stop_queue(rt2x00dev->hw, qid);
-		return NETDEV_TX_BUSY;
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
 	}
 
 	if (rt2x00queue_threshold(queue))
-- 
1.5.6.2

Re: [PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Ivo van Doorn]

On Wednesday 23 July 2008, Daniel Wagner wrote:
> It is not allowed to use NETDEV_TX_BUSY in tx path anymore.

If not, then why is mac80211 checking and handling the return value
and is tx() still a function returning an int. mac80211 is actually requeueing
the frame when the hardware fails to send it, so why should that be completely blocked?

> Signed-off-by: Daniel Wagner <wagi@monom.org>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> ---
>  drivers/net/wireless/rt2x00/rt2x00mac.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
> index 77af1df..64a70c6 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
> @@ -46,7 +46,7 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
>  	skb = dev_alloc_skb(size + rt2x00dev->hw->extra_tx_headroom);
>  	if (!skb) {
>  		WARNING(rt2x00dev, "Failed to create RTS/CTS frame.\n");
> -		return NETDEV_TX_BUSY;
> +		return -1;

I am kind of missing the point here, this patch seems to come down to:
	We can't return TX_BUSY so we return a random other value

Ivo

Re: [PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Wed, 2008-07-23 at 18:22 +0200, Ivo van Doorn wrote:
> On Wednesday 23 July 2008, Daniel Wagner wrote:
> > It is not allowed to use NETDEV_TX_BUSY in tx path anymore.
> 
> If not, then why is mac80211 checking and handling the return value
> and is tx() still a function returning an int. mac80211 is actually requeueing
> the frame when the hardware fails to send it, so why should that be completely blocked?

Well, we want to remove that code from mac80211 because it's horribly
ugly. But yes, if you stop queues it _should_ work.

> >  		WARNING(rt2x00dev, "Failed to create RTS/CTS frame.\n");
> > -		return NETDEV_TX_BUSY;
> > +		return -1;
> 
> I am kind of missing the point here, this patch seems to come down to:
> 	We can't return TX_BUSY so we return a random other value

Yeah that's fishy. But I think this error code is just in this rts/cts
function and getting handled in ->tx()

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Daniel Wagner]

Ivo van Doorn wrote:
> On Wednesday 23 July 2008, Daniel Wagner wrote:
>> It is not allowed to use NETDEV_TX_BUSY in tx path anymore.
> 
> If not, then why is mac80211 checking and handling the return value
> and is tx() still a function returning an int. mac80211 is actually requeueing
> the frame when the hardware fails to send it, so why should that be completely blocked?

Well, I might be completely wrong here. I got this idea from following mail on netdev:
http://marc.info/?l=linux-wireless&m=121025252321824&w=2

>> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
>> @@ -46,7 +46,7 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
>>  	skb = dev_alloc_skb(size + rt2x00dev->hw->extra_tx_headroom);
>>  	if (!skb) {
>>  		WARNING(rt2x00dev, "Failed to create RTS/CTS frame.\n");
>> -		return NETDEV_TX_BUSY;
>> +		return -1;
> 
> I am kind of missing the point here, this patch seems to come down to:
> 	We can't return TX_BUSY so we return a random other value

Unfortunately yes. The last return in this function is still NETDEV_TX_OK. My idea
was to return 0 on success in error -1. Is this not the expected normal
'typeless' return behavior of a function, no? 

daniel

Re: [PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Daniel Wagner]

Ivo van Doorn wrote:
> On Wednesday 23 July 2008, Daniel Wagner wrote:
>> Ivo van Doorn wrote:
>>> On Wednesday 23 July 2008, Daniel Wagner wrote:
>>>> It is not allowed to use NETDEV_TX_BUSY in tx path anymore.
>>> If not, then why is mac80211 checking and handling the return value
>>> and is tx() still a function returning an int. mac80211 is actually requeueing
>>> the frame when the hardware fails to send it, so why should that be completely blocked?
>> Well, I might be completely wrong here. I got this idea from following mail on netdev:
>> http://marc.info/?l=linux-wireless&m=121025252321824&w=2
> 
> Hmm, well Johannes just indicated that the code I mentioned earlier will be removed,
> in that case I am fine with a patch like this, however with a few adjustments. ;)

Sure :)

> Please make it a decent -E... error code then. Looks far less obscure then -1.

No problemo.

> Also could you make the exit code with
> 
> 	ieee80211_stop_queue(rt2x00dev->hw, qid);
> 	dev_kfree_skb_any(skb);
> 	return NETDEV_TX_OK;
> 
> and exit_fail goto?

yes

daniel

Re: [PATCH] rt2x00mac: In error case stop netdev queue, free skb and return NETDEV_TX_OK

[Ivo van Doorn]

On Wednesday 23 July 2008, Daniel Wagner wrote:
> Ivo van Doorn wrote:
> > On Wednesday 23 July 2008, Daniel Wagner wrote:
> >> It is not allowed to use NETDEV_TX_BUSY in tx path anymore.
> > 
> > If not, then why is mac80211 checking and handling the return value
> > and is tx() still a function returning an int. mac80211 is actually requeueing
> > the frame when the hardware fails to send it, so why should that be completely blocked?
> 
> Well, I might be completely wrong here. I got this idea from following mail on netdev:
> http://marc.info/?l=linux-wireless&m=121025252321824&w=2

Hmm, well Johannes just indicated that the code I mentioned earlier will be removed,
in that case I am fine with a patch like this, however with a few adjustments. ;)

> >> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
> >> @@ -46,7 +46,7 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
> >>  	skb = dev_alloc_skb(size + rt2x00dev->hw->extra_tx_headroom);
> >>  	if (!skb) {
> >>  		WARNING(rt2x00dev, "Failed to create RTS/CTS frame.\n");
> >> -		return NETDEV_TX_BUSY;
> >> +		return -1;
> > 
> > I am kind of missing the point here, this patch seems to come down to:
> > 	We can't return TX_BUSY so we return a random other value
> 
> Unfortunately yes. The last return in this function is still NETDEV_TX_OK. My idea
> was to return 0 on success in error -1. Is this not the expected normal
> 'typeless' return behavior of a function, no? 

Please make it a decent -E... error code then. Looks far less obscure then -1.

Also could you make the exit code with

	ieee80211_stop_queue(rt2x00dev->hw, qid);
	dev_kfree_skb_any(skb);
	return NETDEV_TX_OK;

and exit_fail goto?

Saves a lot of duplicate code. ;)

Thanks,

Ivo