[Proposal]TX flags

[Proposal]TX flags

[Gábor Stefanik]

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

I propose the following suggested Radiotap features to be standardised:

* A new, 16-bit wide "TX flags" field (field number 15), containing
the following flags:
   - TX failed due to excessive retries,
   - CTS-to-self protection,
   - RTS/CTS-handshake,
   - Frame shall not be ACKed ("No-ACK"), and
   - Sequence number shall not be recalculated ("No-Seq").

Rationale for these changes:

     1. The "excessive retries" flag can be used by the drivers to
indicate that despite numerous retransmissions, the frame was never
acknowledged. This is useful for drivers that can't return the number
of retransmissions used, e.g. when retransmissions are handled by
hardware, with no feedback about the retries used to the driver. This
should never be set together with the No-ACK flag. It only makes sense
to use this on feedback of transmitted frames. This is currently
implemented in the Linux kernel.
     2. The CTS-to-self protection flag is useful both when frames are
sent and for feedback of transmissions. The userspace can send a frame
with this flag set, to request the driver to transmit the frame with
CTS-to-self protection (this is not implemented anywhere yet). On
feedback, the driver can indicate that it automatically transmitted
the frame using CTS-to-self protection, useful if the driver can't
return the actual CTS frame (this is implemented in Linux).
     3. The RTS/CTS handshake flag can be used the same way as the
CTS-to-self flag, to request/feedback the use of RTS/CTS handshakes
for transmitting frames. The feedback part is implemented in Linux.
     4. The No-ACK flag indicates that the frame being sent shall not
be ACKed, and as such, should only be transmitted once, with no
retransmissions. This only makes sense when sending frames from
userspace, and is useful for sending frames that are not to be ACKed
according to the IEEE 802.11 specification (e.g. multicasts, ACKs,
beacons, malformed/non-standard frames).
     5. The No-Seq flag is used when sending frames from the
userspace, to indicate that the frame already has its sequence number
preconfigured, and should not be renumbered by the low-level. This is
useful for sending fragments of a frame one-by-one (fragments need to
have the same sequence number to allow correct reassembly).

This can be accomplished with the following Radiotap changes:
* Define a new field, TX flags, at number 15, with the following parameters:
   - Field contents: an unsigned 16-bit integer,
   - Field alignment: 2;
   - Define bit 0 (mask 0x0001) of this field as "TX failed due to
excessive retries",
   - Define bit 1 (mask 0x0002) as "CTS-to-self protection",
   - Define bit 2 (mask 0x0004) as "RTS/CTS handshake",
   - Define bit 3 (mask 0x0008) as "Frame shall not be ACKed",
   - Define bit 4 (mask 0x0010) as "Pre-calculated sequence number",
   - Reserve all other bits in this field (mask 0xffe0) for future use.

Two patches have been attached to implement these changes in:
 * the Linux mac80211 wireless stack (already implements the field
with bits 0, 1 and 2 for feedback only). The attached patch implements
bits 3 and 4 for sending.
 * the Wireshark network traffic analyzer, allowing dissection of the
entire field.

If no one objects to this proposal in 7 days, I will resubmit it as a
formal RFA, to allow eventual adoption.

--Gábor Stefanik

[-- Attachment #2: wireshark-radiotap-txflags.patch --]
[-- Type: text/x-patch, Size: 6366 bytes --]

 packet-radiotap.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

Index: epan/dissectors/packet-radiotap.c
===================================================================
--- epan/dissectors/packet-radiotap.c	(revision 28045)
+++ epan/dissectors/packet-radiotap.c	(working copy)
@@ -105,6 +105,7 @@
     IEEE80211_RADIOTAP_DB_ANTSIGNAL = 12,
     IEEE80211_RADIOTAP_DB_ANTNOISE = 13,
     IEEE80211_RADIOTAP_FCS = 14,
+    IEEE80211_RADIOTAP_TX_FLAGS = 15,
     IEEE80211_RADIOTAP_XCHANNEL = 18,
     IEEE80211_RADIOTAP_EXT = 31
 };
