drivers returning wrong results from their _tx op

drivers returning wrong results from their _tx op

[Johannes Berg]

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

spatch:

@ r1 @
identifier _tx, _ops;
@@
 struct ieee80211_ops _ops = {
...
 .tx = _tx,
...
 };

@ r2 depends on r1 @
identifier r1._tx;
@@
static int _tx(...)
{
...
-return ...;
}


shows a lot...

First off: all those should return NETDEV_TX_OK/BUSY.

ath5k: returns 0/-1
ath9k: returns 0/-1
iwl-agn: returns 0 (== NETDEV_TX_OK, but still should be changed)
iwl3945: same
libertas_tf: same
rtl8180: same
p54: same (some paths)
rtl8187: that's why I started looking
zd1211: returns -E* codes


These need to be fixed, the bad ones are zd1211 and rtl8187, the -1 from
ath5k/9k is not much of a worry but should be fixed anyway.

Preferably, drivers should just drop frames when they can't handle them,
but keep in mind fragmentation and keep enough space for up to 9 or 10
(I think) fragments.

johannes

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

Re: drivers returning wrong results from their _tx op

[Bob Copeland]

On Mon, Jan 5, 2009 at 5:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> spatch:
>
> @ r1 @
> identifier _tx, _ops;

spatch is very neat...

> First off: all those should return NETDEV_TX_OK/BUSY.
> ath5k: returns 0/-1

Can the kerneldoc for mac80211 ops structure document it?

I'll send a patch for ath5k unless someone beats me to it.

-- 
Bob Copeland %% www.bobcopeland.com

Re: drivers returning wrong results from their _tx op

[Johannes Berg]

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

On Mon, 2009-01-05 at 17:24 -0500, Bob Copeland wrote:
> On Mon, Jan 5, 2009 at 5:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > spatch:
> >
> > @ r1 @
> > identifier _tx, _ops;
> 
> spatch is very neat...

Yes, indeed, it is, I've used it a few times already :)

> > First off: all those should return NETDEV_TX_OK/BUSY.
> > ath5k: returns 0/-1
> 
> Can the kerneldoc for mac80211 ops structure document it?

I guess it should, want to send a patch?

> I'll send a patch for ath5k unless someone beats me to it.

Thanks.

johannes

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

Re: drivers returning wrong results from their _tx op

[Bob Copeland]

On Tue, Jan 06, 2009 at 02:00:59PM +0100, Johannes Berg wrote:
> I guess it should, want to send a patch?

Some of the return values were already documented in the ops comment, 
and others like set_key() elsewhere.  Here are more, though I guessed 
on a couple of them...

From: Bob Copeland <me@bobcopeland.com>
Date: Thu, 8 Jan 2009 21:00:34 -0500
Subject: [PATCH] mac80211: document return codes from ops callbacks

