[patch below, sorry for the long list of CCs]
So I dug deeper into the DMA problems I had with iwlagn and a kind soul
helped me in that he said something about pci-e alignment and mentioned
the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
something the device would fail with, but when I looked into the
function for a minute anyway I stumbled over this little gem:
BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
Clearly, that is a totally bogus check, one would hope the compiler
removes it entirely. (Think about it)
After fixing it, I obviously ran into it, nothing guarantees the
alignment the way you want it, because of the way skbs and their
headroom are allocated. I won't explain that here nor double-check that
I'm right, that goes beyond what most of the CC'ed people care about.
So then I came up with the patch below, and so far my system has
survived minutes with 64K pages, when it would previously fail in
seconds. And I haven't seen a single instance of the TX bug either. But
when you see the patch it'll be pretty obvious to you why.
This should fix the following reported kernel bugs:
http://bugzilla.kernel.org/show_bug.cgi?id=11596
http://bugzilla.kernel.org/show_bug.cgi?id=11393
http://bugzilla.kernel.org/show_bug.cgi?id=11983
I haven't checked if there are any elsewhere, but I suppose RHBZ will
have a few instances too...
I'd like to ask anyone who is CC'ed (those are people I know ran into
the bug) to try this patch.
I am convinced that this patch is correct in spirit, but I haven't
understood why, for example, there are so many unmap calls. I'm not
entirely convinced that this is the only bug leading to the TX reply
errors.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 6 +++---
drivers/net/wireless/iwlwifi/iwl-dev.h | 3 ++-
drivers/net/wireless/iwlwifi/iwl-rx.c | 19 +++++++++++--------
3 files changed, 16 insertions(+), 12 deletions(-)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-18 00:25:20.467874837 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-18 00:48:33.639664984 +0100
@@ -1229,7 +1229,7 @@ void iwl_rx_handle(struct iwl_priv *priv
rxq->queue[i] = NULL;
- pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->dma_addr,
+ pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->aligned_dma_addr,
priv->hw_params.rx_buf_size,
PCI_DMA_FROMDEVICE);
pkt = (struct iwl_rx_packet *)rxb->skb->data;
@@ -1282,8 +1282,8 @@ void iwl_rx_handle(struct iwl_priv *priv
rxb->skb = NULL;
}
- pci_unmap_single(priv->pci_dev, rxb->dma_addr,
- priv->hw_params.rx_buf_size,
+ pci_unmap_single(priv->pci_dev, rxb->real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
spin_lock_irqsave(&rxq->lock, flags);
list_add_tail(&rxb->list, &priv->rxq.rx_used);
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-11-18 00:35:58.731046047 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-11-18 00:36:07.097669223 +0100
@@ -89,7 +89,8 @@ extern struct iwl_cfg iwl5100_abg_cfg;
#define DEFAULT_LONG_RETRY_LIMIT 4U
struct iwl_rx_mem_buffer {
- dma_addr_t dma_addr;
+ dma_addr_t real_dma_addr;
+ dma_addr_t aligned_dma_addr;
struct sk_buff *skb;
struct list_head list;
};
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-11-17 23:52:50.090694856 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-11-18 01:03:25.503685123 +0100
@@ -204,7 +204,7 @@ int iwl_rx_queue_restock(struct iwl_priv
list_del(element);
/* Point to Rx buffer via next RBD in circular buffer */
- rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->dma_addr);
+ rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->aligned_dma_addr);
rxq->queue[rxq->write] = rxb;
rxq->write = (rxq->write + 1) & RX_QUEUE_MASK;
rxq->free_count--;
@@ -250,7 +250,7 @@ void iwl_rx_allocate(struct iwl_priv *pr
rxb = list_entry(element, struct iwl_rx_mem_buffer, list);
/* Alloc a new receive buffer */
- rxb->skb = alloc_skb(priv->hw_params.rx_buf_size,
+ rxb->skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
__GFP_NOWARN | GFP_ATOMIC);
if (!rxb->skb) {
if (net_ratelimit())
@@ -265,11 +265,14 @@ void iwl_rx_allocate(struct iwl_priv *pr
list_del(element);
/* Get physical address of RB/SKB */
- rxb->dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
- priv->hw_params.rx_buf_size, PCI_DMA_FROMDEVICE);
+ rxb->real_dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
+ priv->hw_params.rx_buf_size + 256, PCI_DMA_FROMDEVICE);
- /* RBD must be 256 bytes aligned and no more than 36 bits */
- BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
+ /* dma address must be no more than 36 bits */
+ BUG_ON(rxb->real_dma_addr & ~DMA_BIT_MASK(36));
+ /* and also 256 byte aligned! */
+ rxb->aligned_dma_addr = ALIGN(rxb->real_dma_addr, 256);
+ skb_reserve(rxb->skb, rxb->aligned_dma_addr - rxb->real_dma_addr);
list_add_tail(&rxb->list, &rxq->rx_free);
rxq->free_count++;
@@ -302,7 +305,7 @@ void iwl_rx_queue_free(struct iwl_priv *
for (i = 0; i < RX_QUEUE_SIZE + RX_FREE_BUFFERS; i++) {
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
+ rxq->pool[i].real_dma_addr,
priv->hw_params.rx_buf_size,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(rxq->pool[i].skb);
@@ -370,7 +373,7 @@ void iwl_rx_queue_reset(struct iwl_priv
* to an SKB, so we need to unmap and free potential storage */
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
+ rxq->pool[i].real_dma_addr,
priv->hw_params.rx_buf_size,
PCI_DMA_FROMDEVICE);
priv->alloc_rxb_skb--;
On Tue, Nov 18, 2008 at 5:25 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-11-18 at 16:13 +0200, Tomas Winkler wrote:
>
>> >> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
>> >> helped me in that he said something about pci-e alignment and mentioned
>> >> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
>> >> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
>> >> something the device would fail with
>> >
>> > Come to think of it, maybe it does. One common failure case seems to be
>> > "this goes wrong with an 11n AP, but not on a regular one", so one
>> > explanation for that would be that the hardware designers expect the
>> > driver to split up the DMA into multiple DMA descriptors at 4K
>> > boundaries, but iwlagn uses only one and undefined behaviour results.
>> > That's something you'll need to ask the HW designers though or look up
>> > in the datasheet.
>>
>> You are off here. The constrain is just that address will fit in 32
>> bit register. I don't know why this constrain, but there is always a
>> reason.
>
> There's also the PCI-E constraint of having to split up DMA at 4k
> boundaries, which is what I was talking about above.
Have to check this but I'm not sure if this is anyhow related.
After reading the
> code I'm fully aware of the 32-bit pointer (I wouldn't call it a
> register, it's also just DMA memory)
Just general wording.
>> We've assumed that allocations are on the page or cache boundaries I'm
>> not who already assured me that this is the case.
>
> He lied.
Not just that, it was never confirmed this is the issue it was the
first place we started to look.
This code is there from time zero of the driver something like kernel
2.6.18 and the problem popped only in kernel 2.6.27 so what else has
changed.
>
>> I actually haven't
>> encountered this problem on X86_64 I even have the correct check.
>
> Are you using swiotlb? That's the only case I can think of where the
> alignment would always be satisfied.
>
Yes I have CONFIG_SWIOTLB=y in my config spec for X86_64, nevertheless
you've mentioned that the allocation problem is due to headroom
allocation in skb?!
>> What
>> I know there is difference in memory mapping in PPC64 as it has IOMMU.
>
> The powerpc IOMMU design isn't very peculiar. It maps 4k pages of
> machine memory into device space. If the memory pointer you're mapping
> isn't aligned, then the device memory pointer will not be either. I
> suspect the same happens on any iommu design, other than machines
> without an iommu of course, using swiotlb.
>
> And since the skb data is simply assigned with kmalloc, it surely won't
> be aligned on a 256 byte boundary. 256 is _huge_.
>
>> Hope this address the wrong command queue number bug but I'm not
>> sure.
>
> It certainly does for me.
>
>> There are other places with alignment requirement in the driver we
>> probably need to address as well.
>
> Haven't found any, but yeah, anything >16 byte alignment isn't
> guaranteed (I think, might be 8, might be 32, not exactly sure)
>
>
> Incidentally, where does the 36 bit limit come from? The RX has a 40 bit
> limit if you see that it uses 32 bit pointer that's shifted by 8 bits.
I'm checking with the HW guys
Tomas
On Tue, Nov 18, 2008 at 11:43 AM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-11-18 at 01:47 +0100, Johannes Berg wrote:
>
>> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
>> helped me in that he said something about pci-e alignment and mentioned
>> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
>> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
>> something the device would fail with
>
> Come to think of it, maybe it does. One common failure case seems to be
> "this goes wrong with an 11n AP, but not on a regular one", so one
> explanation for that would be that the hardware designers expect the
> driver to split up the DMA into multiple DMA descriptors at 4K
> boundaries, but iwlagn uses only one and undefined behaviour results.
> That's something you'll need to ask the HW designers though or look up
> in the datasheet.
You are off here. The constrain is just that address will fit in 32
bit register. I don't know why this constrain, but there is always a
reason.
We've assumed that allocations are on the page or cache boundaries I'm
not who already assured me that this is the case. I actually haven't
encountered this problem on X86_64 I even have the correct check. What
I know there is difference in memory mapping in PPC64 as it has IOMMU.
Hope this address the wrong command queue number bug but I'm not
sure.
There are other places with alignment requirement in the driver we
probably need to address as well.
Tomas
On Wed, 2008-11-19 at 14:24 +0200, Tomas Winkler wrote:
> On Wed, Nov 19, 2008 at 1:40 PM, Johannes Berg
> <[email protected]> wrote:
> > On Wed, 2008-11-19 at 11:21 +0200, Tomas Winkler wrote:
> >
> >> > Indeed. One would seem to be the buffer list itself, which also needs to
> >> > fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
> >> > 16-byte alignment which probably is always satisfied, but I'm not sure,
> >> > you'll need to research the alignment guarantees.
> >>
> >> Note these are allocated using persistent calls and not kmalloc I hope
> >> at least does are page alignment.
> >
> > Good point. On powerpc at least that uses alloc_pages internally.
>
> Not sure if if wouldn't be better just work directly with pages in RX?
Well, in theory yes, but it's not possible to have skb->data point to a
separate page rather than a kmalloc area. And while you could allocate
an skb with very little headroom and the rest of the skb data in the
fraglist with pages, mac80211 couldn't handle it.
johannes
On Wed, Nov 19, 2008 at 1:40 PM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-11-19 at 11:21 +0200, Tomas Winkler wrote:
>
>> > Indeed. One would seem to be the buffer list itself, which also needs to
>> > fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
>> > 16-byte alignment which probably is always satisfied, but I'm not sure,
>> > you'll need to research the alignment guarantees.
>>
>> Note these are allocated using persistent calls and not kmalloc I hope
>> at least does are page alignment.
>
> Good point. On powerpc at least that uses alloc_pages internally.
Not sure if if wouldn't be better just work directly with pages in RX?
Tomas
On Wed, 2008-11-19 at 14:09 +0100, Johannes Berg wrote:
> On Wed, 2008-11-19 at 15:02 +0200, Tomas Winkler wrote:
>
> > > Here's a thought: iwl_rx_queue_update_write_ptr doesn't synchronise the
> > > pointer rxq->bd memory to the device. So if you have a platform that's
> > > not cache coherent for IO devices (mine is) that might be a problem.
> >
> > Need to issue wmb() there or something like this?
>
> well rb is also dma memory, no? So you'd have to use dma_sync_ or
> something, just like with skb data, no?
or maybe not? I can't look into this today, sorry.
johannes
For the RX DMA fix for iwlwifi ("iwlagn: fix RX skb alignment") Luis
pointed out:
> aligned_dma_addr can obviously be > real_dma_addr at this point, what
> guarantees we can use it on our own whim?
I asked around, and he's right, there may be platforms that do not allow
passing such such an address to the DMA API functions. This patch
changes it by using the proper dma_sync_single_range_for_cpu API
invented for this purpose.
Cc: Luis R. Rodriguez <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
--- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-19 01:16:12.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-19 01:19:13.000000000 +0100
@@ -1229,9 +1229,11 @@ void iwl_rx_handle(struct iwl_priv *priv
rxq->queue[i] = NULL;
- pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->aligned_dma_addr,
- priv->hw_params.rx_buf_size,
- PCI_DMA_FROMDEVICE);
+ dma_sync_single_range_for_cpu(
+ &priv->pci_dev->dev, rxb->real_dma_addr,
+ rxb->aligned_dma_addr - rxb->real_dma_addr,
+ priv->hw_params.rx_buf_size,
+ PCI_DMA_FROMDEVICE);
pkt = (struct iwl_rx_packet *)rxb->skb->data;
/* Reclaim a command buffer only if this packet is a response
On Tue, 2008-11-18 at 09:57 +0800, Zhu Yi wrote:
> We do suspect the 256 alignment requirement will be an issue. Thus we do
> let people can reproduce this problem check with it. See comment #21,
> #22, #23 in bug
> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1703. The
> result is people still see this problem even the DMA address is 256
> bytes aligned.
Ok. Reading those comments, I wouldn't be too sure that he still ran
into it without the alignment warning, since he said he missed some
messages. Remember that the driver posts a whole bunch of RX buffers at
init time (I saw this, it filled way more than my screen when printing
just a line for each buffer) and then doesn't do much for quite a while
until you turn on wireless, and the driver might init earlier than other
things so he may have missed those messages. Anyway, hard to tell, hence
me asking if this patch fixes it.
Thanks for the review, you're right about the two missing +256.
Anyone who wants to try it before one of us posts a fixed version should
fix their version as below, I need to get some sleep.
> > @@ -302,7 +305,7 @@ void iwl_rx_queue_free(struct iwl_priv *
> > for (i = 0; i < RX_QUEUE_SIZE + RX_FREE_BUFFERS; i++) {
> > if (rxq->pool[i].skb != NULL) {
> > pci_unmap_single(priv->pci_dev,
> > - rxq->pool[i].dma_addr,
> > + rxq->pool[i].real_dma_addr,
> > priv->hw_params.rx_buf_size,
>
> priv->hw_params.rx_buf_size + 256.
>
> > PCI_DMA_FROMDEVICE);
> > dev_kfree_skb(rxq->pool[i].skb);
> > @@ -370,7 +373,7 @@ void iwl_rx_queue_reset(struct iwl_priv
> > * to an SKB, so we need to unmap and free potential storage */
> > if (rxq->pool[i].skb != NULL) {
> > pci_unmap_single(priv->pci_dev,
> > - rxq->pool[i].dma_addr,
> > + rxq->pool[i].real_dma_addr,
> > priv->hw_params.rx_buf_size,
> > PCI_DMA_FROMDEVICE);
> > priv->alloc_rxb_skb--;
>
> ditto.
johannes
[dropping a few CCs]
On Tue, 2008-11-18 at 16:13 +0200, Tomas Winkler wrote:
> There are other places with alignment requirement in the driver we
> probably need to address as well.
Indeed. One would seem to be the buffer list itself, which also needs to
fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
16-byte alignment which probably is always satisfied, but I'm not sure,
you'll need to research the alignment guarantees.
Another, different, question: I don't see where the driver tells the
hardware how large the buffers are it allocated. Can you point me to
that? You can switch 4k/8k ampdu but I don't see where you tell the
hardware.
johannes
On Wed, 2008-11-19 at 14:12 +0100, Johannes Berg wrote:
> On Wed, 2008-11-19 at 14:09 +0100, Johannes Berg wrote:
> > On Wed, 2008-11-19 at 15:02 +0200, Tomas Winkler wrote:
> >
> > > > Here's a thought: iwl_rx_queue_update_write_ptr doesn't synchronise the
> > > > pointer rxq->bd memory to the device. So if you have a platform that's
> > > > not cache coherent for IO devices (mine is) that might be a problem.
> > >
> > > Need to issue wmb() there or something like this?
> >
> > well rb is also dma memory, no? So you'd have to use dma_sync_ or
> > something, just like with skb data, no?
>
> or maybe not? I can't look into this today, sorry.
Ah, no, you don't have to because this is allocated with
pci_alloc_consistent. I'm getting confused, sorry.
johannes
> I'd like to ask anyone who is CC'ed (those are people I know ran into
> the bug) to try this patch.
Here's an updated version fixing the two errors pointed out by Zhu Yi.
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 6 +++---
drivers/net/wireless/iwlwifi/iwl-dev.h | 3 ++-
drivers/net/wireless/iwlwifi/iwl-rx.c | 23 +++++++++++++----------
3 files changed, 18 insertions(+), 14 deletions(-)
--- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-17 23:35:34.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-18 11:45:11.000000000 +0100
@@ -1229,7 +1229,7 @@ void iwl_rx_handle(struct iwl_priv *priv
rxq->queue[i] = NULL;
- pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->dma_addr,
+ pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->aligned_dma_addr,
priv->hw_params.rx_buf_size,
PCI_DMA_FROMDEVICE);
pkt = (struct iwl_rx_packet *)rxb->skb->data;
@@ -1282,8 +1282,8 @@ void iwl_rx_handle(struct iwl_priv *priv
rxb->skb = NULL;
}
- pci_unmap_single(priv->pci_dev, rxb->dma_addr,
- priv->hw_params.rx_buf_size,
+ pci_unmap_single(priv->pci_dev, rxb->real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
spin_lock_irqsave(&rxq->lock, flags);
list_add_tail(&rxb->list, &priv->rxq.rx_used);
--- everything.orig/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-11-11 01:00:58.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-11-18 11:45:11.000000000 +0100
@@ -89,7 +89,8 @@ extern struct iwl_cfg iwl5100_abg_cfg;
#define DEFAULT_LONG_RETRY_LIMIT 4U
struct iwl_rx_mem_buffer {
- dma_addr_t dma_addr;
+ dma_addr_t real_dma_addr;
+ dma_addr_t aligned_dma_addr;
struct sk_buff *skb;
struct list_head list;
};
--- everything.orig/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-11-18 01:50:05.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-11-18 11:46:05.000000000 +0100
@@ -204,7 +204,7 @@ int iwl_rx_queue_restock(struct iwl_priv
list_del(element);
/* Point to Rx buffer via next RBD in circular buffer */
- rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->dma_addr);
+ rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->aligned_dma_addr);
rxq->queue[rxq->write] = rxb;
rxq->write = (rxq->write + 1) & RX_QUEUE_MASK;
rxq->free_count--;
@@ -250,7 +250,7 @@ void iwl_rx_allocate(struct iwl_priv *pr
rxb = list_entry(element, struct iwl_rx_mem_buffer, list);
/* Alloc a new receive buffer */
- rxb->skb = alloc_skb(priv->hw_params.rx_buf_size,
+ rxb->skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
__GFP_NOWARN | GFP_ATOMIC);
if (!rxb->skb) {
if (net_ratelimit())
@@ -265,11 +265,14 @@ void iwl_rx_allocate(struct iwl_priv *pr
list_del(element);
/* Get physical address of RB/SKB */
- rxb->dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
- priv->hw_params.rx_buf_size, PCI_DMA_FROMDEVICE);
+ rxb->real_dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
+ priv->hw_params.rx_buf_size + 256, PCI_DMA_FROMDEVICE);
- /* RBD must be 256 bytes aligned and no more than 36 bits */
- BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
+ /* dma address must be no more than 36 bits */
+ BUG_ON(rxb->real_dma_addr & ~DMA_BIT_MASK(36));
+ /* and also 256 byte aligned! */
+ rxb->aligned_dma_addr = ALIGN(rxb->real_dma_addr, 256);
+ skb_reserve(rxb->skb, rxb->aligned_dma_addr - rxb->real_dma_addr);
list_add_tail(&rxb->list, &rxq->rx_free);
rxq->free_count++;
@@ -302,8 +305,8 @@ void iwl_rx_queue_free(struct iwl_priv *
for (i = 0; i < RX_QUEUE_SIZE + RX_FREE_BUFFERS; i++) {
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
- priv->hw_params.rx_buf_size,
+ rxq->pool[i].real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(rxq->pool[i].skb);
}
@@ -370,8 +373,8 @@ void iwl_rx_queue_reset(struct iwl_priv
* to an SKB, so we need to unmap and free potential storage */
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
- priv->hw_params.rx_buf_size,
+ rxq->pool[i].real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
priv->alloc_rxb_skb--;
dev_kfree_skb(rxq->pool[i].skb);
On Wed, Nov 19, 2008 at 1:36 PM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-11-19 at 11:15 +0200, Tomas Winkler wrote:
>
>> >> You are off here. The constrain is just that address will fit in 32
>> >> bit register. I don't know why this constrain, but there is always a
>> >> reason.
>> >
>> > There's also the PCI-E constraint of having to split up DMA at 4k
>> > boundaries, which is what I was talking about above.
>>
>> Have to check this but I'm not sure if this is anyhow related.
>
> It's not. I misunderstood how RX DMA works, but given that you actually
> have to allocate the buffers that large the HW has to support splitting.
>
>> Not just that, it was never confirmed this is the issue it was the
>> first place we started to look.
>> This code is there from time zero of the driver something like kernel
>> 2.6.18 and the problem popped only in kernel 2.6.27 so what else has
>> changed.
>
> Yeah, it's weird, and it doesn't even seem to solve it for everyone. So
> there must be another thing too.
It seems that your wrong command queue problem is no the same root
cause we see on x86 platforms they have just same phenomena, because
there is 0xff data on the RX buffer.
>
>
>> Yes I have CONFIG_SWIOTLB=y in my config spec for X86_64, nevertheless
>> you've mentioned that the allocation problem is due to headroom
>> allocation in skb?!
>
> Well, the skb "head" is _all_ of the skb data that is not part of an
> extra buffer. So skb->data is the "head" while the skb fraglist is
> referred to as the data. That can be a bit confusing. Anyway skb->data
> is just allocated with kmalloc and on my machine that has 128 byte
> alignment. The IOMMU maps pages, so doesn't change the alignment. If you
> are using swiotlb (just having it in the config isn't enough, check the
> kernel messages) then much DMA will be done with bounce buffer.
>
Thanks will look at it.
>> > Incidentally, where does the 36 bit limit come from? The RX has a 40 bit
>> > limit if you see that it uses 32 bit pointer that's shifted by 8 bits.
>>
>> I'm checking with the HW guys
>
> Seems that isn't necessary, there are some things like the rbb stats
> that are only shifted down by 4 bits, so they need a 36 bit mask. Bit
> unfortunate that this single allocations forces all into the mask, but I
> don't right now see how to just get that single one into 36 and the rest
> into 40.
I've checked and the 256 alignment is really just to support larger
memory range in further NICs
The penalty for this on window is just 256 not so big because there is
no memory ownership handling
Thanks
Tomas
On Tue, 2008-11-18 at 01:47 +0100, Johannes Berg wrote:
> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
> helped me in that he said something about pci-e alignment and mentioned
> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
> something the device would fail with
Come to think of it, maybe it does. One common failure case seems to be
"this goes wrong with an 11n AP, but not on a regular one", so one
explanation for that would be that the hardware designers expect the
driver to split up the DMA into multiple DMA descriptors at 4K
boundaries, but iwlagn uses only one and undefined behaviour results.
That's something you'll need to ask the HW designers though or look up
in the datasheet.
johannes
On Wed, 2008-11-19 at 11:15 +0200, Tomas Winkler wrote:
> >> You are off here. The constrain is just that address will fit in 32
> >> bit register. I don't know why this constrain, but there is always a
> >> reason.
> >
> > There's also the PCI-E constraint of having to split up DMA at 4k
> > boundaries, which is what I was talking about above.
>
> Have to check this but I'm not sure if this is anyhow related.
It's not. I misunderstood how RX DMA works, but given that you actually
have to allocate the buffers that large the HW has to support splitting.
> Not just that, it was never confirmed this is the issue it was the
> first place we started to look.
> This code is there from time zero of the driver something like kernel
> 2.6.18 and the problem popped only in kernel 2.6.27 so what else has
> changed.
Yeah, it's weird, and it doesn't even seem to solve it for everyone. So
there must be another thing too.
> Yes I have CONFIG_SWIOTLB=y in my config spec for X86_64, nevertheless
> you've mentioned that the allocation problem is due to headroom
> allocation in skb?!
Well, the skb "head" is _all_ of the skb data that is not part of an
extra buffer. So skb->data is the "head" while the skb fraglist is
referred to as the data. That can be a bit confusing. Anyway skb->data
is just allocated with kmalloc and on my machine that has 128 byte
alignment. The IOMMU maps pages, so doesn't change the alignment. If you
are using swiotlb (just having it in the config isn't enough, check the
kernel messages) then much DMA will be done with bounce buffer.
> > Incidentally, where does the 36 bit limit come from? The RX has a 40 bit
> > limit if you see that it uses 32 bit pointer that's shifted by 8 bits.
>
> I'm checking with the HW guys
Seems that isn't necessary, there are some things like the rbb stats
that are only shifted down by 4 bits, so they need a 36 bit mask. Bit
unfortunate that this single allocations forces all into the mask, but I
don't right now see how to just get that single one into 36 and the rest
into 40.
johannes
On Wed, Nov 19, 2008 at 2:48 AM, Johannes Berg
<[email protected]> wrote:
> [dropping a few CCs]
>
> On Tue, 2008-11-18 at 16:13 +0200, Tomas Winkler wrote:
>
>> There are other places with alignment requirement in the driver we
>> probably need to address as well.
>
> Indeed. One would seem to be the buffer list itself, which also needs to
> fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
> 16-byte alignment which probably is always satisfied, but I'm not sure,
> you'll need to research the alignment guarantees.
Note these are allocated using persistent calls and not kmalloc I hope
at least does are page alignment.
We have some alignment requirements also on TX side
>
> Another, different, question: I don't see where the driver tells the
> hardware how large the buffers are it allocated. Can you point me to
> that? You can switch 4k/8k ampdu but I don't see where you tell the
> hardware.
in iwl_rx_init
rb_size =
FH_RCSR_RX_CONFIG_REG_VAL_RB_SIZE_4K .. 8K .. can go up to 16K.
This amsdu size (not ampdu) influence the size of RB we choose
Tomas
On Wed, Nov 19, 2008 at 2:33 PM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-11-19 at 14:22 +0200, Tomas Winkler wrote:
>
>> > Yeah, it's weird, and it doesn't even seem to solve it for everyone. So
>> > there must be another thing too.
>>
>> It seems that your wrong command queue problem is no the same root
>> cause we see on x86 platforms they have just same phenomena, because
>> there is 0xff data on the RX buffer.
>
> Well we've seen at one report that it helps, on x86. And the alignment
> thing is definitely a bug, so fixing it had to help somewhere.
No doubt that it has to be fixed,
But yes,
> it looks like there's a different bug leading to the same symptoms
> elsewhere...
Yes we are tracking it
> Here's a thought: iwl_rx_queue_update_write_ptr doesn't synchronise the
> pointer rxq->bd memory to the device. So if you have a platform that's
> not cache coherent for IO devices (mine is) that might be a problem.
Need to issue wmb() there or something like this?
> johannes
>
> I'd like to ask anyone who is CC'ed (those are people I know ran into
> the bug) to try this patch.
And here's a version against v2.6.28-rc5 (build tested only)
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 6 +++---
drivers/net/wireless/iwlwifi/iwl-dev.h | 3 ++-
drivers/net/wireless/iwlwifi/iwl-rx.c | 26 +++++++++++++++++---------
3 files changed, 22 insertions(+), 13 deletions(-)
--- test-tree.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-18 11:51:49.000000000 +0100
+++ test-tree/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-11-18 11:52:54.000000000 +0100
@@ -1384,7 +1384,7 @@ void iwl_rx_handle(struct iwl_priv *priv
rxq->queue[i] = NULL;
- pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->dma_addr,
+ pci_dma_sync_single_for_cpu(priv->pci_dev, rxb->aligned_dma_addr,
priv->hw_params.rx_buf_size,
PCI_DMA_FROMDEVICE);
pkt = (struct iwl_rx_packet *)rxb->skb->data;
@@ -1436,8 +1436,8 @@ void iwl_rx_handle(struct iwl_priv *priv
rxb->skb = NULL;
}
- pci_unmap_single(priv->pci_dev, rxb->dma_addr,
- priv->hw_params.rx_buf_size,
+ pci_unmap_single(priv->pci_dev, rxb->real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
spin_lock_irqsave(&rxq->lock, flags);
list_add_tail(&rxb->list, &priv->rxq.rx_used);
--- test-tree.orig/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-10-23 23:10:47.000000000 +0200
+++ test-tree/drivers/net/wireless/iwlwifi/iwl-dev.h 2008-11-18 11:52:54.000000000 +0100
@@ -89,7 +89,8 @@ extern struct iwl_cfg iwl5100_abg_cfg;
#define DEFAULT_LONG_RETRY_LIMIT 4U
struct iwl_rx_mem_buffer {
- dma_addr_t dma_addr;
+ dma_addr_t real_dma_addr;
+ dma_addr_t aligned_dma_addr;
struct sk_buff *skb;
struct list_head list;
};
--- test-tree.orig/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-10-23 23:10:47.000000000 +0200
+++ test-tree/drivers/net/wireless/iwlwifi/iwl-rx.c 2008-11-18 11:54:13.000000000 +0100
@@ -204,7 +204,7 @@ int iwl_rx_queue_restock(struct iwl_priv
list_del(element);
/* Point to Rx buffer via next RBD in circular buffer */
- rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->dma_addr);
+ rxq->bd[rxq->write] = iwl_dma_addr2rbd_ptr(priv, rxb->aligned_dma_addr);
rxq->queue[rxq->write] = rxb;
rxq->write = (rxq->write + 1) & RX_QUEUE_MASK;
rxq->free_count--;
@@ -251,7 +251,7 @@ void iwl_rx_allocate(struct iwl_priv *pr
rxb = list_entry(element, struct iwl_rx_mem_buffer, list);
/* Alloc a new receive buffer */
- rxb->skb = alloc_skb(priv->hw_params.rx_buf_size,
+ rxb->skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
__GFP_NOWARN | GFP_ATOMIC);
if (!rxb->skb) {
if (net_ratelimit())
@@ -266,9 +266,17 @@ void iwl_rx_allocate(struct iwl_priv *pr
list_del(element);
/* Get physical address of RB/SKB */
- rxb->dma_addr =
- pci_map_single(priv->pci_dev, rxb->skb->data,
- priv->hw_params.rx_buf_size, PCI_DMA_FROMDEVICE);
+ rxb->real_dma_addr = pci_map_single(
+ priv->pci_dev,
+ rxb->skb->data,
+ priv->hw_params.rx_buf_size + 256,
+ PCI_DMA_FROMDEVICE);
+ /* dma address must be no more than 36 bits */
+ BUG_ON(rxb->real_dma_addr & ~DMA_BIT_MASK(36));
+ /* and also 256 byte aligned! */
+ rxb->aligned_dma_addr = ALIGN(rxb->real_dma_addr, 256);
+ skb_reserve(rxb->skb, rxb->aligned_dma_addr - rxb->real_dma_addr);
+
list_add_tail(&rxb->list, &rxq->rx_free);
rxq->free_count++;
}
@@ -300,8 +308,8 @@ void iwl_rx_queue_free(struct iwl_priv *
for (i = 0; i < RX_QUEUE_SIZE + RX_FREE_BUFFERS; i++) {
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
- priv->hw_params.rx_buf_size,
+ rxq->pool[i].real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(rxq->pool[i].skb);
}
@@ -354,8 +362,8 @@ void iwl_rx_queue_reset(struct iwl_priv
* to an SKB, so we need to unmap and free potential storage */
if (rxq->pool[i].skb != NULL) {
pci_unmap_single(priv->pci_dev,
- rxq->pool[i].dma_addr,
- priv->hw_params.rx_buf_size,
+ rxq->pool[i].real_dma_addr,
+ priv->hw_params.rx_buf_size + 256,
PCI_DMA_FROMDEVICE);
priv->alloc_rxb_skb--;
dev_kfree_skb(rxq->pool[i].skb);
On Wed, 2008-11-19 at 00:38 +0800, Thomas Witt wrote:
> Johannes Berg wrote:
> >> I'd like to ask anyone who is CC'ed (those are people I know ran into
> >> the bug) to try this patch.
> >
> > And here's a version against v2.6.28-rc5 (build tested only)
>
>
>
> That one fixes the Problem for me
Hey, you also said my patch fixed your problem in comment #42. ;-)
Johannes, FYI. From the last comment (#47) of bug
http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1703. The
problem is not totally solved.
Thanks,
-yi
On Wed, 2008-11-19 at 15:02 +0200, Tomas Winkler wrote:
> > Here's a thought: iwl_rx_queue_update_write_ptr doesn't synchronise the
> > pointer rxq->bd memory to the device. So if you have a platform that's
> > not cache coherent for IO devices (mine is) that might be a problem.
>
> Need to issue wmb() there or something like this?
well rb is also dma memory, no? So you'd have to use dma_sync_ or
something, just like with skb data, no?
johannes
On Tue, 2008-11-18 at 16:13 +0200, Tomas Winkler wrote:
> >> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
> >> helped me in that he said something about pci-e alignment and mentioned
> >> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
> >> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
> >> something the device would fail with
> >
> > Come to think of it, maybe it does. One common failure case seems to be
> > "this goes wrong with an 11n AP, but not on a regular one", so one
> > explanation for that would be that the hardware designers expect the
> > driver to split up the DMA into multiple DMA descriptors at 4K
> > boundaries, but iwlagn uses only one and undefined behaviour results.
> > That's something you'll need to ask the HW designers though or look up
> > in the datasheet.
>
> You are off here. The constrain is just that address will fit in 32
> bit register. I don't know why this constrain, but there is always a
> reason.
There's also the PCI-E constraint of having to split up DMA at 4k
boundaries, which is what I was talking about above. After reading the
code I'm fully aware of the 32-bit pointer (I wouldn't call it a
register, it's also just DMA memory)
> We've assumed that allocations are on the page or cache boundaries I'm
> not who already assured me that this is the case.
He lied.
> I actually haven't
> encountered this problem on X86_64 I even have the correct check.
Are you using swiotlb? That's the only case I can think of where the
alignment would always be satisfied.
> What
> I know there is difference in memory mapping in PPC64 as it has IOMMU.
The powerpc IOMMU design isn't very peculiar. It maps 4k pages of
machine memory into device space. If the memory pointer you're mapping
isn't aligned, then the device memory pointer will not be either. I
suspect the same happens on any iommu design, other than machines
without an iommu of course, using swiotlb.
And since the skb data is simply assigned with kmalloc, it surely won't
be aligned on a 256 byte boundary. 256 is _huge_.
> Hope this address the wrong command queue number bug but I'm not
> sure.
It certainly does for me.
> There are other places with alignment requirement in the driver we
> probably need to address as well.
Haven't found any, but yeah, anything >16 byte alignment isn't
guaranteed (I think, might be 8, might be 32, not exactly sure)
Incidentally, where does the 36 bit limit come from? The RX has a 40 bit
limit if you see that it uses 32 bit pointer that's shifted by 8 bits.
johannes
On Tue, Nov 18, 2008 at 2:52 AM, Johannes Berg
<[email protected]> wrote:
> @@ -265,11 +265,14 @@ void iwl_rx_allocate(struct iwl_priv *pr
> list_del(element);
>
> /* Get physical address of RB/SKB */
> - rxb->dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
> - priv->hw_params.rx_buf_size, PCI_DMA_FROMDEVICE);
> + rxb->real_dma_addr = pci_map_single(priv->pci_dev, rxb->skb->data,
> + priv->hw_params.rx_buf_size + 256, PCI_DMA_FROMDEVICE);
>
> - /* RBD must be 256 bytes aligned and no more than 36 bits */
> - BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
> + /* dma address must be no more than 36 bits */
> + BUG_ON(rxb->real_dma_addr & ~DMA_BIT_MASK(36));
> + /* and also 256 byte aligned! */
> + rxb->aligned_dma_addr = ALIGN(rxb->real_dma_addr, 256);
aligned_dma_addr can obviously be > real_dma_addr at this point, what
gaurantees we can use it on our own whim?
Luis
On Wed, Nov 19, 2008 at 2:28 PM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-11-19 at 14:24 +0200, Tomas Winkler wrote:
>> On Wed, Nov 19, 2008 at 1:40 PM, Johannes Berg
>> <[email protected]> wrote:
>> > On Wed, 2008-11-19 at 11:21 +0200, Tomas Winkler wrote:
>> >
>> >> > Indeed. One would seem to be the buffer list itself, which also needs to
>> >> > fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
>> >> > 16-byte alignment which probably is always satisfied, but I'm not sure,
>> >> > you'll need to research the alignment guarantees.
>> >>
>> >> Note these are allocated using persistent calls and not kmalloc I hope
>> >> at least does are page alignment.
>> >
>> > Good point. On powerpc at least that uses alloc_pages internally.
>>
>> Not sure if if wouldn't be better just work directly with pages in RX?
>
> Well, in theory yes, but it's not possible to have skb->data point to a
> separate page rather than a kmalloc area. And while you could allocate
> an skb with very little headroom and the rest of the skb data in the
> fraglist with pages, mac80211 couldn't handle it.
I will look how this is handled in Ethernet drivers there should be
some example with jumbo packets.
Tomas
On Tue, 2008-11-18 at 08:47 +0800, Johannes Berg wrote:
> [patch below, sorry for the long list of CCs]
>
> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
> helped me in that he said something about pci-e alignment and mentioned
> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
> something the device would fail with, but when I looked into the
> function for a minute anyway I stumbled over this little gem:
>
> BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
OK, it should be (~DMA_BIT_MASK(36) | 0xff). My bad.
> Clearly, that is a totally bogus check, one would hope the compiler
> removes it entirely. (Think about it)
Sure.
> After fixing it, I obviously ran into it, nothing guarantees the
> alignment the way you want it, because of the way skbs and their
> headroom are allocated. I won't explain that here nor double-check that
> I'm right, that goes beyond what most of the CC'ed people care about.
We do suspect the 256 alignment requirement will be an issue. Thus we do
let people can reproduce this problem check with it. See comment #21,
#22, #23 in bug
http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1703. The
result is people still see this problem even the DMA address is 256
bytes aligned.
> So then I came up with the patch below, and so far my system has
> survived minutes with 64K pages, when it would previously fail in
> seconds. And I haven't seen a single instance of the TX bug either. But
> when you see the patch it'll be pretty obvious to you why.
Good. Your patch is generally correct. At least, it helps to enforce our
alignment requirement. But whether it can resolve the problem (I really
hope so) or not, let's wait and see.
> This should fix the following reported kernel bugs:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=11596
> http://bugzilla.kernel.org/show_bug.cgi?id=11393
> http://bugzilla.kernel.org/show_bug.cgi?id=11983
>
> I haven't checked if there are any elsewhere, but I suppose RHBZ will
> have a few instances too...
>
> I'd like to ask anyone who is CC'ed (those are people I know ran into
> the bug) to try this patch.
>
> I am convinced that this patch is correct in spirit, but I haven't
> understood why, for example, there are so many unmap calls. I'm not
> entirely convinced that this is the only bug leading to the TX reply
> errors.
>
> Signed-off-by: Johannes Berg <[email protected]>
[...]
> @@ -302,7 +305,7 @@ void iwl_rx_queue_free(struct iwl_priv *
> for (i = 0; i < RX_QUEUE_SIZE + RX_FREE_BUFFERS; i++) {
> if (rxq->pool[i].skb != NULL) {
> pci_unmap_single(priv->pci_dev,
> - rxq->pool[i].dma_addr,
> + rxq->pool[i].real_dma_addr,
> priv->hw_params.rx_buf_size,
priv->hw_params.rx_buf_size + 256.
> PCI_DMA_FROMDEVICE);
> dev_kfree_skb(rxq->pool[i].skb);
> @@ -370,7 +373,7 @@ void iwl_rx_queue_reset(struct iwl_priv
> * to an SKB, so we need to unmap and free potential storage */
> if (rxq->pool[i].skb != NULL) {
> pci_unmap_single(priv->pci_dev,
> - rxq->pool[i].dma_addr,
> + rxq->pool[i].real_dma_addr,
> priv->hw_params.rx_buf_size,
> PCI_DMA_FROMDEVICE);
> priv->alloc_rxb_skb--;
ditto.
Thanks,
-yi
On Wed, 2008-11-19 at 11:21 +0200, Tomas Winkler wrote:
> > Indeed. One would seem to be the buffer list itself, which also needs to
> > fit within a 40-bit mask with 256 alignment (rxq->bd), the rb_stts needs
> > 16-byte alignment which probably is always satisfied, but I'm not sure,
> > you'll need to research the alignment guarantees.
>
> Note these are allocated using persistent calls and not kmalloc I hope
> at least does are page alignment.
Good point. On powerpc at least that uses alloc_pages internally.
> We have some alignment requirements also on TX side
>
> >
> > Another, different, question: I don't see where the driver tells the
> > hardware how large the buffers are it allocated. Can you point me to
> > that? You can switch 4k/8k ampdu but I don't see where you tell the
> > hardware.
>
> in iwl_rx_init
> rb_size =
> FH_RCSR_RX_CONFIG_REG_VAL_RB_SIZE_4K .. 8K .. can go up to 16K.
> This amsdu size (not ampdu) influence the size of RB we choose
Thanks. Yeah, ampdus are deaggregated already, my mistake.
johannes
Johannes Berg wrote:
>> I'd like to ask anyone who is CC'ed (those are people I know ran into
>> the bug) to try this patch.
>
> And here's a version against v2.6.28-rc5 (build tested only)
That one fixes the Problem for me,
Thomas.
On Wed, 2008-11-19 at 14:22 +0200, Tomas Winkler wrote:
> > Yeah, it's weird, and it doesn't even seem to solve it for everyone. So
> > there must be another thing too.
>
> It seems that your wrong command queue problem is no the same root
> cause we see on x86 platforms they have just same phenomena, because
> there is 0xff data on the RX buffer.
Well we've seen at one report that it helps, on x86. And the alignment
thing is definitely a bug, so fixing it had to help somewhere. But yes,
it looks like there's a different bug leading to the same symptoms
elsewhere...
Here's a thought: iwl_rx_queue_update_write_ptr doesn't synchronise the
pointer rxq->bd memory to the device. So if you have a platform that's
not cache coherent for IO devices (mine is) that might be a problem.
johannes
Reinette,
> Above you mention that this patch fixes bug #11596. Could you please
> explain how this patch addresses that problem?
That looks like a mistake, I don't think it can fix that bug. I probably
pasted the wrong URL, I had a whole bunch of iwlwifi bugs open
everywhere around. I apologise for any problems caused by this, was this
bug closed erroneously?
johannes
On Mon, 2008-12-01 at 11:37 -0800, Johannes Berg wrote:
> Reinette,
>
> > Above you mention that this patch fixes bug #11596. Could you please
> > explain how this patch addresses that problem?
>
> That looks like a mistake, I don't think it can fix that bug. I probably
> pasted the wrong URL, I had a whole bunch of iwlwifi bugs open
> everywhere around. I apologise for any problems caused by this, was this
> bug closed erroneously?
No problem was caused. The kernel.org bug was closed because one of the
reporters was unable to reproduce the issue after upgrading the kernel.
Unfortunately no particular patch was identified that specifically
addresses the problem.
The bug in our system is still open because we still see the issue in
our test environment. We will keep digging.
Reinette
Johannes,
On Mon, 2008-11-17 at 16:47 -0800, Johannes Berg wrote:
> [patch below, sorry for the long list of CCs]
>
> So I dug deeper into the DMA problems I had with iwlagn and a kind soul
> helped me in that he said something about pci-e alignment and mentioned
> the iwl_rx_allocate function to check for crossing 4KB boundaries. Since
> there's 8KB A-MPDU support, crossing 4k boundaries didn't seem like
> something the device would fail with, but when I looked into the
> function for a minute anyway I stumbled over this little gem:
>
> BUG_ON(rxb->dma_addr & (~DMA_BIT_MASK(36) & 0xff));
>
> Clearly, that is a totally bogus check, one would hope the compiler
> removes it entirely. (Think about it)
>
> After fixing it, I obviously ran into it, nothing guarantees the
> alignment the way you want it, because of the way skbs and their
> headroom are allocated. I won't explain that here nor double-check that
> I'm right, that goes beyond what most of the CC'ed people care about.
>
> So then I came up with the patch below, and so far my system has
> survived minutes with 64K pages, when it would previously fail in
> seconds. And I haven't seen a single instance of the TX bug either. But
> when you see the patch it'll be pretty obvious to you why.
>
> This should fix the following reported kernel bugs:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=11596
> http://bugzilla.kernel.org/show_bug.cgi?id=11393
> http://bugzilla.kernel.org/show_bug.cgi?id=11983
>
> I haven't checked if there are any elsewhere, but I suppose RHBZ will
> have a few instances too...
>
> I'd like to ask anyone who is CC'ed (those are people I know ran into
> the bug) to try this patch.
>
> I am convinced that this patch is correct in spirit, but I haven't
> understood why, for example, there are so many unmap calls. I'm not
> entirely convinced that this is the only bug leading to the TX reply
> errors.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
Above you mention that this patch fixes bug #11596. Could you please
explain how this patch addresses that problem? A short summary of the
bug is that no data (ping packets) can be transmitted while station is
associated. We have, what appears to be, a similar bug in our bug
tracking system
(http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1807 ), but
this patch was not able to resolve this issue.
Bug #11596 was recently closed after the user tested with 2.6.27.4,
which does not have this patch.
Thank you
Reinette