Re: mac80211 AP mode powersaving problems?

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1.1: Type: text/plain, Size: 2552 bytes --]

Hey,

Good news. I figured out the DTIM thing and the multicast frame queue
stuff. For multicast frames, you simply send them down queue four and
they are dequeued and sent by the ucode after the DTIM beacon.

> But that's not all, also multicast frames (including ARP broadcast)
> are not buffered correctly. They are sent randomly instead of directly
> after a DTIM beacon. Also Multicast bit is not set and DTIM is always
> zero even though DTIM period is two.

Actually, they were sent right away because we enqueued them to queue
zero, not queue four.

Please try the attached patches. I haven't implemented the set_tim()
callback yet, but the device should now wake up for multicast traffic,
not for unicast yet. The patches work for me on a heavily patched kernel
('everything' plus http://johannes.sipsolutions.net/patches/kernel/all/)

I'll update the specs to indicate that
 (1) the tim offset shm thing needs to be set as well as the dtim period
 (2) queue four must be used for multicast frames [in client mode we
     never send multicast frames so doing it unconditionally is fine]
 (3) updating the beacon on every beacon interrupt isn't necessary, it
     only needs to be updated if it has changed (it works fine even if
     you update it, it's just not necessary)
 (4) the TIM is software controlled and should be set by the driver with
     help from the beacon interrupt.
 (5) the 0xa8 (==0x54*2) shm offset needs to be described, it's the
     "last multicast frame" for clearing the more-data bit and 0xffff is
     a special value that causes the microcode to clear the more-data
     bit on all multicast frames (odd, and I don't know why that would
     be useful, probably something with IBSS when we're not beaconing)

Due to the last point, I'm not entirely sure these patches are correct.
And I haven't bothered about PIO, do we even have access to that queue
there?

As for the whole beacon handling... I suggest that we change mac80211 to
give the beacon head and beacon tail to the driver like it gets it from
hostapd. Thing is, we first go to all the effort to build a valid beacon
frame, but then the driver gets to parse it all out again. That's dumb.
Hardware that has beaconing offload and implements the set_tim()
callback needs to know the TIM position, so it's probably better off
knowing the beacon head and tail. Drivers that call get_beacon() don't
care about conf->beacon except that they need to free it. Not obvious,
and many are probably buggy.

johannes

[-- Attachment #1.2: 055-b43-timb-position.patch --]
[-- Type: text/x-vhdl, Size: 1730 bytes --]

---
 drivers/net/wireless/b43/main.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

--- wireless-dev.orig/drivers/net/wireless/b43/main.c	2007-08-29 01:43:44.404618695 +0200
+++ wireless-dev/drivers/net/wireless/b43/main.c	2007-08-29 01:43:46.954618695 +0200
@@ -1180,6 +1180,7 @@ static void b43_write_beacon_template(st
 	len = min((size_t) dev->cached_beacon->len,
 		  0x200 - sizeof(struct b43_plcp_hdr6));
 	data = (const u8 *)(dev->cached_beacon->data);
+	printk(KERN_DEBUG "writing beacon\n");
 	b43_write_template_common(dev, data,
 				  len, ram_offset, shm_size_offset, rate);
 }
@@ -1242,6 +1243,13 @@ static u8 *b43_generate_probe_resp(struc
 			memcpy(dest_data + dest_pos, src_data + src_pos,
 			       elem_size);
 			dest_pos += elem_size;
+		} else {
+			printk(KERN_DEBUG "TIM is at 0x%x\n", dest_pos);
+			printk(KERN_DEBUG "DTIM period is %d\n", src_data[src_pos + 3]);
+			b43_shm_write16(dev, B43_SHM_SHARED,
+					B43_SHM_SH_TIMBPOS, dest_pos + 6 /* PLCP */);
+			b43_shm_write16(dev, B43_SHM_SHARED,
+					B43_SHM_SH_DTIMPER, src_data[src_pos + 3]);
 		}
 	}
 	*dest_size = dest_pos;
@@ -1371,12 +1379,12 @@ static void handle_irq_beacon(struct b43
 		return;
 	}
 	if (!(status & 0x1)) {
-		b43_write_beacon_template(dev, 0x68, 0x18, B43_CCK_RATE_1MB);
+//		b43_write_beacon_template(dev, 0x68, 0x18, B43_CCK_RATE_1MB);
 		status |= 0x1;
 		b43_write32(dev, B43_MMIO_STATUS2_BITFIELD, status);
 	}
 	if (!(status & 0x2)) {
-		b43_write_beacon_template(dev, 0x468, 0x1A, B43_CCK_RATE_1MB);
+//		b43_write_beacon_template(dev, 0x468, 0x1A, B43_CCK_RATE_1MB);
 		status |= 0x2;
 		b43_write32(dev, B43_MMIO_STATUS2_BITFIELD, status);
 	}

[-- Attachment #1.3: 057-b43-mcast.patch --]
[-- Type: text/x-vhdl, Size: 2307 bytes --]

---
 drivers/net/wireless/b43/dma.c  |   19 ++++++++++++++++---
 drivers/net/wireless/b43/xmit.c |   10 ++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

--- wireless-dev.orig/drivers/net/wireless/b43/dma.c	2007-08-29 02:22:29.084618695 +0200
+++ wireless-dev/drivers/net/wireless/b43/dma.c	2007-08-29 02:45:38.574618695 +0200
@@ -1038,6 +1038,8 @@ static u16 generate_cookie(struct b43_dm
 	 * in the lower 12 bits.
 	 * Note that the cookie must never be 0, as this
 	 * is a special value used in RX path.
+	 * It can also not be 0xFFFF because that is special
+	 * for multicast frames.
 	 */
 	switch (ring->index) {
 	case 0:
@@ -1056,7 +1058,7 @@ static u16 generate_cookie(struct b43_dm
 		cookie = 0xE000;
 		break;
 	case 5:
-		cookie = 0xF000;
+		cookie = 0x9000;
 		break;
 	}
 	B43_WARN_ON(slot & ~0x0FFF);
@@ -1088,7 +1090,7 @@ struct b43_dmaring *parse_cookie(struct 
 	case 0xE000:
 		ring = dma->tx_ring4;
 		break;
-	case 0xF000:
+	case 0x9000:
 		ring = dma->tx_ring5;
 		break;
 	default:
@@ -1205,7 +1207,18 @@ int b43_dma_tx(struct b43_wldev *dev,
 	int err = 0;
 	unsigned long flags;
 
-	ring = priority_to_txring(dev, ctl->queue);
+	if (skb->len < 5) {
+		printk(KERN_ERR "really short frame\n");
+		return -EIO;
+	}
+
+	if (skb->data[4] & 1) {
+		ring = dev->dma.tx_ring4;
+		/* set more data bit, ucode will clear on the last frame */
+		skb->data[1] |= 0x20;
+	} else {
+		ring = priority_to_txring(dev, ctl->queue);
+	}
 	spin_lock_irqsave(&ring->lock, flags);
 	B43_WARN_ON(!ring->tx);
 	if (unlikely(free_slots(ring) < SLOTS_PER_PACKET)) {
--- wireless-dev.orig/drivers/net/wireless/b43/xmit.c	2007-08-29 02:39:53.884618695 +0200
+++ wireless-dev/drivers/net/wireless/b43/xmit.c	2007-08-29 02:47:34.304618695 +0200
@@ -195,6 +195,16 @@ static void generate_txhdr_fw4(struct b4
 	u16 phy_ctl = 0;
 	u8 extra_ft = 0;
 
+	/*
+	 * give microcode the cookie of the last frame in the multicast
+	 * queue so it can clear the more-data bit in it
+	 */
+	/* FIXME: should really check the queue or something */
+	if ((cookie & 0xF000) == 0x9000) {
+		/* FIXME: locking */
+		b43_shm_write16(dev, B43_SHM_SHARED, 0x54*2, cookie);
+	}
+
 	memset(txhdr, 0, sizeof(*txhdr));
 
 	rate = txctl->tx_rate;

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