[PATCH] b43: do not stack-allocate pio rx/tx header buffers

[PATCH] b43: do not stack-allocate pio rx/tx header buffers

[Albert Herranz]

The DMA-API debugging facility complains about b43 mapping memory from
stack for SDIO-based cards, as can be seen in the following two
stack traces.

  Call Trace:
  [d2ef7c60] [c01c55f0] check_for_stack+0xf4/0x10c (unreliable)
  [d2ef7c80] [c01c6d10] debug_dma_map_sg+0xc4/0x134
  [d2ef7cb0] [c0281e10] sdhci_prepare_data+0x200/0x590
  [d2ef7ce0] [c0282288] sdhci_send_command+0xe8/0x320
  [d2ef7d00] [c02825e0] sdhci_request+0x120/0x1b4
  [d2ef7d20] [c0277ad4] mmc_wait_for_req+0x124/0x140
  [d2ef7d50] [c027c2f0] mmc_io_rw_extended+0x188/0x214
  [d2ef7e10] [c027d7c4] sdio_io_rw_ext_helper+0x104/0x224
  [d2ef7e40] [c0296000] ssb_sdio_block_read+0xac/0x110
  [d2ef7e60] [c0237f98] pio_rx_frame+0x1d0/0x43c
  [d2ef7eb0] [c0238244] b43_pio_rx+0x40/0x90
  [d2ef7ed0] [c0226124] b43_do_interrupt_thread+0x108/0x444
  [d2ef7f30] [c0226504] b43_sdio_interrupt_handler+0x58/0x74
  [d2ef7f40] [c0239110] b43_sdio_interrupt_dispatcher+0x4c/0x68
  [d2ef7f60] [c027e1ac] sdio_irq_thread+0xf8/0x284
  [d2ef7fb0] [c0054538] kthread+0x78/0x7c
  [d2ef7ff0] [c00124ec] kernel_thread+0x4c/0x68

  Call Trace:
  [d3335c20] [c01c55f0] check_for_stack+0xf4/0x10c (unreliable)
  [d3335c40] [c01c6d10] debug_dma_map_sg+0xc4/0x134
  [d3335c70] [c0281e14] sdhci_prepare_data+0x200/0x590
  [d3335ca0] [c028228c] sdhci_send_command+0xe8/0x320
  [d3335cc0] [c02825e4] sdhci_request+0x120/0x1b4
  [d3335ce0] [c0277ad8] mmc_wait_for_req+0x124/0x140
  [d3335d10] [c027c2f4] mmc_io_rw_extended+0x188/0x214
  [d3335dd0] [c027d86c] sdio_io_rw_ext_helper+0x1a8/0x224
  [d3335e00] [c0295ef4] ssb_sdio_block_write+0xac/0x110
  [d3335e20] [c0237a18] tx_write_4byte_queue+0x6c/0x138
  [d3335e40] [c0238610] pio_tx_frame+0x1c4/0x224
  [d3335ee0] [c0238718] b43_pio_tx+0xa8/0x1f4
  [d3335f00] [c0220380] b43_tx_work+0x4c/0xe0
  [d3335f20] [c004fa74] run_workqueue+0x120/0x1a4
  [d3335f70] [c004fb44] worker_thread+0x4c/0xb0
  [d3335fb0] [c0054538] kthread+0x78/0x7c
  [d3335ff0] [c00124ec] kernel_thread+0x4c/0x68