For any callbacks in ieee80211_ops, specify what values the return
codes represent.  While at it, fix a couple of capitalization and
punctuation differences.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 include/net/mac80211.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b7b9014..903dade 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1170,6 +1170,7 @@ enum ieee80211_ampdu_mlme_action {
  *	configuration in the TX control data. This handler should,
  *	preferably, never fail and stop queues appropriately, more
  *	importantly, however, it must never fail for A-MPDU-queues.
+ *	Returns NETDEV_TX_OK, or NETDEV_TX_BUSY if it fails.
  *	Must be implemented and atomic.
  *
  * @start: Called before the first netdevice attached to the hardware
@@ -1210,9 +1211,12 @@ enum ieee80211_ampdu_mlme_action {
  *
  * @config: Handler for configuration requests. IEEE 802.11 code calls this
  *	function to change hardware configuration, e.g., channel.
+ *	This function should never fail but returns a negative error code
+ *	if it does.
  *
  * @config_interface: Handler for configuration requests related to interfaces
  *	(e.g. BSSID changes.)
+ *	Returns a negative error code which will be seen in userspace.
  *
  * @bss_info_changed: Handler for configuration requests related to BSS
  *	parameters that may vary during BSS's lifespan, and may affect low
@@ -1232,6 +1236,7 @@ enum ieee80211_ampdu_mlme_action {
  *	This callback can sleep, and is only called between add_interface
  *	and remove_interface calls, i.e. while the given virtual interface
  *	is enabled.
+ *	Returns a negative error code if the key can't be added.
  *
  * @update_tkip_key: See the section "Hardware crypto acceleration"
  * 	This callback will be called in the context of Rx. Called for drivers
@@ -1243,8 +1248,10 @@ enum ieee80211_ampdu_mlme_action {
  *	bands. When the scan finishes, ieee80211_scan_completed() must be
  *	called; note that it also must be called when the scan cannot finish
  *	because the hardware is turned off! Anything else is a bug!
+ *	Returns a negative error code which will be seen in userspace.
  *
- * @get_stats: return low-level statistics
+ * @get_stats: Return low-level statistics.
+ * 	Returns zero if statistics are available.
  *
  * @get_tkip_seq: If your device implements TKIP encryption in hardware this
  *	callback should be provided to read the TKIP transmit IVs (both IV32
@@ -1258,6 +1265,7 @@ enum ieee80211_ampdu_mlme_action {
  *
  * @conf_tx: Configure TX queue parameters (EDCF (aifs, cw_min, cw_max),
  *	bursting) for a hardware TX queue.
+ *	Returns a negative error code on failure.
  *
  * @get_tx_stats: Get statistics of the current TX queue status. This is used
  *	to get number of currently queued packets (queue length), maximum queue
@@ -1277,13 +1285,15 @@ enum ieee80211_ampdu_mlme_action {
  * @tx_last_beacon: Determine whether the last IBSS beacon was sent by us.
  *	This is needed only for IBSS mode and the result of this function is
  *	used to determine whether to reply to Probe Requests.
+ *	Returns non-zero if this device sent the last beacon.
  *
  * @ampdu_action: Perform a certain A-MPDU action
  * 	The RA/TID combination determines the destination and TID we want
  * 	the ampdu action to be performed for. The action is defined through
  * 	ieee80211_ampdu_mlme_action. Starting sequence number (@ssn)
- * 	is the first frame we expect to perform the action on. notice
+ * 	is the first frame we expect to perform the action on. Notice
  * 	that TX/RX_STOP can pass NULL for this parameter.
+ *	Returns a negative error code on failure.
  */
 struct ieee80211_ops {
 	int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb);
-- 
1.6.0.6

-- 
Bob Copeland %% www.bobcopeland.com

Re: drivers returning wrong results from their _tx op

[Kalle Valo]

Bob Copeland <me@bobcopeland.com> writes:

> On Tue, Jan 06, 2009 at 02:00:59PM +0100, Johannes Berg wrote:
>> I guess it should, want to send a patch?
>
> Some of the return values were already documented in the ops comment, 
> and others like set_key() elsewhere.  Here are more, though I guessed 
> on a couple of them...
>
> From: Bob Copeland <me@bobcopeland.com>
> Date: Thu, 8 Jan 2009 21:00:34 -0500
> Subject: [PATCH] mac80211: document return codes from ops callbacks
>
> For any callbacks in ieee80211_ops, specify what values the return
> codes represent.  While at it, fix a couple of capitalization and
> punctuation differences.
>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  include/net/mac80211.h |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index b7b9014..903dade 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1170,6 +1170,7 @@ enum ieee80211_ampdu_mlme_action {
>   *	configuration in the TX control data. This handler should,
>   *	preferably, never fail and stop queues appropriately, more
>   *	importantly, however, it must never fail for A-MPDU-queues.
> + *	Returns NETDEV_TX_OK, or NETDEV_TX_BUSY if it fails.
>   *	Must be implemented and atomic.

Based on discussion from netdev I have understood that NETDEV_TX_BUSY
is not recommended, it's better to stop the queue already earlier,
drop the frame or store the skb somewhere else temporarily. Johannes,
have I understood this correctly?

It would be nice to have this documented here.

-- 
Kalle Valo

Re: drivers returning wrong results from their _tx op

[Johannes Berg]

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

On Fri, 2009-01-09 at 10:13 +0200, Kalle Valo wrote:

> > @@ -1170,6 +1170,7 @@ enum ieee80211_ampdu_mlme_action {
> >   *	configuration in the TX control data. This handler should,
> >   *	preferably, never fail and stop queues appropriately, more
> >   *	importantly, however, it must never fail for A-MPDU-queues.
> > + *	Returns NETDEV_TX_OK, or NETDEV_TX_BUSY if it fails.
> >   *	Must be implemented and atomic.
> 
> Based on discussion from netdev I have understood that NETDEV_TX_BUSY
> is not recommended, it's better to stop the queue already earlier,
> drop the frame or store the skb somewhere else temporarily. Johannes,
> have I understood this correctly?

Yes. Well, returning BUSY _should_ work, but I'd like to remove it and
the complicated retry logic from mac80211. Only adm8211 really needs it,
afaik, all other drivers can just drop the skb in error conditions,
adm8211 can only transmit one skb at a time so needs this for fragmented
frames which don't adhere to stopping the queue, unfortunately. This I
may change at some time.

johannes

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

Re: drivers returning wrong results from their _tx op

[Bob Copeland]

On Fri, 09 Jan 2009 10:13:56 +0200, Kalle Valo wrote
> > @@ -1170,6 +1170,7 @@ enum ieee80211_ampdu_mlme_action {
> >   *	configuration in the TX control data. This handler should,
> >   *	preferably, never fail and stop queues appropriately, more
> >   *	importantly, however, it must never fail for A-MPDU-queues.
> > + *	Returns NETDEV_TX_OK, or NETDEV_TX_BUSY if it fails.
> >   *	Must be implemented and atomic.
> 
> Based on discussion from netdev I have understood that NETDEV_TX_BUSY
> is not recommended, it's better to stop the queue already earlier,
> drop the frame or store the skb somewhere else temporarily. Johannes,
> have I understood this correctly?
> 
> It would be nice to have this documented here.

Read the sentence before the added line :-)

I was just interested in what all the callbacks hope to return.  
There seem to be some discrepancies, e.g. tx_last_beacon returns a 
bool disguised as an int (0=failure), while get_stats returns an 
error code but only checks for 0=OK.  Whether the documentation is
ultimately worthwhile or not is an open question.

Also, unless my grep-fu is weak, get_tx_stats didn't seem to be 
used at all.

-- 
Bob Copeland %% www.bobcopeland.com

Re: drivers returning wrong results from their _tx op

[Kalle Valo]

"Bob Copeland" <me@bobcopeland.com> writes:

> On Fri, 09 Jan 2009 10:13:56 +0200, Kalle Valo wrote
>> > @@ -1170,6 +1170,7 @@ enum ieee80211_ampdu_mlme_action {
>> >   *	configuration in the TX control data. This handler should,
>> >   *	preferably, never fail and stop queues appropriately, more
>> >   *	importantly, however, it must never fail for A-MPDU-queues.
>> > + *	Returns NETDEV_TX_OK, or NETDEV_TX_BUSY if it fails.
>> >   *	Must be implemented and atomic.
>> 
>> Based on discussion from netdev I have understood that NETDEV_TX_BUSY
>> is not recommended, it's better to stop the queue already earlier,
>> drop the frame or store the skb somewhere else temporarily. Johannes,
>> have I understood this correctly?
>> 
>> It would be nice to have this documented here.
>
> Read the sentence before the added line :-)

I read it. Adding "or NETDEV_TX_BUSY if it fails" is just confusing,
better to state that it just should not fail.

-- 
Kalle Valo

Re: drivers returning wrong results from their _tx op

[Bob Copeland]

On Fri, 09 Jan 2009 18:17:41 +0200, Kalle Valo wrote
> I read it. Adding "or NETDEV_TX_BUSY if it fails" is just confusing,
> better to state that it just should not fail.

Fair enough, but the state of things today is some drivers do.
Yes, we're guilty of that in ath5k and it should be fixed.  But like 
->config(), if the callback should never ever fail, then returning
an error code at all is pretty suspect.  Would this be better?

"This function should return NETDEV_TX_OK except in very limited cases."

Then when someone asks which cases, you say "none"...

-- 
Bob Copeland %% www.bobcopeland.com

Re: drivers returning wrong results from their _tx op

[Kalle Valo]

"Bob Copeland" <me@bobcopeland.com> writes:

> On Fri, 09 Jan 2009 18:17:41 +0200, Kalle Valo wrote
>> I read it. Adding "or NETDEV_TX_BUSY if it fails" is just confusing,
>> better to state that it just should not fail.
>
> Fair enough, but the state of things today is some drivers do.
> Yes, we're guilty of that in ath5k and it should be fixed.  But like 
> ->config(), if the callback should never ever fail, then returning
> an error code at all is pretty suspect.

Yes, that's why it's already better to deny this kind of usage in
documentation already now.

> Would this be better?
>
> "This function should return NETDEV_TX_OK except in very limited cases."
>
> Then when someone asks which cases, you say "none"...

That's better.

Thanks for doing this. It's very important to document the return
values, especially for the people not familiar with mac80211.

-- 
Kalle Valo

Re: drivers returning wrong results from their _tx op

[Bob Copeland]

On Fri, Jan 09, 2009 at 08:47:53PM +0200, Kalle Valo wrote:
> Yes, that's why it's already better to deny this kind of usage in
> documentation already now.

Good point.. ok, here's an update.

From: Bob Copeland <me@bobcopeland.com>
Date: Thu, 8 Jan 2009 21:00:34 -0500
Subject: [PATCH] mac80211: document return codes from ops callbacks

For any callbacks in ieee80211_ops, specify what values the return
codes represent.  While at it, fix a couple of capitalization and
punctuation differences.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 include/net/mac80211.h |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 00f7ec0..6153abb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1218,6 +1218,8 @@ enum ieee80211_ampdu_mlme_action {
  *	configuration in the TX control data. This handler should,
  *	preferably, never fail and stop queues appropriately, more
  *	importantly, however, it must never fail for A-MPDU-queues.
+ *	This function should return NETDEV_TX_OK except in very
+ *	limited cases.
  *	Must be implemented and atomic.
  *
  * @start: Called before the first netdevice attached to the hardware
@@ -1258,9 +1260,12 @@ enum ieee80211_ampdu_mlme_action {
  *
  * @config: Handler for configuration requests. IEEE 802.11 code calls this
  *	function to change hardware configuration, e.g., channel.
+ *	This function should never fail but returns a negative error code
+ *	if it does.
  *
  * @config_interface: Handler for configuration requests related to interfaces
  *	(e.g. BSSID changes.)
+ *	Returns a negative error code which will be seen in userspace.
  *
  * @bss_info_changed: Handler for configuration requests related to BSS
  *	parameters that may vary during BSS's lifespan, and may affect low
@@ -1280,6 +1285,7 @@ enum ieee80211_ampdu_mlme_action {
  *	This callback can sleep, and is only called between add_interface
  *	and remove_interface calls, i.e. while the given virtual interface
  *	is enabled.
+ *	Returns a negative error code if the key can't be added.
  *
  * @update_tkip_key: See the section "Hardware crypto acceleration"
  * 	This callback will be called in the context of Rx. Called for drivers
@@ -1291,8 +1297,10 @@ enum ieee80211_ampdu_mlme_action {
  *	bands. When the scan finishes, ieee80211_scan_completed() must be
  *	called; note that it also must be called when the scan cannot finish
  *	because the hardware is turned off! Anything else is a bug!
+ *	Returns a negative error code which will be seen in userspace.
  *
- * @get_stats: return low-level statistics
+ * @get_stats: Return low-level statistics.
+ * 	Returns zero if statistics are available.
  *
  * @get_tkip_seq: If your device implements TKIP encryption in hardware this
  *	callback should be provided to read the TKIP transmit IVs (both IV32
@@ -1306,6 +1314,7 @@ enum ieee80211_ampdu_mlme_action {
  *
  * @conf_tx: Configure TX queue parameters (EDCF (aifs, cw_min, cw_max),
  *	bursting) for a hardware TX queue.
+ *	Returns a negative error code on failure.
  *
  * @get_tx_stats: Get statistics of the current TX queue status. This is used
  *	to get number of currently queued packets (queue length), maximum queue
@@ -1325,13 +1334,15 @@ enum ieee80211_ampdu_mlme_action {
  * @tx_last_beacon: Determine whether the last IBSS beacon was sent by us.
  *	This is needed only for IBSS mode and the result of this function is
  *	used to determine whether to reply to Probe Requests.
+ *	Returns non-zero if this device sent the last beacon.
  *
  * @ampdu_action: Perform a certain A-MPDU action
  * 	The RA/TID combination determines the destination and TID we want
  * 	the ampdu action to be performed for. The action is defined through
  * 	ieee80211_ampdu_mlme_action. Starting sequence number (@ssn)
- * 	is the first frame we expect to perform the action on. notice
+ * 	is the first frame we expect to perform the action on. Notice
  * 	that TX/RX_STOP can pass NULL for this parameter.
+ *	Returns a negative error code on failure.
  */
 struct ieee80211_ops {
 	int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb);
-- 
1.6.0.6

-- 
Bob Copeland %% www.bobcopeland.com

Re: drivers returning wrong results from their _tx op

[Kalle Valo]

Bob Copeland <me@bobcopeland.com> writes:

> On Fri, Jan 09, 2009 at 08:47:53PM +0200, Kalle Valo wrote:
>> Yes, that's why it's already better to deny this kind of usage in
>> documentation already now.
>
> Good point.. ok, here's an update.

Looks good to me.

> From: Bob Copeland <me@bobcopeland.com>
> Date: Thu, 8 Jan 2009 21:00:34 -0500
> Subject: [PATCH] mac80211: document return codes from ops callbacks
>
> For any callbacks in ieee80211_ops, specify what values the return
> codes represent.  While at it, fix a couple of capitalization and
> punctuation differences.
>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>

Reviewed-by: Kalle Valo <kalle.valo@iki.fi>

-- 
Kalle Valo