LinuxLists.cc - atheros hardware needs padding for QoS data

2007-10-12 10:46:33

Subject: atheros hardware needs padding for QoS data

hello!

it seems the atheros hardware needs a padding to 4 byte boundaries after the
801.11 header when it sends QoS frames and it will add such a padding
automatically when it receives QoS data frames.

in my case this made the ath5k driver associate with an QoS enabled AP (same
type of card with the madwifi driver in default config) fine, but i could not
get any ARP requests thru - since the padding was missing there were 2 bytes
lost in the packet received on the AP side, so no further communication...

this need for padding is also reflected in the madwifi code and this comment:
/*
* Indicate we need the 802.11 header padded to a
* 32-bit boundary for 4-address and QoS frames.
*/

i created the following simple hack, to see if the problem really is the
missing padding. it just adds the padding all the time, so it will only work
with QoS enabled APs, but it makes me able to communicate with my QoS AP.

i didn't see how mac80211 would allow to solve this elegantly at this time.
are there any other drivers with a similar kind of padding requirement? how
could that be implemented cleanly?

luis, does this hack fix your problem with DHCP too?

bruno

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 064924c..bb11e63 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1019,6 +1019,8 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
return TXRX_DROP;

hdrlen = ieee80211_get_hdrlen(fc);
+ if ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA)
+ hdrlen = hdrlen + 2;

/* convert IEEE 802.11 header + possible LLC headers into Ethernet
* header
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 957ec3c..6be9b95 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1405,7 +1405,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
if (sta) {
if (sta->flags & WLAN_STA_WME) {
fc |= IEEE80211_STYPE_QOS_DATA;
- hdrlen += 2;
+ hdrlen += 4; //2;
}
sta_info_put(sta);
}

2007-10-15 07:34:27

by Bruno Randolf

[permalink] [raw]

Subject: [PATCH] Re: atheros hardware needs padding for QoS data

what do you think about the following solution?

Atheros hardware requires the header to be padded to 4 byte (32 bit)
boundaries. It expects the payload data to start at multiples of 4 byte for TX
frames and will add the padding for received frames. For most headers there is
no need for padding, since they are multiples of 4 already - except QoS and 4
address headers.

This adds a new hw flag IEEE80211_HW_HEADER_PADDING to tell mac80211 that such
a padding is needed.
---
drivers/net/wireless/ath5k/base.c | 1 +
include/net/mac80211.h | 8 ++++++++
net/mac80211/rx.c | 20 ++++++++++++++++++++
net/mac80211/tx.c | 5 +++++
4 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c
b/drivers/net/wireless/ath5k/base.c
index 08530d5..b705f68 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -2382,6 +2382,7 @@ static int __devinit ath_pci_probe(struct pci_dev *pdev,
hw->extra_tx_headroom = 2;
hw->channel_change_time = 5000;
hw->max_rssi = 127; /* FIXME: get a real value for this. */
+ hw->flags |= IEEE80211_HW_HEADER_PADDING;
sc = hw->priv;
sc->hw = hw;
sc->pdev = pdev;
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2b1bffb..18c30e6 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -662,12 +662,20 @@ enum sta_notify_cmd {
* @IEEE80211_HW_DEFAULT_REG_DOMAIN_CONFIGURED:
* Channels are already configured to the default regulatory domain
* specified in the device's EEPROM
+ *
+ * @IEEE80211_HW_HEADER_PADDING:
+ * Some hardware requires the header to be padded to 4 byte (32 bit)
+ * boundaries. It expects the payload data to start at multiples
+ * of 4 byte for TX frames and will add the padding for received
+ * frames. For most headers there is no need for padding, since they
+ * are multiples of 4 already - except QoS and 4 address headers.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_HOST_GEN_BEACON_TEMPLATE = 1<<0,
IEEE80211_HW_RX_INCLUDES_FCS = 1<<1,
IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING = 1<<2,
IEEE80211_HW_DEFAULT_REG_DOMAIN_CONFIGURED = 1<<3,
+ IEEE80211_HW_HEADER_PADDING = 1<<4,
};

