[PATCH] ath5k: fix requested allocated RX skb size for DMA

[PATCH] ath5k: fix requested allocated RX skb size for DMA

[Luis R. Rodriguez]

When we dev_alloc_skb() our RX buffers we were asking
for them to be of size:

sc->rxbufsize + sc->common.cachelsz - 1

but when mapping processor virtual memeory for access by the
hardware we were only giving it the sc->rxbufsize, therefore
always preventing the device from accessing the fully allocated
skb.

This patch fixes the disparity. It is unclear to me if there are
other consequences to having this mismatch other than perhaps never
allowing the device to use the full capacity of the allocated skb.

Cc: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Kossifidis <mickflemm@gmail.com>
Cc: Bob Copeland <me@bobcopeland.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/base.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 2b3cf39..75843f2 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1157,17 +1157,18 @@ struct sk_buff *ath5k_rx_skb_alloc(struct ath5k_softc *sc, dma_addr_t *skb_addr)
 	 * fake physical layer header at the start.
 	 */
 	skb = ath_rxbuf_alloc(&sc->common,
-			      sc->rxbufsize + sc->common.cachelsz - 1,
+			      sc->rxbufsize,
 			      GFP_ATOMIC);
 
 	if (!skb) {
 		ATH5K_ERR(sc, "can't alloc skbuff of size %u\n",
-				sc->rxbufsize + sc->common.cachelsz - 1);
+				sc->rxbufsize);
 		return NULL;
 	}
 
 	*skb_addr = pci_map_single(sc->pdev,
 		skb->data, sc->rxbufsize, PCI_DMA_FROMDEVICE);
+
 	if (unlikely(pci_dma_mapping_error(sc->pdev, *skb_addr))) {
 		ATH5K_ERR(sc, "%s: DMA mapping failed\n", __func__);
 		dev_kfree_skb(skb);
@@ -1607,9 +1608,10 @@ ath5k_rx_start(struct ath5k_softc *sc)
 	int ret;
 
 	sc->rxbufsize = roundup(IEEE80211_MAX_LEN, sc->common.cachelsz);
+	sc->rxbufsize += sc->common.cachelsz - 1;
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "cachelsz %u rxbufsize %u\n",
-		sc->common.cachelsz, sc->rxbufsize);
+		  sc->common.cachelsz, sc->rxbufsize);
 
 	spin_lock_bh(&sc->rxbuflock);
 	sc->rxlink = NULL;
-- 
1.6.3.3

Re: [PATCH] ath5k: fix requested allocated RX skb size for DMA

[Jiri Slaby]

On 08/12/2009 06:58 PM, Luis R. Rodriguez wrote:
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -1157,17 +1157,18 @@ struct sk_buff *ath5k_rx_skb_alloc(struct ath5k_softc *sc, dma_addr_t *skb_addr)
>  	 * fake physical layer header at the start.
>  	 */
>  	skb = ath_rxbuf_alloc(&sc->common,
> -			      sc->rxbufsize + sc->common.cachelsz - 1,

What was the exact purpose of this? My guess is that we should map
starting at a next cache line boundary and put this aligned address to
the device instead?

Re: [ath9k-devel] [PATCH] ath5k: fix requested allocated RX skb size for DMA

[Luis R. Rodriguez]

On Wed, Aug 12, 2009 at 11:13 AM, Jiri Slaby<jirislaby@gmail.com> wrote:
> On 08/12/2009 06:58 PM, Luis R. Rodriguez wrote:
>> --- a/drivers/net/wireless/ath/ath5k/base.c
>> +++ b/drivers/net/wireless/ath/ath5k/base.c
>> @@ -1157,17 +1157,18 @@ struct sk_buff *ath5k_rx_skb_alloc(struct ath5k_softc *sc, dma_addr_t *skb_addr)
>>        * fake physical layer header at the start.
>>        */
>>       skb = ath_rxbuf_alloc(&sc->common,
>> -                           sc->rxbufsize + sc->common.cachelsz - 1,
>
> What was the exact purpose of this? My guess is that we should map
> starting at a next cache line boundary and put this aligned address to
> the device instead?

I've heard two theories:

1) AR5210 *required* it otherwise bad we would get bad data
2) Performance considerations

I've tried checking internally but haven't found out yet the exact
answer and this seems purely historical. I poked Sam Leffler to see if
he recalls. But as we the 0x41 reset which I removed it seems to be
best to leave unless we can be 100% sure we can remove this.

  Luis

Re: [ath9k-devel] [PATCH] ath5k: fix requested allocated RX skb size for DMA

[Bob Copeland]

On Wed, Aug 12, 2009 at 2:48 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> On Wed, Aug 12, 2009 at 11:13 AM, Jiri Slaby<jirislaby@gmail.com> wrote:
>> What was the exact purpose of this? My guess is that we should map
>> starting at a next cache line boundary and put this aligned address to
>> the device instead?
>
> I've heard two theories:
>
> 1) AR5210 *required* it otherwise bad we would get bad data
> 2) Performance considerations

>From what I can tell Jiri has the right guess:

  // ?? nice pointer arithmetic... should use PTR_ALIGN here?
  off = ((unsigned long) skb->data) % sc->cachelsz;
  if (off != 0)
      skb_reserve(skb, sc->cachelsz);

in other words, when we allocate, round up to the next cache line
greater than IEEE80211_MAX_LEN, then add an extra cache_line-1
bytes so we can map starting from it.

dev_alloc_skb already does some padding and alignment, and it's
configurable on a per-arch basis (though looks like only powerpc
sets it to L1 cache size, everywhere else it's 32 bytes.)

I guess if someone is bored some benchmarking would be useful.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath9k-devel] [PATCH] ath5k: fix requested allocated RX skb size for DMA

[Luis R. Rodriguez]

John please ignore this patch, its busted, as Bob noted on IRC.

  Luis