@@ -168,6 +169,15 @@
 #define	IEEE80211_RADIOTAP_F_BADFCS	0x40	/* does not pass FCS check */
 #define	IEEE80211_RADIOTAP_F_SHORTGI	0x80	/* HT short GI */
 
+/* For IEEE80211_RADIOTAP_TX_FLAGS */
+#define IEEE80211_RADIOTAP_F_TX_FAIL	0x0001	/* failed due to excessive
+						 * retries */
+#define IEEE80211_RADIOTAP_F_TX_CTS	0x0002	/* used cts 'protection' */
+#define IEEE80211_RADIOTAP_F_TX_RTS	0x0004	/* used rts/cts handshake */
+#define IEEE80211_RADIOTAP_F_TX_NOACK	0x0008	/* frame should not be ACKed */
+#define IEEE80211_RADIOTAP_F_TX_NOSEQ	0x0010	/* sequence number handled
+						 * by userspace */
+
 /* XXX need max array size */
 static const int ieee80211_htrates[16] = {
 	13,		/* IFM_IEEE80211_MCS0 */
@@ -211,6 +221,12 @@
 static int hf_radiotap_channel_flags_sturbo = -1;
 static int hf_radiotap_channel_flags_half = -1;
 static int hf_radiotap_channel_flags_quarter = -1;
+static int hf_radiotap_txflags = -1;
+static int hf_radiotap_txflags_fail = -1;
+static int hf_radiotap_txflags_cts = -1;
+static int hf_radiotap_txflags_rts = -1;
+static int hf_radiotap_txflags_noack = -1;
+static int hf_radiotap_txflags_noseq = -1;
 static int hf_radiotap_xchannel = -1;
 static int hf_radiotap_xchannel_frequency = -1;
 static int hf_radiotap_xchannel_flags = -1;
@@ -260,6 +276,7 @@
 static int hf_radiotap_present_db_antsignal = -1;
 static int hf_radiotap_present_db_antnoise = -1;
 static int hf_radiotap_present_fcs = -1;
+static int hf_radiotap_present_txflags = -1;
 static int hf_radiotap_present_xchannel = -1;
 static int hf_radiotap_present_ext = -1;
 
@@ -282,6 +299,7 @@
 static gint ett_radiotap_present = -1;
 static gint ett_radiotap_flags = -1;
 static gint ett_radiotap_channel_flags = -1;
+static gint ett_radiotap_txflags = -1;
 static gint ett_radiotap_xchannel_flags = -1;
 
 static dissector_handle_t ieee80211_handle;
@@ -451,6 +469,7 @@
 #define RADIOTAP_MASK_DB_ANTSIGNAL          0x00001000
 #define RADIOTAP_MASK_DB_ANTNOISE           0x00002000
 #define RADIOTAP_MASK_FCS                   0x00004000
+#define RADIOTAP_MASK_TXFLAGS               0x00008000
 #define RADIOTAP_MASK_XCHANNEL              0x00040000
 #define RADIOTAP_MASK_EXT                   0x80000000
 
@@ -530,6 +549,11 @@
 	FT_BOOLEAN, 32, NULL, RADIOTAP_MASK_FCS,
 	"Specifies if the FCS field is present", HFILL } },
 
