On Sunday 18 March 2007 06:15, andy@xxxxxxxxxxx wrote:
> From: Andy Green <andy@xxxxxxxxxxx>
>
> Try #3
> - moved to Michael Wu's method of tracking if we came in on a
> monitor interface by using ifindex
> - removed older proposed monitor interface tracking method and flags
> - style fixes
> - removed duped #include that is present in Michael Wu's patch already
>
> Try #2
> - took Michael Wu's advice about better tools and basing on wireless-dev
> - took Luis Rodriguez's advice about coding style makeover
> - took Pavel Roskin's advice about little-endian radiotap
>
I've mostly made comments about style issues. There are only comments on the
first instance of any style problem so please check the rest of the code for
the same problems.
> Index: linux-2.6.20.i386/include/net/mac80211.h
> ===================================================
>
> diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
> index fb33b90..21e8990 100644
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> @@ -1054,7 +1054,163 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data
> *tx) }
>
>
> -static void inline
> +/* deal with packet injection down monitor interface
> + * with Radiotap Header -- only called for monitor mode interface
> + */
> +
> +static ieee80211_txrx_result
> +__ieee80211_convert_radiotap_to_control_and_remove(
> + struct ieee80211_txrx_data *tx,
> + struct sk_buff *skb, struct ieee80211_tx_control *control)
> +{
> + /* this is the moment to interpret the radiotap header that
> + * must be at the start of the packet injected in Monitor
> + * mode into control and then discard the radiotap header
> + *
> + * Need to take some care with endian-ness since radiotap
> + * is apparently a little-endian struct, including the args
> + */
Line this up. :)
> +
> + struct ieee80211_radiotap_header *rthdr =
> + (struct ieee80211_radiotap_header *) skb->data;
> +
> + /* small length lookup table for all radiotap types we heard of
> + * starting from b0 in the bitmap, so we can walk the payload
> + * area of the radiotap header
> + */
> +
> + static const u8 radiotap_entry_sizes[] = {
> + 8, /* IEEE80211_RADIOTAP_TSFT */
> + 1, /* IEEE80211_RADIOTAP_FLAGS */
> + 1, /* IEEE80211_RADIOTAP_RATE */
> + 4, /* IEEE80211_RADIOTAP_CHANNEL */
> + 2, /* IEEE80211_RADIOTAP_FHSS */
> + 1, /* IEEE80211_RADIOTAP_DBM_ANTSIGNAL */
> + 1, /* IEEE80211_RADIOTAP_DBM_ANTNOISE */
> + 2, /* IEEE80211_RADIOTAP_LOCK_QUALITY */
> + 2, /* IEEE80211_RADIOTAP_TX_ATTENUATION */
> + 2, /* IEEE80211_RADIOTAP_DB_TX_ATTENUATION */
> + 1, /* IEEE80211_RADIOTAP_DBM_TX_POWER */
> + 1, /* IEEE80211_RADIOTAP_ANTENNA */
> + 1, /* IEEE80211_RADIOTAP_DB_ANTSIGNAL */
> + 1 /* IEEE80211_RADIOTAP_DB_ANTNOISE */
> + /* add more here as they are defined */
> + };
Hm, this could be integrated into the switch statement below, but it doesn't
really matter much.
Have you looked into padding issues with radiotap headers? For example, if
there is a 1 byte field which is then followed by a 4 byte field, there needs
to be 3 bytes of padding after the first field, but if the field after were 2
bytes long, the padding would only be 1 byte (according to my understanding
of the radiotap specs).
> + int tap_index = 0;
> + u8 *tap_arg = skb->data + sizeof(struct ieee80211_radiotap_header);
> + u32 *curr_arg_bitmap = &rthdr->it_present;
> + u32 arg_bitmap = le32_to_cpu(*curr_arg_bitmap);
> +
> + if (rthdr->it_version) return TXRX_DROP; /* version byte as magic */
Put the return on the next line.
> +
> + /* sanity check for skb length and radiotap length field */
> + if (skb->len < (le16_to_cpu(rthdr->it_len) +
> + sizeof(struct ieee80211_hdr)))
> + return TXRX_DROP;
> +
> + /* find payload start allowing for extended bitmap(s) */
> +
> + if (le32_to_cpu(rthdr->it_present) & 0x80000000) {
> + while( le32_to_cpu(*((u32 *)tap_arg)) & 0x80000000)
Space after while, remove the space before le32_to_cpu.
> + tap_arg += sizeof(u32);
> + tap_arg += sizeof(u32);
> + }
> +
> + /* default control situation for all injected packets */
> +
> + control->retry_limit = 1; /* no retry */
> + control->key_idx = -1; /* no encryption key */
> + control->flags &= ~(IEEE80211_TXCTL_USE_RTS_CTS |
> + IEEE80211_TXCTL_USE_CTS_PROTECT);
> + control->flags |= (IEEE80211_TXCTL_DO_NOT_ENCRYPT |
> + IEEE80211_TXCTL_NO_ACK);
> + control->antenna_sel_tx = 0; /* default to default/diversity */
> +
> + /* for every radiotap entry we can at
> + * least skip (by knowing the length)...
> + */
> +
> + while (tap_index < sizeof(radiotap_entry_sizes)) {
> + int i, target_rate;
> +
> + if (!(arg_bitmap & 1)) goto next_entry;
> +
> + /* arg is present */
> +
> + switch(tap_index) { /* deal with if interested */
Space after switch.
> +
> + case IEEE80211_RADIOTAP_RATE:
> + /* radiotap "rate" u8 is in
> + * 500kbps units, eg, 0x02=1Mbps
> + * ieee80211 "rate" int is
> + * in 100kbps units, eg, 0x0a=1Mbps
> + */
> + target_rate = (*tap_arg) * 5;
> + for (i = 0; i < tx->local->num_curr_rates; i++) {
> + struct ieee80211_rate *r =
> + &tx->local->curr_rates[i];
> +
> + if (r->rate <= target_rate) {
> + if (r->flags &
> + IEEE80211_RATE_PREAMBLE2) {
> + control->tx_rate = r->val2;
> + } else {
> + control->tx_rate = r->val;
> + }
> +
> +
> + /* end on exact match */
Too much indenting on comments again.
> + if (r->rate == target_rate)
> + i = tx->local->num_curr_rates;
> + }
> + }
> + break;
> +
> + case IEEE80211_RADIOTAP_ANTENNA:
> + /* radiotap uses 0 for 1st ant,
> + * mac80211 is 1 for 1st ant
> + * absence of IEEE80211_RADIOTAP_ANTENNA
> + * gives default/diversity
> + */
> + control->antenna_sel_tx = (*tap_arg) + 1;
> + break;
> +
> + case IEEE80211_RADIOTAP_DBM_TX_POWER:
> + control->power_level = *tap_arg;
> + break;
> +
> + default:
> + break;
> + }
> +
> + tap_arg += radiotap_entry_sizes[tap_index];
> +
> + next_entry:
This label is indented too much.
> +
> + tap_index++;
> + if (unlikely((tap_index & 31) == 0)) {
> + /* completed current u32 bitmap */
> + if (arg_bitmap & 1) { /* b31 was set, there is more */
> + /* move to next u32 bitmap */
> + curr_arg_bitmap++;
> + arg_bitmap = le32_to_cpu(*curr_arg_bitmap);
> + } else {
> + /* he didn't give us any more bitmaps: end */
I suppose this is an accurate assumption 99% of the time, but I think being
gender neutral sounds better.
> @@ -1062,6 +1218,9 @@ __ieee80211_tx_prepare(struct ieee80211_txrx_data
> *tx, {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
> + struct ieee80211_sub_if_data *sdata;
> + ieee80211_txrx_result res=TXRX_CONTINUE;
Space before and after the =.
> @@ -1071,7 +1230,32 @@ __ieee80211_tx_prepare(struct ieee80211_txrx_data
> *tx, tx->sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> tx->sta = sta_info_get(local, hdr->addr1);
> tx->fc = le16_to_cpu(hdr->frame_control);
> +
> + /* set defaults for things that can be set by
> + * injected radiotap headers
> + */
> +
> control->power_level = local->hw.conf.power_level;
> + control->antenna_sel_tx = local->hw.conf.antenna_sel_tx;
> + if (local->sta_antenna_sel != STA_ANTENNA_SEL_AUTO && tx->sta )
Kill the space before the parenthesis.
> @@ -1215,14 +1397,24 @@ static int ieee80211_tx(struct net_device *dev,
> struct sk_buff *skb, return 0;
> }
>
> - __ieee80211_tx_prepare(&tx, skb, dev, control);
> + res_prepare=__ieee80211_tx_prepare(&tx, skb, dev, control);
> +
> + if (res_prepare==TXRX_DROP) {
Space before and after the ==.
> + dev_kfree_skb(skb);
> + return 0;
> + }
> +
> sta = tx.sta;
> tx.u.tx.mgmt_interface = mgmt;
>
> - for (handler = local->tx_handlers; *handler != NULL; handler++) {
> - res = (*handler)(&tx);
> - if (res != TXRX_CONTINUE)
> - break;
> + if (res_prepare==TXRX_QUEUED) { /* if it was an injected packet */
> + res=TXRX_CONTINUE;
> + } else {
> + for (handler = local->tx_handlers; *handler != NULL; handler++) {
> + res = (*handler)(&tx);
> + if (res != TXRX_CONTINUE)
> + break;
> + }
> }
>
> skb = tx.skb; /* handlers are allowed to change skb */
> @@ -1456,6 +1648,52 @@ static int ieee80211_subif_start_xmit(struct sk_buff
> *skb, goto fail;
> }
>
> + if (unlikely(sdata->type==IEEE80211_IF_TYPE_MNTR)) {
> + struct ieee80211_radiotap_header * prthdr=
Space after prthdr.
We could avoid this check altogether by spinning this code into a different
function and setting the xmit handler appropriately depending on if we're
initializing/switching to a monitor interface or not. Not entirely sure if
it's worth it, but I thought I'd mention it.
Thanks,
-Michael Wu
Attachment:
pgpPVEnzehYvm.pgp
Description: PGP signature