Indeed, b43 currently allocates the PIO RX and TX header buffers
from stack. The solution here is to use heap-allocated buffers instead.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 drivers/net/wireless/b43/b43.h |    9 ++++++++
 drivers/net/wireless/b43/pio.c |   42 +++++++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index fa1549a..b09dda1 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -550,6 +550,9 @@ struct b43_dma {
 struct b43_pio_txqueue;
 struct b43_pio_rxqueue;
 
+struct b43_rxhdr_fw4;
+struct b43_txhdr;
+
 /* Data structures for PIO transmission, per 80211 core. */
 struct b43_pio {
 	struct b43_pio_txqueue *tx_queue_AC_BK; /* Background */
@@ -559,6 +562,12 @@ struct b43_pio {
 	struct b43_pio_txqueue *tx_queue_mcast; /* Multicast */
 
 	struct b43_pio_rxqueue *rx_queue;
+
+	/*
+	 * RX/TX header buffers used by the frame transmit functions.
+	 */
+	struct b43_rxhdr_fw4 *rxhdr;
+	struct b43_txhdr *txhdr;
 };
 
 /* Context information for a noise calculation (Link Quality). */
diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
index e96091b..45aa6e2 100644
--- a/drivers/net/wireless/b43/pio.c
+++ b/drivers/net/wireless/b43/pio.c
@@ -241,6 +241,9 @@ void b43_pio_free(struct b43_wldev *dev)
 	destroy_queue_tx(pio, tx_queue_AC_VI);
 	destroy_queue_tx(pio, tx_queue_AC_BE);
 	destroy_queue_tx(pio, tx_queue_AC_BK);
+
+	kfree(pio->rxhdr);
+	kfree(pio->txhdr);
 }
 
 int b43_pio_init(struct b43_wldev *dev)
@@ -276,11 +279,23 @@ int b43_pio_init(struct b43_wldev *dev)
 	if (!pio->rx_queue)
 		goto err_destroy_mcast;
 
+	pio->rxhdr = kzalloc(sizeof(*pio->rxhdr), GFP_KERNEL);
+	if (!pio->rxhdr)
+		goto err_destroy_rx;
+
+	pio->txhdr = kzalloc(sizeof(*pio->txhdr), GFP_KERNEL);
+	if (!pio->txhdr)
+		goto err_destroy_rxhdr;
+
 	b43dbg(dev->wl, "PIO initialized\n");
 	err = 0;
 out:
 	return err;
 
+err_destroy_rxhdr:
+	kfree(pio->rxhdr);
+err_destroy_rx:
+	destroy_queue_rx(pio, rx_queue);
 err_destroy_mcast:
 	destroy_queue_tx(pio, tx_queue_mcast);
 err_destroy_vo:
@@ -435,8 +450,9 @@ static void pio_tx_frame_4byte_queue(struct b43_pio_txpacket *pack,
 static int pio_tx_frame(struct b43_pio_txqueue *q,
 			struct sk_buff *skb)
 {
+	struct b43_wldev *dev = q->dev;
+	struct b43_pio *pio = &dev->pio;
 	struct b43_pio_txpacket *pack;
-	struct b43_txhdr txhdr;
 	u16 cookie;
 	int err;
 	unsigned int hdrlen;
@@ -447,8 +463,8 @@ static int pio_tx_frame(struct b43_pio_txqueue *q,
 			  struct b43_pio_txpacket, list);
 
 	cookie = generate_cookie(q, pack);
-	hdrlen = b43_txhdr_size(q->dev);
-	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb,
+	hdrlen = b43_txhdr_size(dev);
+	err = b43_generate_txhdr(dev, (u8 *)pio->txhdr, skb,
 				 info, cookie);
 	if (err)
 		return err;
@@ -456,15 +472,15 @@ static int pio_tx_frame(struct b43_pio_txqueue *q,
 	if (info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) {
 		/* Tell the firmware about the cookie of the last
 		 * mcast frame, so it can clear the more-data bit in it. */
-		b43_shm_write16(q->dev, B43_SHM_SHARED,
+		b43_shm_write16(dev, B43_SHM_SHARED,
 				B43_SHM_SH_MCASTCOOKIE, cookie);
 	}
 
 	pack->skb = skb;
 	if (q->rev >= 8)
-		pio_tx_frame_4byte_queue(pack, (const u8 *)&txhdr, hdrlen);
+		pio_tx_frame_4byte_queue(pack, (const u8 *)pio->txhdr, hdrlen);
 	else
-		pio_tx_frame_2byte_queue(pack, (const u8 *)&txhdr, hdrlen);
+		pio_tx_frame_2byte_queue(pack, (const u8 *)pio->txhdr, hdrlen);
 
 	/* Remove it from the list of available packet slots.
 	 * It will be put back when we receive the status report. */
@@ -604,14 +620,14 @@ void b43_pio_get_tx_stats(struct b43_wldev *dev,
 static bool pio_rx_frame(struct b43_pio_rxqueue *q)
 {
 	struct b43_wldev *dev = q->dev;
-	struct b43_rxhdr_fw4 rxhdr;
+	struct b43_pio *pio = &dev->pio;
 	u16 len;
 	u32 macstat;
 	unsigned int i, padding;
 	struct sk_buff *skb;
 	const char *err_msg = NULL;
 
-	memset(&rxhdr, 0, sizeof(rxhdr));
+	memset(pio->rxhdr, 0, sizeof(*pio->rxhdr));
 
 	/* Check if we have data and wait for it to get ready. */
 	if (q->rev >= 8) {
@@ -649,16 +665,16 @@ data_ready:
 
 	/* Get the preamble (RX header) */
 	if (q->rev >= 8) {
-		ssb_block_read(dev->dev, &rxhdr, sizeof(rxhdr),
+		ssb_block_read(dev->dev, pio->rxhdr, sizeof(*pio->rxhdr),
 			       q->mmio_base + B43_PIO8_RXDATA,
 			       sizeof(u32));
 	} else {
-		ssb_block_read(dev->dev, &rxhdr, sizeof(rxhdr),
+		ssb_block_read(dev->dev, pio->rxhdr, sizeof(*pio->rxhdr),
 			       q->mmio_base + B43_PIO_RXDATA,
 			       sizeof(u16));
 	}
 	/* Sanity checks. */
-	len = le16_to_cpu(rxhdr.frame_len);
+	len = le16_to_cpu(pio->rxhdr->frame_len);
 	if (unlikely(len > 0x700)) {
 		err_msg = "len > 0x700";
 		goto rx_error;
@@ -668,7 +684,7 @@ data_ready:
 		goto rx_error;
 	}
 
-	macstat = le32_to_cpu(rxhdr.mac_status);
+	macstat = le32_to_cpu(pio->rxhdr->mac_status);
 	if (macstat & B43_RX_MAC_FCSERR) {
 		if (!(q->dev->wl->filter_flags & FIF_FCSFAIL)) {
 			/* Drop frames with failed FCS. */
@@ -723,7 +739,7 @@ data_ready:
 		}
 	}
 
-	b43_rx(q->dev, skb, &rxhdr);
+	b43_rx(q->dev, skb, pio->rxhdr);
 
 	return 1;
 
-- 
1.6.0.4

Re: [PATCH] b43: do not stack-allocate pio rx/tx header buffers

[Michael Buesch]

On Tuesday 06 October 2009 18:20:43 Albert Herranz wrote:
> The DMA-API debugging facility complains about b43 mapping memory from
> stack for SDIO-based cards, as can be seen in the following two
> stack traces.
> 
>   Call Trace:
>   [d2ef7c60] [c01c55f0] check_for_stack+0xf4/0x10c (unreliable)
>   [d2ef7c80] [c01c6d10] debug_dma_map_sg+0xc4/0x134
>   [d2ef7cb0] [c0281e10] sdhci_prepare_data+0x200/0x590
>   [d2ef7ce0] [c0282288] sdhci_send_command+0xe8/0x320
>   [d2ef7d00] [c02825e0] sdhci_request+0x120/0x1b4
>   [d2ef7d20] [c0277ad4] mmc_wait_for_req+0x124/0x140
>   [d2ef7d50] [c027c2f0] mmc_io_rw_extended+0x188/0x214
>   [d2ef7e10] [c027d7c4] sdio_io_rw_ext_helper+0x104/0x224
>   [d2ef7e40] [c0296000] ssb_sdio_block_read+0xac/0x110
>   [d2ef7e60] [c0237f98] pio_rx_frame+0x1d0/0x43c
>   [d2ef7eb0] [c0238244] b43_pio_rx+0x40/0x90
>   [d2ef7ed0] [c0226124] b43_do_interrupt_thread+0x108/0x444
>   [d2ef7f30] [c0226504] b43_sdio_interrupt_handler+0x58/0x74
>   [d2ef7f40] [c0239110] b43_sdio_interrupt_dispatcher+0x4c/0x68
>   [d2ef7f60] [c027e1ac] sdio_irq_thread+0xf8/0x284
>   [d2ef7fb0] [c0054538] kthread+0x78/0x7c
>   [d2ef7ff0] [c00124ec] kernel_thread+0x4c/0x68
> 
>   Call Trace:
>   [d3335c20] [c01c55f0] check_for_stack+0xf4/0x10c (unreliable)
>   [d3335c40] [c01c6d10] debug_dma_map_sg+0xc4/0x134
>   [d3335c70] [c0281e14] sdhci_prepare_data+0x200/0x590
>   [d3335ca0] [c028228c] sdhci_send_command+0xe8/0x320
>   [d3335cc0] [c02825e4] sdhci_request+0x120/0x1b4
>   [d3335ce0] [c0277ad8] mmc_wait_for_req+0x124/0x140
>   [d3335d10] [c027c2f4] mmc_io_rw_extended+0x188/0x214
>   [d3335dd0] [c027d86c] sdio_io_rw_ext_helper+0x1a8/0x224
>   [d3335e00] [c0295ef4] ssb_sdio_block_write+0xac/0x110
>   [d3335e20] [c0237a18] tx_write_4byte_queue+0x6c/0x138
>   [d3335e40] [c0238610] pio_tx_frame+0x1c4/0x224
>   [d3335ee0] [c0238718] b43_pio_tx+0xa8/0x1f4
>   [d3335f00] [c0220380] b43_tx_work+0x4c/0xe0
>   [d3335f20] [c004fa74] run_workqueue+0x120/0x1a4
>   [d3335f70] [c004fb44] worker_thread+0x4c/0xb0
>   [d3335fb0] [c0054538] kthread+0x78/0x7c
>   [d3335ff0] [c00124ec] kernel_thread+0x4c/0x68
> 
> Indeed, b43 currently allocates the PIO RX and TX header buffers
> from stack. The solution here is to use heap-allocated buffers instead.
> 
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> ---
>  drivers/net/wireless/b43/b43.h |    9 ++++++++
>  drivers/net/wireless/b43/pio.c |   42 +++++++++++++++++++++++++++------------
>  2 files changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index fa1549a..b09dda1 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -550,6 +550,9 @@ struct b43_dma {
>  struct b43_pio_txqueue;
>  struct b43_pio_rxqueue;
>  
> +struct b43_rxhdr_fw4;
> +struct b43_txhdr;
> +
>  /* Data structures for PIO transmission, per 80211 core. */
>  struct b43_pio {
>  	struct b43_pio_txqueue *tx_queue_AC_BK; /* Background */
> @@ -559,6 +562,12 @@ struct b43_pio {
>  	struct b43_pio_txqueue *tx_queue_mcast; /* Multicast */
>  
>  	struct b43_pio_rxqueue *rx_queue;
> +
> +	/*
> +	 * RX/TX header buffers used by the frame transmit functions.
> +	 */
> +	struct b43_rxhdr_fw4 *rxhdr;
> +	struct b43_txhdr *txhdr;
>  };

Just embed it into struct b43_wl (surround it by #ifdef CONFIG_B43_PIO). No need
to kzalloc then and it saves some memory.
You also need to alloc 4 bytes for the tail buffer (that currently is on the stack, too).

-- 
Greetings, Michael.

Re: [PATCH] b43: do not stack-allocate pio rx/tx header buffers

[John W. Linville]

On Tue, Oct 06, 2009 at 10:52:15PM +0200, Michael Buesch wrote:
> On Tuesday 06 October 2009 18:20:43 Albert Herranz wrote:
> > The DMA-API debugging facility complains about b43 mapping memory from
> > stack for SDIO-based cards, as can be seen in the following two
> > stack traces.

<snip>

> > Indeed, b43 currently allocates the PIO RX and TX header buffers
> > from stack. The solution here is to use heap-allocated buffers instead.
> > 
> > Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>

<snip>

> Just embed it into struct b43_wl (surround it by #ifdef CONFIG_B43_PIO). No need
> to kzalloc then and it saves some memory.
> You also need to alloc 4 bytes for the tail buffer (that currently is on the stack, too).

Please make the changes Michael requested and resubmit -- I'll happily
make the adjustments to wireless-testing, etc.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH v2] b43: do not stack-allocate pio rx/tx header and tail buffers

[Albert Herranz]

The DMA-API debugging facility complains about b43 mapping memory from
stack for SDIO-based cards.

Indeed, b43 currently allocates the PIO RX/TX header and tail buffers
from stack. The solution here is to use heap-allocated buffers instead.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
v2
- embed buffers into struct b43_wl, and make them depend on CONFIG_B43_PIO
- take into account tail buffers for unaligned length transfers

 drivers/net/wireless/b43/b43.h  |  168 +++++++++++++++++++++------------------
 drivers/net/wireless/b43/pio.c  |   78 +++++++++---------
 drivers/net/wireless/b43/xmit.c |    2 +-
 3 files changed, 132 insertions(+), 116 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index fa1549a..6607162 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -607,82 +607,7 @@ struct b43_qos_params {
 	struct ieee80211_tx_queue_params p;
 };
 
-struct b43_wldev;
-
-/* Data structure for the WLAN parts (802.11 cores) of the b43 chip. */
-struct b43_wl {
-	/* Pointer to the active wireless device on this chip */
-	struct b43_wldev *current_dev;
-	/* Pointer to the ieee80211 hardware data structure */
-	struct ieee80211_hw *hw;
-
-	/* Global driver mutex. Every operation must run with this mutex locked. */
-	struct mutex mutex;
-	/* Hard-IRQ spinlock. This lock protects things used in the hard-IRQ
-	 * handler, only. This basically is just the IRQ mask register. */
-	spinlock_t hardirq_lock;
-
-	/* The number of queues that were registered with the mac80211 subsystem
-	 * initially. This is a backup copy of hw->queues in case hw->queues has
-	 * to be dynamically lowered at runtime (Firmware does not support QoS).
-	 * hw->queues has to be restored to the original value before unregistering
-	 * from the mac80211 subsystem. */
-	u16 mac80211_initially_registered_queues;
-
-	/* We can only have one operating interface (802.11 core)
-	 * at a time. General information about this interface follows.
-	 */
-
-	struct ieee80211_vif *vif;
-	/* The MAC address of the operating interface. */
-	u8 mac_addr[ETH_ALEN];
-	/* Current BSSID */
-	u8 bssid[ETH_ALEN];
-	/* Interface type. (NL80211_IFTYPE_XXX) */
-	int if_type;
-	/* Is the card operating in AP, STA or IBSS mode? */
-	bool operating;
-	/* filter flags */
-	unsigned int filter_flags;
-	/* Stats about the wireless interface */
-	struct ieee80211_low_level_stats ieee_stats;
-
-#ifdef CONFIG_B43_HWRNG
-	struct hwrng rng;
-	bool rng_initialized;
-	char rng_name[30 + 1];
-#endif /* CONFIG_B43_HWRNG */
-
-	/* List of all wireless devices on this chip */
-	struct list_head devlist;
-	u8 nr_devs;
-
-	bool radiotap_enabled;
-	bool radio_enabled;
-
-	/* The beacon we are currently using (AP or IBSS mode). */
-	struct sk_buff *current_beacon;
-	bool beacon0_uploaded;
-	bool beacon1_uploaded;
-	bool beacon_templates_virgin; /* Never wrote the templates? */
-	struct work_struct beacon_update_trigger;
-
-	/* The current QOS parameters for the 4 queues. */
-	struct b43_qos_params qos_params[4];
-
-	/* Work for adjustment of the transmission power.
-	 * This is scheduled when we determine that the actual TX output
-	 * power doesn't match what we want. */
-	struct work_struct txpower_adjust_work;
-
-	/* Packet transmit work */
-	struct work_struct tx_work;
-	/* Queue of packets to be transmitted. */
-	struct sk_buff_head tx_queue;
-
-	/* The device LEDs. */
-	struct b43_leds leds;
-};
+struct b43_wl;
 
 /* The type of the firmware file. */
 enum b43_firmware_file_type {
@@ -824,6 +749,97 @@ struct b43_wldev {
 #endif
 };
 
+/*
+ * Include goes here to avoid a dependency problem.
+ * A better fix would be to integrate xmit.h into b43.h.
+ */
+#include "xmit.h"
+
+/* Data structure for the WLAN parts (802.11 cores) of the b43 chip. */
+struct b43_wl {
+	/* Pointer to the active wireless device on this chip */
+	struct b43_wldev *current_dev;
+	/* Pointer to the ieee80211 hardware data structure */
+	struct ieee80211_hw *hw;
+
+	/* Global driver mutex. Every operation must run with this mutex locked. */
+	struct mutex mutex;
+	/* Hard-IRQ spinlock. This lock protects things used in the hard-IRQ
+	 * handler, only. This basically is just the IRQ mask register. */
+	spinlock_t hardirq_lock;
+
+	/* The number of queues that were registered with the mac80211 subsystem
+	 * initially. This is a backup copy of hw->queues in case hw->queues has
+	 * to be dynamically lowered at runtime (Firmware does not support QoS).
+	 * hw->queues has to be restored to the original value before unregistering
+	 * from the mac80211 subsystem. */
+	u16 mac80211_initially_registered_queues;
+
+	/* We can only have one operating interface (802.11 core)
+	 * at a time. General information about this interface follows.
+	 */
+
+	struct ieee80211_vif *vif;
+	/* The MAC address of the operating interface. */
+	u8 mac_addr[ETH_ALEN];
+	/* Current BSSID */
+	u8 bssid[ETH_ALEN];
+	/* Interface type. (NL80211_IFTYPE_XXX) */
+	int if_type;
+	/* Is the card operating in AP, STA or IBSS mode? */
+	bool operating;
+	/* filter flags */
+	unsigned int filter_flags;
+	/* Stats about the wireless interface */
+	struct ieee80211_low_level_stats ieee_stats;
+
+#ifdef CONFIG_B43_HWRNG
+	struct hwrng rng;
+	bool rng_initialized;
+	char rng_name[30 + 1];
+#endif /* CONFIG_B43_HWRNG */
+
+	/* List of all wireless devices on this chip */
+	struct list_head devlist;
+	u8 nr_devs;
+
+	bool radiotap_enabled;
+	bool radio_enabled;
+
+	/* The beacon we are currently using (AP or IBSS mode). */
+	struct sk_buff *current_beacon;
+	bool beacon0_uploaded;
+	bool beacon1_uploaded;
+	bool beacon_templates_virgin; /* Never wrote the templates? */
+	struct work_struct beacon_update_trigger;
+
+	/* The current QOS parameters for the 4 queues. */
+	struct b43_qos_params qos_params[4];
+
+	/* Work for adjustment of the transmission power.
+	 * This is scheduled when we determine that the actual TX output
+	 * power doesn't match what we want. */
+	struct work_struct txpower_adjust_work;
+
+	/* Packet transmit work */
+	struct work_struct tx_work;
+	/* Queue of packets to be transmitted. */
+	struct sk_buff_head tx_queue;
+
+	/* The device LEDs. */
+	struct b43_leds leds;
+
+#ifdef CONFIG_B43_PIO
+	/*
+	 * RX/TX header/tail buffers used by the frame transmit functions.
+	 */
+	struct b43_rxhdr_fw4 rxhdr;
+	struct b43_txhdr txhdr;
+	u8 rx_tail[4];
+	u8 tx_tail[4];
+#endif /* CONFIG_B43_PIO */
+};
+
 static inline struct b43_wl *hw_to_b43_wl(struct ieee80211_hw *hw)
 {
 	return hw->priv;
diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
index 9c13979..dbbf0d1 100644
--- a/drivers/net/wireless/b43/pio.c
+++ b/drivers/net/wireless/b43/pio.c
@@ -331,6 +331,7 @@ static u16 tx_write_2byte_queue(struct b43_pio_txqueue *q,
 				unsigned int data_len)
 {
 	struct b43_wldev *dev = q->dev;
+	struct b43_wl *wl = dev->wl;
 	const u8 *data = _data;
 
 	ctl |= B43_PIO_TXCTL_WRITELO | B43_PIO_TXCTL_WRITEHI;
@@ -340,13 +341,12 @@ static u16 tx_write_2byte_queue(struct b43_pio_txqueue *q,
 			q->mmio_base + B43_PIO_TXDATA,
 			sizeof(u16));
 	if (data_len & 1) {
-		u8 tail[2] = { 0, };
-
 		/* Write the last byte. */
 		ctl &= ~B43_PIO_TXCTL_WRITEHI;
 		b43_piotx_write16(q, B43_PIO_TXCTL, ctl);
-		tail[0] = data[data_len - 1];
-		ssb_block_write(dev->dev, tail, 2,
+		wl->tx_tail[0] = data[data_len - 1];
+		wl->tx_tail[1] = 0;
+		ssb_block_write(dev->dev, wl->tx_tail, 2,
 				q->mmio_base + B43_PIO_TXDATA,
 				sizeof(u16));
 	}
@@ -381,6 +381,7 @@ static u32 tx_write_4byte_queue(struct b43_pio_txqueue *q,
 				unsigned int data_len)
 {
 	struct b43_wldev *dev = q->dev;
+	struct b43_wl *wl = dev->wl;
 	const u8 *data = _data;
 
 	ctl |= B43_PIO8_TXCTL_0_7 | B43_PIO8_TXCTL_8_15 |
@@ -391,29 +392,31 @@ static u32 tx_write_4byte_queue(struct b43_pio_txqueue *q,
 			q->mmio_base + B43_PIO8_TXDATA,
 			sizeof(u32));
 	if (data_len & 3) {
-		u8 tail[4] = { 0, };
-
+		wl->tx_tail[3] = 0;
 		/* Write the last few bytes. */
 		ctl &= ~(B43_PIO8_TXCTL_8_15 | B43_PIO8_TXCTL_16_23 |
 			 B43_PIO8_TXCTL_24_31);
 		switch (data_len & 3) {
 		case 3:
 			ctl |= B43_PIO8_TXCTL_16_23 | B43_PIO8_TXCTL_8_15;
-			tail[0] = data[data_len - 3];
-			tail[1] = data[data_len - 2];
-			tail[2] = data[data_len - 1];
+			wl->tx_tail[0] = data[data_len - 3];
+			wl->tx_tail[1] = data[data_len - 2];
+			wl->tx_tail[2] = data[data_len - 1];
 			break;
 		case 2:
 			ctl |= B43_PIO8_TXCTL_8_15;
-			tail[0] = data[data_len - 2];
-			tail[1] = data[data_len - 1];
+			wl->tx_tail[0] = data[data_len - 2];
+			wl->tx_tail[1] = data[data_len - 1];
+			wl->tx_tail[2] = 0;
 			break;
 		case 1:
-			tail[0] = data[data_len - 1];
+			wl->tx_tail[0] = data[data_len - 1];
+			wl->tx_tail[1] = 0;
+			wl->tx_tail[2] = 0;
 			break;
 		}
 		b43_piotx_write32(q, B43_PIO8_TXCTL, ctl);
-		ssb_block_write(dev->dev, tail, 4,
+		ssb_block_write(dev->dev, wl->tx_tail, 4,
 				q->mmio_base + B43_PIO8_TXDATA,
 				sizeof(u32));
 	}
@@ -445,8 +448,9 @@ static void pio_tx_frame_4byte_queue(struct b43_pio_txpacket *pack,
 static int pio_tx_frame(struct b43_pio_txqueue *q,
 			struct sk_buff *skb)
 {
+	struct b43_wldev *dev = q->dev;
+	struct b43_wl *wl = dev->wl;
 	struct b43_pio_txpacket *pack;
-	struct b43_txhdr txhdr;
 	u16 cookie;
 	int err;
 	unsigned int hdrlen;
@@ -457,8 +461,8 @@ static int pio_tx_frame(struct b43_pio_txqueue *q,
 			  struct b43_pio_txpacket, list);
 
 	cookie = generate_cookie(q, pack);
-	hdrlen = b43_txhdr_size(q->dev);
-	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb,
+	hdrlen = b43_txhdr_size(dev);
+	err = b43_generate_txhdr(dev, (u8 *)&wl->txhdr, skb,
 				 info, cookie);
 	if (err)
 		return err;
@@ -466,15 +470,15 @@ static int pio_tx_frame(struct b43_pio_txqueue *q,
 	if (info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) {
 		/* Tell the firmware about the cookie of the last
 		 * mcast frame, so it can clear the more-data bit in it. */
-		b43_shm_write16(q->dev, B43_SHM_SHARED,
+		b43_shm_write16(dev, B43_SHM_SHARED,
 				B43_SHM_SH_MCASTCOOKIE, cookie);
 	}
 
 	pack->skb = skb;
 	if (q->rev >= 8)
-		pio_tx_frame_4byte_queue(pack, (const u8 *)&txhdr, hdrlen);
+		pio_tx_frame_4byte_queue(pack, (const u8 *)&wl->txhdr, hdrlen);
 	else
-		pio_tx_frame_2byte_queue(pack, (const u8 *)&txhdr, hdrlen);
+		pio_tx_frame_2byte_queue(pack, (const u8 *)&wl->txhdr, hdrlen);
 
 	/* Remove it from the list of available packet slots.
 	 * It will be put back when we receive the status report. */
@@ -614,14 +618,14 @@ void b43_pio_get_tx_stats(struct b43_wldev *dev,
 static bool pio_rx_frame(struct b43_pio_rxqueue *q)
 {
 	struct b43_wldev *dev = q->dev;
-	struct b43_rxhdr_fw4 rxhdr;
+	struct b43_wl *wl = dev->wl;
 	u16 len;
 	u32 macstat;
 	unsigned int i, padding;
 	struct sk_buff *skb;
 	const char *err_msg = NULL;
 
-	memset(&rxhdr, 0, sizeof(rxhdr));
+	memset(&wl->rxhdr, 0, sizeof(wl->rxhdr));
 
 	/* Check if we have data and wait for it to get ready. */
 	if (q->rev >= 8) {
@@ -659,16 +663,16 @@ data_ready:
 
 	/* Get the preamble (RX header) */
 	if (q->rev >= 8) {
-		ssb_block_read(dev->dev, &rxhdr, sizeof(rxhdr),
+		ssb_block_read(dev->dev, &wl->rxhdr, sizeof(wl->rxhdr),
 			       q->mmio_base + B43_PIO8_RXDATA,
 			       sizeof(u32));
 	} else {
-		ssb_block_read(dev->dev, &rxhdr, sizeof(rxhdr),
+		ssb_block_read(dev->dev, &wl->rxhdr, sizeof(wl->rxhdr),
 			       q->mmio_base + B43_PIO_RXDATA,
 			       sizeof(u16));
 	}
 	/* Sanity checks. */
-	len = le16_to_cpu(rxhdr.frame_len);
+	len = le16_to_cpu(wl->rxhdr.frame_len);
 	if (unlikely(len > 0x700)) {
 		err_msg = "len > 0x700";
 		goto rx_error;
@@ -678,7 +682,7 @@ data_ready:
 		goto rx_error;
 	}
 
-	macstat = le32_to_cpu(rxhdr.mac_status);
+	macstat = le32_to_cpu(wl->rxhdr.mac_status);
 	if (macstat & B43_RX_MAC_FCSERR) {
 		if (!(q->dev->wl->filter_flags & FIF_FCSFAIL)) {
 			/* Drop frames with failed FCS. */
@@ -703,24 +707,22 @@ data_ready:
 			       q->mmio_base + B43_PIO8_RXDATA,
 			       sizeof(u32));
 		if (len & 3) {
-			u8 tail[4] = { 0, };
-
 			/* Read the last few bytes. */
-			ssb_block_read(dev->dev, tail, 4,
+			ssb_block_read(dev->dev, wl->rx_tail, 4,
 				       q->mmio_base + B43_PIO8_RXDATA,
 				       sizeof(u32));
 			switch (len & 3) {
 			case 3:
-				skb->data[len + padding - 3] = tail[0];
-				skb->data[len + padding - 2] = tail[1];
-				skb->data[len + padding - 1] = tail[2];
+				skb->data[len + padding - 3] = wl->rx_tail[0];
+				skb->data[len + padding - 2] = wl->rx_tail[1];
+				skb->data[len + padding - 1] = wl->rx_tail[2];
 				break;
 			case 2:
-				skb->data[len + padding - 2] = tail[0];
-				skb->data[len + padding - 1] = tail[1];
+				skb->data[len + padding - 2] = wl->rx_tail[0];
+				skb->data[len + padding - 1] = wl->rx_tail[1];
 				break;
 			case 1:
-				skb->data[len + padding - 1] = tail[0];
+				skb->data[len + padding - 1] = wl->rx_tail[0];
 				break;
 			}
 		}
@@ -729,17 +731,15 @@ data_ready:
 			       q->mmio_base + B43_PIO_RXDATA,
 			       sizeof(u16));
 		if (len & 1) {
-			u8 tail[2] = { 0, };
-
 			/* Read the last byte. */
-			ssb_block_read(dev->dev, tail, 2,
+			ssb_block_read(dev->dev, wl->rx_tail, 2,
 				       q->mmio_base + B43_PIO_RXDATA,
 				       sizeof(u16));
-			skb->data[len + padding - 1] = tail[0];
+			skb->data[len + padding - 1] = wl->rx_tail[0];
 		}
 	}
 
-	b43_rx(q->dev, skb, &rxhdr);
+	b43_rx(q->dev, skb, &wl->rxhdr);
 
 	return 1;
 
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index ac9f600..892573b 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -27,7 +27,7 @@
 
 */
 
-#include "xmit.h"
+#include "b43.h"
 #include "phy_common.h"
 #include "dma.h"
 #include "pio.h"
-- 
1.6.0.4

Re: [PATCH v2] b43: do not stack-allocate pio rx/tx header and tail buffers

[Larry Finger]

Albert Herranz wrote:
> The DMA-API debugging facility complains about b43 mapping memory from
> stack for SDIO-based cards.
> 
> Indeed, b43 currently allocates the PIO RX/TX header and tail buffers
> from stack. The solution here is to use heap-allocated buffers instead.
> 
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> ---
> v2
> - embed buffers into struct b43_wl, and make them depend on CONFIG_B43_PIO
> - take into account tail buffers for unaligned length transfers
> 
>  drivers/net/wireless/b43/b43.h  |  168 +++++++++++++++++++++------------------
>  drivers/net/wireless/b43/pio.c  |   78 +++++++++---------
>  drivers/net/wireless/b43/xmit.c |    2 +-
>  3 files changed, 132 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index fa1549a..6607162 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -607,82 +607,7 @@ struct b43_qos_params {
>  	struct ieee80211_tx_queue_params p;
>  };
>  
> -struct b43_wldev;
> -
> -/* Data structure for the WLAN parts (802.11 cores) of the b43 chip. */
> -struct b43_wl {
> -	/* Pointer to the active wireless device on this chip */
> -	struct b43_wldev *current_dev;
> -	/* Pointer to the ieee80211 hardware data structure */
> -	struct ieee80211_hw *hw;

What is the reason for moving the definition of struct b43_wl?

Larry

Re: [PATCH v2] b43: do not stack-allocate pio rx/tx header and tail buffers

[Albert Herranz]

Larry Finger wrote:
> Albert Herranz wrote:
>> The DMA-API debugging facility complains about b43 mapping memory from
>> stack for SDIO-based cards.
>>
>> Indeed, b43 currently allocates the PIO RX/TX header and tail buffers
>> from stack. The solution here is to use heap-allocated buffers instead.
>>
>> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
>> ---
>> v2
>> - embed buffers into struct b43_wl, and make them depend on CONFIG_B43_PIO
>> - take into account tail buffers for unaligned length transfers
>>
>>  drivers/net/wireless/b43/b43.h  |  168 +++++++++++++++++++++------------------
>>  drivers/net/wireless/b43/pio.c  |   78 +++++++++---------
>>  drivers/net/wireless/b43/xmit.c |    2 +-
>>  3 files changed, 132 insertions(+), 116 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
>> index fa1549a..6607162 100644
>> --- a/drivers/net/wireless/b43/b43.h
>> +++ b/drivers/net/wireless/b43/b43.h
>> @@ -607,82 +607,7 @@ struct b43_qos_params {
>>  	struct ieee80211_tx_queue_params p;
>>  };
>>  
>> -struct b43_wldev;
>> -
>> -/* Data structure for the WLAN parts (802.11 cores) of the b43 chip. */
>> -struct b43_wl {
>> -	/* Pointer to the active wireless device on this chip */
>> -	struct b43_wldev *current_dev;
>> -	/* Pointer to the ieee80211 hardware data structure */
>> -	struct ieee80211_hw *hw;
> 
> What is the reason for moving the definition of struct b43_wl?
> 
> Larry
> 
> 

b43_new_kidx_api() (defined in xmit.h) needs struct b43_wldev defined because it dereferences it.
With this patch, struct b43_wl (defined in b43.h) needs struct b43_rxhdr_fw4 and struct b43_txhdr (defined in xmit.h).

So we have b43_wldev -> b43_rxhdr_fw4, b43_txhdr -> b43_wl (at least).

Moving the definition of struct b43_wl after the definition of struct b43_wldev and placing the inclusion of xmit.h between them gets rid of the generated dependencies.

The patch hints too to other possible solution.

>/*
> * Include goes here to avoid a dependency problem.
> * A better fix would be to integrate xmit.h into b43.h.
> */
>#include "xmit.h"

Do you have any other solutions in mind?

Thanks,
Albert

Re: [PATCH v2] b43: do not stack-allocate pio rx/tx header and tail buffers

[Larry Finger]

Albert Herranz wrote:
> Larry Finger wrote:
>> Albert Herranz wrote:
> 
> b43_new_kidx_api() (defined in xmit.h) needs struct b43_wldev defined because it dereferences it.
> With this patch, struct b43_wl (defined in b43.h) needs struct b43_rxhdr_fw4 and struct b43_txhdr (defined in xmit.h).
> 
> So we have b43_wldev -> b43_rxhdr_fw4, b43_txhdr -> b43_wl (at least).
> 
> Moving the definition of struct b43_wl after the definition of struct b43_wldev and placing the inclusion of xmit.h between them gets rid of the generated dependencies.
> 
> The patch hints too to other possible solution.
> 
>> /*
>> * Include goes here to avoid a dependency problem.
>> * A better fix would be to integrate xmit.h into b43.h.
>> */
>> #include "xmit.h"
> 
> Do you have any other solutions in mind?

No. I missed the hint. At first I thought there was a white-space
problem that caused b43_wl to be replaced. When that was not the case,
I posed my question.

All seems OK.

Larry

b43: do not stack-allocate pio rx/tx header and tail buffers (was: pull request: wireless-2.6 2009-10-08)

[Albert Herranz]

Michael,

> Wait for my ack before you apply random b43 patches
                                   ^^^^^^
> The patch needlessly moves huge chunks of crap, adds stupid comments, wastes memory.
                                  ^^^^^^^^^^^^^^       ^^^^^^^^^^^^^^^
> This touched a lot of crap for fixing something that
       ^^^^^^^^^^^^^^^^^^^^^

If using that strong language helps you get your anger out of you I'm fine with that. But I think it is a bit unfair.

I'm not arguing if the patch should have been immediately merged upstream or not without your ack (I'm probably more on your side here, as the patch was still being discussed on the ML).
The patch [1] may not be up to your quality standards or may not take into account other requirements (that you have not expressed nor on the ML nor on IRC) but I'm sure it's far from being "random", "move crap" or "add stupid comments". That's the unfair part.

The reason I posted the initial patch for review was because you kind of told me [2].

[20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches

Then I tried to address your concerns [3] and sent v2 (the patch merged).
In that patch I moved slightly struct b43_wl within b43.h to satisfy the dependencies generated by the addition of the new members to that struct (which was one of your concerns).
There was even a bit of public discussion about the relocation of the struct declaration [4].

After ~22 hours if I'm not mistaken (yes, less than 24...) John, who had previously expressed his intentions to merge the patch [5], picked it for wireless-2.6.
And a day later, more or less again, John did the GIT PULL request.

That's it. IMHO, even if the patch was (mistakenly) merged too early for your standards, it doesn't break anything (AFAIK) and delivers what promises.
And (yes!) it can still be modeled to perfection later.

My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.

Other than that, I'm happy to see that the side conflict generated seems to be in a good shape now.
It would have been a total waste otherwise.

Thanks,
Albert

PS: Sorry to generate a new post, but I wasn't directly CC'ed on this and I'm not subscribed to the MLs. And I did also want to express my opinion! :)

[1] http://git.kernel.org/?p=linux/kernel/git/linville/wireless-2.6.git;a=commitdiff;h=7e937c633f718e0916a294db7282c922c1bf3ce3
[2] http://bcm-specs.sipsolutions.net/irc-logs/bcm-specs.2009-10-01
[3] http://marc.info/?l=linux-wireless&m=125486249831043
[4] http://marc.info/?l=linux-wireless&m=125493883613139
[5] http://marc.info/?l=linux-wireless&m=125486386100505

Re: b43: do not stack-allocate pio rx/tx header and tail buffers (was: pull request: wireless-2.6 2009-10-08)

[Michael Buesch]

On Friday 09 October 2009 19:46:31 Albert Herranz wrote:
> I'm not arguing if the patch should have been immediately merged upstream or not without your ack (I'm probably more on your side here, as the patch was still being discussed on the ML).
> The patch [1] may not be up to your quality standards or may not take into account other requirements (that you have not expressed nor on the ML nor on IRC) but I'm sure it's far from being "random", "move crap" or "add stupid comments". That's the unfair part.
> 
> The reason I posted the initial patch for review was because you kind of told me [2].
> 
> [20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches

Yeah, but that doesn't mean that either hack is acceptable. It just means that my time is limited
and I added this non-issue (which I still think it is) to the very bottom of my priority list.

> After ~22 hours if I'm not mistaken (yes, less than 24...) John, who had previously expressed his intentions to merge the patch [5], picked it for wireless-2.6.
> And a day later, more or less again, John did the GIT PULL request.

My impression was, that if the maintainer rejects a patch and asks for a new version,
the upstream maintainer must not take the patch until the maintainer acked the new version.
It seems I was wrong, though.

> My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
> I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.

It _was_ immediately NAK'ed by me. I did not know that I need to NAK
every single new version of a patch explicitely.
I thought NAK-ing a patch would put it into some special state that only an explicit ACK could
take it out of.

-- 
Greetings, Michael.

Re: b43: do not stack-allocate pio rx/tx header and tail buffers

[Albert Herranz]

Michael Buesch wrote:
>> The reason I posted the initial patch for review was because you kind of told me [2].
>>
>> [20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches
> 
> Yeah, but that doesn't mean that either hack is acceptable. It just means that my time is limited
> and I added this non-issue (which I still think it is) to the very bottom of my priority list.

The patch got elaborated and discussed publicly (successfully or not) by following your instructions.

>> My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
>> I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.
> 
> It _was_ immediately NAK'ed by me. I did not know that I need to NAK
> every single new version of a patch explicitely.
> I thought NAK-ing a patch would put it into some special state that only an explicit ACK could
> take it out of.

We all sure had a communication issue here.

What you thought it was an (implicit) NAK for the _initial_ version of the patch, others took that as "fix-those-concerns-and-its-fine".
And the expressed concerns where addressed later in the merged patch, sub-optimally (not crappily).

Looking at your new "[PATCH] b43: Optimize PIO scratchbuffer usage" to address the changes introduced by the merged patch, the merged solution is not that _blatantly_ far from your solution.
The patch would have probably got there in one iteration if you have had the chance again to express your new concerns about v2.

I'm sure we can avoid this in the future by being a bit more explicit.

Thanks,
Albert