+    { &hf_radiotap_present_txflags,
+      { "TX Flags", "radiotap.present.txflags",
+	FT_BOOLEAN, 32, NULL, RADIOTAP_MASK_TXFLAGS,
+	"Specifies if the TX flags field is present", HFILL } },
+
     { &hf_radiotap_present_xchannel,
       { "Channel+", "radiotap.present.xchannel",
 	FT_BOOLEAN, 32, NULL, RADIOTAP_MASK_XCHANNEL,
@@ -653,6 +677,35 @@
        { "Quarter Rate Channel (5MHz Channel Width)", "radiotap.channel.type.quarter",
 	 FT_BOOLEAN, 16, NULL, 0x8000, "Channel Type Quarter Rate", HFILL } },
 
+    { &hf_radiotap_txflags,
+      { "TX flags", "radiotap.txflags",
+	FT_UINT16, BASE_HEX, NULL, 0x0, "", HFILL } },
+
+    { &hf_radiotap_txflags_fail,
+      { "Excessive retries", "radiotap.txflags.fail",
+	FT_BOOLEAN, 16, NULL, IEEE80211_RADIOTAP_F_TX_FAIL,
+	"Frame TX failed due to excessive retries", HFILL } },
+
+    { &hf_radiotap_txflags_cts,
+      { "CTS-to-self", "radiotap.txflags.cts",
+	FT_BOOLEAN, 16, NULL, IEEE80211_RADIOTAP_F_TX_CTS,
+	"Frame used CTS-to-self protection", HFILL } },
+
+    { &hf_radiotap_txflags_rts,
+      { "RTS/CTS", "radiotap.txflags.rts",
+	FT_BOOLEAN, 16, NULL, IEEE80211_RADIOTAP_F_TX_RTS,
+	"Frame was transmitted with an RTS/CTS-handshake", HFILL } },
+
+    { &hf_radiotap_txflags_noack,
+      { "No ACK expected", "radiotap.txflags.noack",
+	FT_BOOLEAN, 16, NULL, IEEE80211_RADIOTAP_F_TX_NOACK,
+	"Frame was transmitted once with no waiting for an ACK", HFILL } },
+
+    { &hf_radiotap_txflags_noseq,
+      { "No sequence counter", "radiotap.txflags.noseq",
+	FT_BOOLEAN, 16, NULL, IEEE80211_RADIOTAP_F_TX_NOSEQ,
+	"Frame had its sequence number configured by userspace", HFILL } },
+
     { &hf_radiotap_xchannel,
       { "Channel number", "radiotap.xchannel",
 	FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL } },
@@ -780,6 +833,7 @@
     &ett_radiotap_present,
     &ett_radiotap_flags,
     &ett_radiotap_channel_flags,
+    &ett_radiotap_txflags,
     &ett_radiotap_xchannel_flags
   };
 
@@ -896,6 +950,8 @@
 	    tvb, 4, 4, TRUE);
 	proto_tree_add_item(present_tree, hf_radiotap_present_fcs,
 	    tvb, 4, 4, TRUE);