/**
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 064924c..2ad94d1 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -419,6 +419,25 @@ ieee80211_rx_h_check(struct ieee80211_txrx_data *rx)

static ieee80211_txrx_result
+ieee80211_rx_h_remove_padding(struct ieee80211_txrx_data *rx)
+{
+ struct ieee80211_hw *hw = local_to_hw(rx->local);
+ int hdrlen;
+ int pad;
+
+ if (!(hw->flags & IEEE80211_HW_HEADER_PADDING))
+ return TXRX_CONTINUE;
+
+ hdrlen = ieee80211_get_hdrlen(rx->fc);
+ if ((pad = hdrlen % 4)) {
+ memmove(rx->skb->data + pad, rx->skb->data, hdrlen);
+ skb_pull(rx->skb, pad);
+ }
+
+ return TXRX_CONTINUE;
+}
+
+static ieee80211_txrx_result
ieee80211_rx_h_decrypt(struct ieee80211_txrx_data *rx)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
@@ -1320,6 +1339,7 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
ieee80211_rx_h_if_stats,
ieee80211_rx_h_passive_scan,
ieee80211_rx_h_check,
+ ieee80211_rx_h_remove_padding,
ieee80211_rx_h_decrypt,
ieee80211_rx_h_sta_process,
ieee80211_rx_h_defragment,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 957ec3c..ce4c6ce 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1334,6 +1334,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+ struct ieee80211_hw *hw = local_to_hw(local);
struct ieee80211_tx_packet_data *pkt_data;
struct ieee80211_sub_if_data *sdata;
int ret = 1, head_need;
@@ -1410,6 +1411,10 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
sta_info_put(sta);
}

+ if (hw->flags & IEEE80211_HW_HEADER_PADDING) {
+ hdrlen = roundup(hdrlen, 4);
+ }
+
hdr.frame_control = cpu_to_le16(fc);
hdr.duration_id = 0;
hdr.seq_ctrl = 0;
--
1.5.3.4

2007-10-15 08:31:32

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCH] Re: atheros hardware needs padding for QoS data

On Mon, 2007-10-15 at 16:34 +0900, bruno randolf wrote:
> what do you think about the following solution?

I don't like it. If we wanted to implement this in mac80211 we should do
a zero-copy solution with pointers to header/data portion which is very
hard right now because mac80211 has much assumptions that it's all
contiguous. Secondly, this adds overhead for all hardware to mac80211
for something that can be *trivially* solved in the driver, and only a
single driver so far requires it. I think the driver is a *much* better
place to do this.

> Atheros hardware requires the header to be padded to 4 byte (32 bit)
> boundaries. It expects the payload data to start at multiples of 4 byte for TX
> frames and will add the padding for received frames. For most headers there is
> no need for padding, since they are multiples of 4 already - except QoS and 4
> address headers.
>
> This adds a new hw flag IEEE80211_HW_HEADER_PADDING to tell mac80211 that such
> a padding is needed.

[patch snipped]

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-10-15 12:30:37

by Bruno Randolf

On Monday 15 October 2007 17:47:16 Johannes Berg wrote:
> > another solution i thought of was signalling the mac80211 layer that we
> > need padding which could then just adjust it's headerlen. but then
> > different drivers might need different padding in different places (i
> > don't know?). what do you think of this approach?
>
> That could be worthwhile, though the headerlen calculation is called
> very very often and adding another branch into it could very well impact
> performance worse than doing the memmove.

now you confuse me. this is exactly what i attempted with the previous patch,
which you didn't like...

unfortunately it's not possible to implement it there as a zero copy solution,
because ieee80211_rx_h_remove_qos_control messes with the header itself.

bruno

2007-10-15 03:41:13

by Bruno Randolf

[permalink] [raw]

Subject: Re: atheros hardware needs padding for QoS data

On Friday 12 October 2007 22:09:38 Johannes Berg wrote:
> On Fri, 2007-10-12 at 13:58 +0200, Johannes Berg wrote:
> > b43 also has padding added on RX which we simply remove with a memmove()
>
> Umm, no, this is wrong, the padding is in another place.

nevermind. i thought of this as an option as well.

but isn't doing a memmove() quite inefficient? especially since we are dealing
with QoS packets that might be important.

another solution i thought of was signalling the mac80211 layer that we need
padding which could then just adjust it's headerlen. but then different
drivers might need different padding in different places (i don't know?).
what do you think of this approach?

bruno