2010-04-10 15:05:15

by Ming Lei

[permalink] [raw]
Subject: [PATCH 1/3] ath9k-htc: replace __dev_alloc_skb with alloc_skb in ath9k_hif_usb_alloc_rx_urbs

From: Ming Lei <[email protected]>

In ath9k_hif_usb_alloc_rx_urbs, ath9k-htc will pass skb->data into
usb hcd and usb hcd will do dma mapping and unmapping to the buffer
pointed by skb->data, so we should pass a cache-line aligned address.

This patch replace __dev_alloc_skb with alloc_skb to make skb->data
pointed to a cacheline aligned address simply since ath9k-htc does not
skb_push on the skb.

Signed-off-by: Ming Lei <[email protected]>
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index e2117e7..a498815 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -625,7 +625,7 @@ static int ath9k_hif_usb_alloc_rx_urbs(struct hif_device_usb *hif_dev)
}

/* Allocate buffer */
- skb = __dev_alloc_skb(MAX_RX_BUF_SIZE, GFP_KERNEL);
+ skb = alloc_skb(MAX_RX_BUF_SIZE, GFP_KERNEL);
if (!skb) {
ret = -ENOMEM;
goto err_skb;
--
1.6.2.5



2010-04-11 05:33:57

by Sujith

[permalink] [raw]
Subject: [PATCH 1/3] ath9k-htc: replace __dev_alloc_skb with alloc_skb in ath9k_hif_usb_alloc_rx_urbs

[email protected] wrote:
> In ath9k_hif_usb_alloc_rx_urbs, ath9k-htc will pass skb->data into
> usb hcd and usb hcd will do dma mapping and unmapping to the buffer
> pointed by skb->data, so we should pass a cache-line aligned address.
>
> This patch replace __dev_alloc_skb with alloc_skb to make skb->data
> pointed to a cacheline aligned address simply since ath9k-htc does not
> skb_push on the skb.
>

AFAIK, the only architecture that has a requirement to override NET_SKB_PAD
is powerpc, and it does it anyway - by setting it to a cacheline size.

So why do we need to do this ?

Sujith

2010-04-12 07:23:46

by Sujith

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath9k-htc: replace __dev_alloc_skb with alloc_skb in ath9k_hif_usb_alloc_rx_urbs

Ming Lei wrote:
> The skb->data is passed into usb hcd and usb hcd will do dma mapping
> for the buffer of skb->data, so it is better to keep it cache-line aligned.
>
> Size of cache line is very cpu dependent, we can't suppose it is
> 32byte(NET_SKB_PAD) aligned, 64byte or more is very possible.

Hm. Indeed, using alloc_skb() is right in this case,
since we reuse the same RX buffer and never pass it on to mac80211 directly.
Instead, new SKBs are formed from the stream and sent to mac80211.

So yes, it makes sense to use alloc_skb() instead of dev_alloc_skb().
A small nitpick: It would be a bit clearer if you can use kfree_skb()
instead of dev_kfree_skb_any() in the corresponding places.

Sujith

2010-04-12 14:19:01

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath9k-htc: replace __dev_alloc_skb with alloc_skb in ath9k_hif_usb_alloc_rx_urbs

2010/4/12 Sujith <[email protected]>:
> Ming Lei wrote:
>> The skb->data is passed into usb hcd and usb hcd will do dma mapping
>> for the buffer of skb->data, so it is better to keep it cache-line aligned.
>>
>> Size of cache line is very cpu dependent, we can't suppose it is
>> 32byte(NET_SKB_PAD) aligned, 64byte or more is very possible.
>
> Hm. Indeed, using alloc_skb() is right in this case,
> since we reuse the same RX buffer and never pass it on to mac80211 directly.
> Instead, new SKBs are formed from the stream and sent to mac80211.
>
> So yes, it makes sense to use alloc_skb() instead of dev_alloc_skb().
> A small nitpick: It would be a bit clearer if you can use kfree_skb()
> instead of dev_kfree_skb_any() in the corresponding places.

OK, I'll submit a new version to use kfree_skb.


--
Lei Ming

2010-04-11 13:11:37

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath9k-htc: replace __dev_alloc_skb with alloc_skb in ath9k_hif_usb_alloc_rx_urbs

2010/4/11 Sujith <[email protected]>:
> [email protected] wrote:
>> In ath9k_hif_usb_alloc_rx_urbs, ath9k-htc will pass skb->data into
>> usb hcd and usb hcd will do dma mapping and unmapping to the buffer
>> pointed by skb->data, so we should pass a cache-line aligned address.
>>
>> This patch replace __dev_alloc_skb with alloc_skb to make skb->data
>> pointed to a cacheline aligned address simply since ath9k-htc does not
>> skb_push on the skb.
>>
>
> AFAIK, the only architecture that has a requirement to override NET_SKB_PAD
> is powerpc, and it does it anyway - by setting it to a cacheline size.
>
> So why do we need to do this ?

Documentation/DMA-API.txt says:
| Warnings: Memory coherency operates at a granularity called the cache
| line width. In order for memory mapped by this API to operate
| correctly, the mapped region must begin exactly on a cache line
| boundary and end exactly on one (to prevent two separately mapped
| regions from sharing a single cache line). Since the cache line size
| may not be known at compile time, the API will not enforce this
| requirement. Therefore, it is recommended that driver writers who
| don't take special care to determine the cache line size at run time
| only map virtual regions that begin and end on page boundaries (which
| are guaranteed also to be cache line boundaries)

The skb->data is passed into usb hcd and usb hcd will do dma mapping
for the buffer of skb->data, so it is better to keep it cache-line aligned.

Size of cache line is very cpu dependent, we can't suppose it is
32byte(NET_SKB_PAD) aligned, 64byte or more is very possible.

Thanks,

--
Lei Ming