+	proto_tree_add_item(present_tree, hf_radiotap_present_txflags,
+	    tvb, 4, 4, TRUE);
 	proto_tree_add_item(present_tree, hf_radiotap_present_xchannel,
 	    tvb, 4, 4, TRUE);
 	proto_tree_add_item(present_tree, hf_radiotap_present_ext,
@@ -1242,6 +1298,34 @@
 	    offset+=4;
 	    length_remaining-=4;
 	    break;
+	case IEEE80211_RADIOTAP_TX_FLAGS: {
+	    proto_item *it;
+
+	    align_offset = ALIGN_OFFSET(offset, 2);
+	    offset += align_offset;
+	    length_remaining -= align_offset;
+	    if (length_remaining < 2)
+		break;
+	    if (tree) {
+		flags = tvb_get_letohs(tvb, offset);
+		it = proto_tree_add_uint(radiotap_tree, hf_radiotap_txflags,
+			tvb, offset, 2, flags);
+		flags_tree = proto_item_add_subtree(it, ett_radiotap_txflags);
+		proto_tree_add_boolean(flags_tree, hf_radiotap_txflags_fail,
+			tvb, offset, 1, flags);
+		proto_tree_add_boolean(flags_tree, hf_radiotap_txflags_cts,
+			tvb, offset, 1, flags);
+		proto_tree_add_boolean(flags_tree, hf_radiotap_txflags_rts,
+			tvb, offset, 1, flags);
+		proto_tree_add_boolean(flags_tree, hf_radiotap_txflags_noack,
+			tvb, offset, 1, flags);
+		proto_tree_add_boolean(flags_tree, hf_radiotap_txflags_noseq,
+			tvb, offset, 1, flags);
+		}
+		offset+=2;
+	        length_remaining-=2;
+	    break;
+	}
 	default:
 	    /*
 	     * This indicates a field whose size we do not

[-- Attachment #3: linux-radiotap-txflags.patch --]
[-- Type: text/x-patch, Size: 4488 bytes --]

 Documentation/networking/mac80211-injection.txt |   12 +++++++++++-
 include/net/ieee80211_radiotap.h                |    3 +++
 net/mac80211/tx.c                               |   21 ++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/mac80211-injection.txt b/Documentation/networking/mac80211-injection.txt
index 84906ef..cd80f4a 100644
--- a/Documentation/networking/mac80211-injection.txt
+++ b/Documentation/networking/mac80211-injection.txt
@@ -36,10 +36,20 @@ radiotap headers and used to control injection:
    IEEE80211_RADIOTAP_F_FCS: FCS will be removed and recalculated
    IEEE80211_RADIOTAP_F_WEP: frame will be encrypted if key available
    IEEE80211_RADIOTAP_F_FRAG: frame will be fragmented if longer than the
-			      current fragmentation threshold. Note that
+			      current fragmentation threshold. (Note that
 			      this flag is only reliable when software
 			      fragmentation is enabled)
 
+
+ * IEEE80211_RADIOTAP_TX_FLAGS
+
+   IEEE80211_RADIOTAP_F_TX_NOACK: frame will be considered one that is not
+				  expected to be ACKed, and will only be
+				  transmitted once, with no retransmissions.
+   IEEE80211_RADIOTAP_F_TX_NOSEQ: frame will have its sequence number retained
+				  as handed to mac80211 by userspace, useful
+				  when injecting fragments of a single packet.
+
 The injection code can also skip all other currently defined radiotap fields
 facilitating replay of captured radiotap headers directly.
 
diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h
index 23c3f3d..210dbd9 100644
--- a/include/net/ieee80211_radiotap.h
+++ b/include/net/ieee80211_radiotap.h
@@ -240,6 +240,9 @@ enum ieee80211_radiotap_type {
 						 * retries */
 #define IEEE80211_RADIOTAP_F_TX_CTS	0x0002	/* used cts 'protection' */
 #define IEEE80211_RADIOTAP_F_TX_RTS	0x0004	/* used rts/cts handshake */
+#define IEEE80211_RADIOTAP_F_TX_NOACK	0x0008	/* frame should not be ACKed */
+#define IEEE80211_RADIOTAP_F_TX_NOSEQ	0x0010	/* sequence number handled
+						 * by userspace */
 
 /* Ugly macro to convert literal channel numbers into their mhz equivalents
  * There are certianly some conditions that will break this (like feeding it '30')
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3fb04a8..b47435d 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -650,6 +650,10 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 	u8 *qc;
 	int tid;
 
+	if (unlikely(!(info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ)))
+		return TX_CONTINUE;
+	info->flags &= ~IEEE80211_TX_CTL_ASSIGN_SEQ;
+
 	/*
 	 * Packet injection may want to control the sequence
 	 * number, if we have no matching interface then we
@@ -901,6 +905,7 @@ __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx,
 	struct ieee80211_radiotap_header *rthdr =
 		(struct ieee80211_radiotap_header *) skb->data;
 	struct ieee80211_supported_band *sband;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	int ret = ieee80211_radiotap_iterator_init(&iterator, rthdr, skb->len);
 
 	sband = tx->local->hw.wiphy->bands[tx->channel->band];
@@ -947,6 +952,13 @@ __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx,
 			if (*iterator.this_arg & IEEE80211_RADIOTAP_F_FRAG)
 				tx->flags |= IEEE80211_TX_FRAGMENTED;
 			break;
+		case IEEE80211_RADIOTAP_TX_FLAGS:
+			if (*iterator.this_arg & IEEE80211_RADIOTAP_F_TX_NOACK)
+				info->flags |= IEEE80211_TX_CTL_NO_ACK;
+			if (*iterator.this_arg & IEEE80211_RADIOTAP_F_TX_NOSEQ)
+				info->flags &= ~IEEE80211_TX_CTL_ASSIGN_SEQ;
+			break;
+
 
 		/*
 		 * Please update the file
@@ -999,6 +1011,8 @@ __ieee80211_tx_prepare(struct ieee80211_tx_data *tx,
 	 * it will be cleared/left by radiotap as desired.
 	 */
 	tx->flags |= IEEE80211_TX_FRAGMENTED;
+	/* Same here, controlled by radiotap and the stack */
+	info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
 
 	/* process and remove the injection radiotap header */
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1062,13 +1076,10 @@ __ieee80211_tx_prepare(struct ieee80211_tx_data *tx,
 			return TX_QUEUED;
 	}
 
-	if (is_multicast_ether_addr(hdr->addr1)) {
-		tx->flags &= ~IEEE80211_TX_UNICAST;
+	if (is_multicast_ether_addr(hdr->addr1))
 		info->flags |= IEEE80211_TX_CTL_NO_ACK;
-	} else {
+	else
 		tx->flags |= IEEE80211_TX_UNICAST;
-		info->flags &= ~IEEE80211_TX_CTL_NO_ACK;
-	}
 
 	if (tx->flags & IEEE80211_TX_FRAGMENTED) {
 		if ((tx->flags & IEEE80211_TX_UNICAST) &&

Re: [Proposal]TX flags

[David Young]

On Wed, Apr 15, 2009 at 02:33:05AM +0200, G?bor Stefanik wrote:
> I propose the following suggested Radiotap features to be standardised:
> 
> * A new, 16-bit wide "TX flags" field (field number 15), containing
> the following flags:
>    - TX failed due to excessive retries,
>    - CTS-to-self protection,
>    - RTS/CTS-handshake,
>    - Frame shall not be ACKed ("No-ACK"), and
>    - Sequence number shall not be recalculated ("No-Seq").

Thanks.

FWIW, these assignments are compatible with NetBSD.  They appear to be
compatible with FreeBSD, too.

> If no one objects to this proposal in 7 days, I will resubmit it as a
> formal RFA, to allow eventual adoption.

Great!

Dave

-- 
David Young             OJC Technologies
dyoung-eZodSLrBbDpBDgjK7y7TUQ@public.gmane.org      Urbana, IL * (217) 278-3933

Re: [Proposal]TX flags

[Johannes Berg]

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

On Wed, 2009-04-15 at 02:33 +0200, Gábor Stefanik wrote:

>    - Define bit 1 (mask 0x0002) as "CTS-to-self protection",
>    - Define bit 2 (mask 0x0004) as "RTS/CTS handshake",

Since you're defining these for injection too I think it would be useful
to also define a way to _not_ use RTS or CTS, or define a way to use the
default that the device would use from dot11RTSThreshold for example.

Otherwise, when leaving out these bits, it wouldn't be clear whether
that means "do not use, overriding default" or "use default".

johannes

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

Re: [Proposal]TX flags

[Gábor Stefanik]

2009/4/16 Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>:
> On Wed, 2009-04-15 at 02:33 +0200, Gábor Stefanik wrote:
>
>>    - Define bit 1 (mask 0x0002) as "CTS-to-self protection",
>>    - Define bit 2 (mask 0x0004) as "RTS/CTS handshake",
>
> Since you're defining these for injection too I think it would be useful
> to also define a way to _not_ use RTS or CTS, or define a way to use the
> default that the device would use from dot11RTSThreshold for example.
>
> Otherwise, when leaving out these bits, it wouldn't be clear whether
> that means "do not use, overriding default" or "use default".
>
> johannes
>

Here is how I think this can be done:
Because it's impossible to have a packet protected both by CTS-to-self
and by RTS/CTS, we can define the case where both the CTS and RTS
flags are set as "Automatically use RTS or CTS as needed". The same
should be true when the TX flags field is missing entirely. When the
TX flags field is present, and both flags are 0, that should be
interpreted as "don't protect the packet".

So, in summary, this is how things should be interpreted:

TX Flags absent: Use RTS & CTS as needed.
TX Flags present: {
RTS=0, CTS=0: Use neither RTS nor CTS.
RTS=0, CTS=1: Use CTS-to-self.
RTS=1, CTS=0: Use RTS/CTS-handshake.
RTS=1, CTS=1: Use RTS & CTS as needed.
}

Alternatively, the meanings of the {0,0} and {1,1} cases could be
switched around (making the {0,0} case more logical, at the expense of
the {1,1} one):

TX Flags absent: Use RTS & CTS as needed.
TX Flags present: {
RTS=0, CTS=0: Use RTS & CTS as needed.
RTS=0, CTS=1: Use CTS-to-self.
RTS=1, CTS=0: Use RTS/CTS-handshake.
RTS=1, CTS=1: Use neither RTS nor CTS.
}

(By reading the second proposal again, I find it more and more
sympathetic... but let the discussion decide.)

--Gábor

-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Proposal]TX flags

[Johannes Berg]

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

On Thu, 2009-04-16 at 20:47 +0200, Gábor Stefanik wrote:

> Alternatively, the meanings of the {0,0} and {1,1} cases could be
> switched around (making the {0,0} case more logical, at the expense of
> the {1,1} one):
> 
> TX Flags absent: Use RTS & CTS as needed.
> TX Flags present: {
> RTS=0, CTS=0: Use RTS & CTS as needed.
> RTS=0, CTS=1: Use CTS-to-self.
> RTS=1, CTS=0: Use RTS/CTS-handshake.
> RTS=1, CTS=1: Use neither RTS nor CTS.
> }
> 
> (By reading the second proposal again, I find it more and more
> sympathetic... but let the discussion decide.)

That _works_, but is impossible to describe in any feature discovery.

johannes

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

Re: [Proposal]TX flags

[Michael Buesch]

On Thursday 16 April 2009 20:59:34 Johannes Berg wrote:
> On Thu, 2009-04-16 at 20:47 +0200, Gábor Stefanik wrote:
> 
> > Alternatively, the meanings of the {0,0} and {1,1} cases could be
> > switched around (making the {0,0} case more logical, at the expense of
> > the {1,1} one):
> > 
> > TX Flags absent: Use RTS & CTS as needed.
> > TX Flags present: {
> > RTS=0, CTS=0: Use RTS & CTS as needed.
> > RTS=0, CTS=1: Use CTS-to-self.
> > RTS=1, CTS=0: Use RTS/CTS-handshake.
> > RTS=1, CTS=1: Use neither RTS nor CTS.

The first and the last thing let my head explode, because it's not what somebody
would expect from such bits. This kind of logic is also used in wext. And it's why I hate wext.
"bit0 means x, bit1 means y, buuuuuuuuuuuuuuuut iff both bits are set the whole logic
is inverted and whatever..."
That complicates _every_ single test of the bit (always need if (bit0 is set but not bit1))
It produces spaghetti code interpreting these bits with lots of branches and special
conditions that nobody does understand by reading the code alone.
If you can't encode your functionality into a boolean, do _NOT_ use bits to encode it.
Use integers to encode tristate or quadstate or whatever.
You essentially _did_ that already, if you look at your bits. You use the two individual bits
as 2bit integer value. So why not spell it out and use an integer field for that information?

-- 
Greetings, Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Proposal]TX flags

[David Young]

On Thu, Apr 16, 2009 at 09:10:04PM +0200, Michael Buesch wrote:
> On Thursday 16 April 2009 20:59:34 Johannes Berg wrote:
> > On Thu, 2009-04-16 at 20:47 +0200, G?bor Stefanik wrote:
> > 
> > > Alternatively, the meanings of the {0,0} and {1,1} cases could be
> > > switched around (making the {0,0} case more logical, at the expense of
> > > the {1,1} one):
> > > 
> > > TX Flags absent: Use RTS & CTS as needed.
> > > TX Flags present: {
> > > RTS=0, CTS=0: Use RTS & CTS as needed.
> > > RTS=0, CTS=1: Use CTS-to-self.
> > > RTS=1, CTS=0: Use RTS/CTS-handshake.
> > > RTS=1, CTS=1: Use neither RTS nor CTS.
> 
> The first and the last thing let my head explode, because it's not
> what somebody would expect from such bits. This kind of logic is also
> used in wext. And it's why I hate wext. "bit0 means x, bit1 means y,
> buuuuuuuuuuuuuuuut iff both bits are set the whole logic is inverted
> and whatever..."  That complicates _every_ single test of the bit
> (always need if (bit0 is set but not bit1)) It produces spaghetti code
> interpreting these bits with lots of branches and special conditions
> that nobody does understand by reading the code alone.  If you can't
> encode your functionality into a boolean, do _NOT_ use bits to encode
> it.  Use integers to encode tristate or quadstate or whatever.  You
> essentially _did_ that already, if you look at your bits. You use the
> two individual bits as 2bit integer value. So why not spell it out and
> use an integer field for that information?

G?bor,

I see the point that Michael is making.  What do you think?  Shall
we treat it as a 2-bit wide unsigned integer field in the Tx flags,
instead?

Dave

-- 
David Young             OJC Technologies
dyoung-eZodSLrBbDpBDgjK7y7TUQ@public.gmane.org      Urbana, IL * (217) 278-3933

Re: [Proposal]TX flags

[Gábor Stefanik]

On Thu, Apr 16, 2009 at 10:48 PM, David Young <dyoung-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Apr 16, 2009 at 09:10:04PM +0200, Michael Buesch wrote:
>> On Thursday 16 April 2009 20:59:34 Johannes Berg wrote:
>> > On Thu, 2009-04-16 at 20:47 +0200, G?bor Stefanik wrote:
>> >
>> > > Alternatively, the meanings of the {0,0} and {1,1} cases could be
>> > > switched around (making the {0,0} case more logical, at the expense of
>> > > the {1,1} one):
>> > >
>> > > TX Flags absent: Use RTS & CTS as needed.
>> > > TX Flags present: {
>> > > RTS=0, CTS=0: Use RTS & CTS as needed.
>> > > RTS=0, CTS=1: Use CTS-to-self.
>> > > RTS=1, CTS=0: Use RTS/CTS-handshake.
>> > > RTS=1, CTS=1: Use neither RTS nor CTS.
>>
>> The first and the last thing let my head explode, because it's not
>> what somebody would expect from such bits. This kind of logic is also
>> used in wext. And it's why I hate wext. "bit0 means x, bit1 means y,
>> buuuuuuuuuuuuuuuut iff both bits are set the whole logic is inverted
>> and whatever..."  That complicates _every_ single test of the bit
>> (always need if (bit0 is set but not bit1)) It produces spaghetti code
>> interpreting these bits with lots of branches and special conditions
>> that nobody does understand by reading the code alone.  If you can't
>> encode your functionality into a boolean, do _NOT_ use bits to encode
>> it.  Use integers to encode tristate or quadstate or whatever.  You
>> essentially _did_ that already, if you look at your bits. You use the
>> two individual bits as 2bit integer value. So why not spell it out and
>> use an integer field for that information?
>
> G?bor,
>
> I see the point that Michael is making.  What do you think?  Shall
> we treat it as a 2-bit wide unsigned integer field in the Tx flags,
> instead?
>

IMO that is a good idea, if we accept having non-booleans in a flags
field. In that case, this proposal comes to my mind:
-Define the second and third bits (mask 0x0006) as a quad-state flag
indicating the use of RTS/CTS. So, we can have these values for the
flag (accessible as (TXFlags & 0x0006) >> 1):
0: neither
1: rts
2: cts
3: auto-select (only makes sense when sending & during feature discovery).

Also, a proposal for feature discovery:
The userspace can send a packet that consists of nothing but a
"christmas tree" radiotap header, with no payload, but with all fields
and flags set that are known by the userspace app. The response could
be another payloadless radiotap header, copying the bits set by
userspace, but unsetting the ones not supported by the hardware. This
way, the response packet has all flags set that are supported by both
the sender and the userspace app.

-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Proposal]TX flags

[Johannes Berg]

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

On Fri, 2009-04-17 at 03:24 +0200, Gábor Stefanik wrote:

> > I see the point that Michael is making.  What do you think?  Shall
> > we treat it as a 2-bit wide unsigned integer field in the Tx flags,
> > instead?
> >
> 
> IMO that is a good idea, if we accept having non-booleans in a flags
> field. In that case, this proposal comes to my mind:
> -Define the second and third bits (mask 0x0006) as a quad-state flag
> indicating the use of RTS/CTS. So, we can have these values for the
> flag (accessible as (TXFlags & 0x0006) >> 1):
> 0: neither
> 1: rts
> 2: cts
> 3: auto-select (only makes sense when sending & during feature discovery)=

Re: [Proposal]TX flags

[David Young]

On Thu, Apr 16, 2009 at 08:59:34PM +0200, Johannes Berg wrote:
> On Thu, 2009-04-16 at 20:47 +0200, G?bor Stefanik wrote:
> 
> > Alternatively, the meanings of the {0,0} and {1,1} cases could be
> > switched around (making the {0,0} case more logical, at the expense of
> > the {1,1} one):
> > 
> > TX Flags absent: Use RTS & CTS as needed.
> > TX Flags present: {
> > RTS=0, CTS=0: Use RTS & CTS as needed.
> > RTS=0, CTS=1: Use CTS-to-self.
> > RTS=1, CTS=0: Use RTS/CTS-handshake.
> > RTS=1, CTS=1: Use neither RTS nor CTS.
> > }
> > 
> > (By reading the second proposal again, I find it more and more
> > sympathetic... but let the discussion decide.)
> 
> That _works_, but is impossible to describe in any feature discovery.

The discovery mechanism that we have begun to discuss would have a hard
time describing that feature at its current level of development, but
that is not the only feature that it will have a hard time describing.
Feature discovery may need more development before we measure new
proposals against it.  What do you think?

Dave

-- 
David Young             OJC Technologies
dyoung-eZodSLrBbDpBDgjK7y7TUQ@public.gmane.org      Urbana, IL * (217) 278-3933

Re: [Proposal]TX flags

[Johannes Berg]

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

On Thu, 2009-04-16 at 15:33 -0500, David Young wrote:

> > > TX Flags absent: Use RTS & CTS as needed.
> > > TX Flags present: {
> > > RTS=0, CTS=0: Use RTS & CTS as needed.
> > > RTS=0, CTS=1: Use CTS-to-self.
> > > RTS=1, CTS=0: Use RTS/CTS-handshake.
> > > RTS=1, CTS=1: Use neither RTS nor CTS.
> > > }
> > > 
> > > (By reading the second proposal again, I find it more and more
> > > sympathetic... but let the discussion decide.)
> > 
> > That _works_, but is impossible to describe in any feature discovery.
> 
> The discovery mechanism that we have begun to discuss would have a hard
> time describing that feature at its current level of development, but
> that is not the only feature that it will have a hard time describing.
> Feature discovery may need more development before we measure new
> proposals against it.  What do you think?

True, but why make the job harder than it is? Michael has a good point
too. And we have free bits, I don't see why we can't add one for "do
what I said" (wrt. rts/cts), which, if unset means to do be in automatic
mode and ignore the other bits.

Or we really do use a two-bit field with values;
	0 automatic
	1 neither
	2 rts
	3 cts
or something.

johannes

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