Re: [PATCH] b43: Refresh RX poison on buffer recycling
On Mar 30, 2009, at 11:35 PM, Francesco Gringoli wrote:
On Mar 28, 2009, at 12:41 AM, Michael Buesch wrote:
The RX buffer poison needs to be refreshed, if we recycle an RX
buffer,
because it might be (partially) overwritten by some DMA operations.
Cc: sta...@kernel.org
Cc: Francesco Gringoli francesco.gring...@ing.unibs.it
Signed-off-by: Michael Buesch m...@bu3sch.de
---
Francesco, please stresstest this on top of the other patch that
adds poisoning.
Hi Michael,
great work! No more crashes with the two patches. I will continue
stress testing anyway but it seems stable.
Hi Michael,
I run the patched kernel for some time and, though it is stable and
never crashes, there are still (few) some strange rx events. I will
refer to them as Wrong Frame Type I, Type II, and III. Please note
however, that we can observe them ONLY when the FCSFAIL bit is set in
the mac_ctrl_high (at least in my setup).
- Type I: plcp length mismatch
These frames have a plcp that seems to be correct (I mean, the first
two bytes appear to be taken from a valid plcp), the padding reported
by the firmware in this cases is correct too (e.g. the padding always
point to the byte where the valid plcp seems to start). HOWEVER, the
length of such frames is different than the length encoded in their
plcp. In all these frames the B43_RX_MAC_FCSERR bit is not set, though
these frames will fail a crc check. We should check the plcp matches
the skb length and manually set the RX_FLAG_FAILED_FCS_CRC bit in the
status field so that mac80211 will skip these frames.
- Type II: fcs is wrong
These frames have a correct plcp that matches their skb length. Their
FCS however is wrong! but the B43_RX_MAC_FCSERR is not set. Again we
should manually set the RX_FLAG_FAILED_FCS_CRC bit in the status field
so that mac80211 will skip these frames. I believe that these frames
are nothing more than Type I but the broken length collided with their
actual length.
- Type III: plcp is not a... plcp
These frames have a plcp that is not a plcp, in the sense that the
first two bytes (with both padding 0 or 2) does not represent any
possible plcp.
I attach a patch to correctly set the RX_FLAG_FAILED_FCS_CRC bit in
the status field on such situations so that such frames are not passed
to the upper layers.
Cheers,
-FG
Index: wireless-testing/drivers/net/wireless/b43/xmit.c
===
--- drivers/net/wireless/b43/xmit.c 2009-03-26 19:41:53.0 +0100
+++ drivers/net/wireless/b43/xmit.c 2009-03-27 20:55:31.0 +0100
@@ -27,6 +27,7 @@
*/
+#include linux/crc32.h
#include xmit.h
#include phy_common.h
#include dma.h
@@ -560,6 +562,67 @@
goto drop;
}
plcp = (struct b43_plcp_hdr6 *)(skb-data + padding);
+
+ if ((dev-wl-filter_flags FIF_FCSFAIL) !(macstat
B43_RX_MAC_FCSERR)) {
+ int mismatch = 0;
+ int skb_len = skb-len - 6 - padding;
+ u8* plcp_data = (u8*) plcp;
+ if (phystat0 B43_RX_PHYST0_OFDM) {
+ int pkt_len = plcp_data[0] |
+ plcp_data[1] 8 |
+ plcp_data[2] 16 |
+ plcp_data[3] 24;
+ pkt_len = 5;
+ pkt_len = 0xFFF;
+ if(pkt_len != skb_len) {
+ mismatch = 1;
+ }
+ }
+ else {
+ int speed;
+ int len1 = (plcp_data[3] 8) | plcp_data[2];
+ int len2;
+ switch(plcp_data[0]) {
+ case 0x0A:
+ speed = 2;
+ break;
+ case 0x14:
+ speed = 4;
+ break;
+ case 0x37:
+ speed = 11;
+ break;
+ case 0x6E:
+ speed = 22;
+ break;
+ default:
+ speed = 1;
+ break;
+ }
+ len2 = skb_len * 16 / speed;
+ if((skb_len * 16 % speed) 0)
+ len2++;
+
+ if(len1 != len2) {
+ mismatch = 1;
+ }
+ }
+ if(mismatch) {
+ dev-wl-ieee_stats.dot11FCSErrorCount++;
+ macstat |= B43_RX_MAC_FCSERR;
+ status.flag |= RX_FLAG_FAILED_FCS_CRC;
+ plcp_data = (u8*) (struct b43_plcp_hdr6 *)(skb-data);
+ b43dbg(dev-wl, RX: padding or plcp
Re: [PATCH] b43: Refresh RX poison on buffer recycling
On Sun, Apr 5, 2009 at 2:01 PM, Francesco Gringoli francesco.gring...@ing.unibs.it wrote: On Mar 30, 2009, at 11:35 PM, Francesco Gringoli wrote: On Mar 28, 2009, at 12:41 AM, Michael Buesch wrote: The RX buffer poison needs to be refreshed, if we recycle an RX buffer, because it might be (partially) overwritten by some DMA operations. Cc: sta...@kernel.org Cc: Francesco Gringoli francesco.gring...@ing.unibs.it Signed-off-by: Michael Buesch m...@bu3sch.de --- Francesco, please stresstest this on top of the other patch that adds poisoning. Hi Michael, great work! No more crashes with the two patches. I will continue stress testing anyway but it seems stable. Hi Michael, I run the patched kernel for some time and, though it is stable and never crashes, there are still (few) some strange rx events. I will refer to them as Wrong Frame Type I, Type II, and III. Please note however, that we can observe them ONLY when the FCSFAIL bit is set in the mac_ctrl_high (at least in my setup). - Type I: plcp length mismatch These frames have a plcp that seems to be correct (I mean, the first two bytes appear to be taken from a valid plcp), the padding reported by the firmware in this cases is correct too (e.g. the padding always point to the byte where the valid plcp seems to start). HOWEVER, the length of such frames is different than the length encoded in their plcp. In all these frames the B43_RX_MAC_FCSERR bit is not set, though these frames will fail a crc check. We should check the plcp matches the skb length and manually set the RX_FLAG_FAILED_FCS_CRC bit in the status field so that mac80211 will skip these frames. - Type II: fcs is wrong These frames have a correct plcp that matches their skb length. Their FCS however is wrong! but the B43_RX_MAC_FCSERR is not set. Again we should manually set the RX_FLAG_FAILED_FCS_CRC bit in the status field so that mac80211 will skip these frames. I believe that these frames are nothing more than Type I but the broken length collided with their actual length. - Type III: plcp is not a... plcp These frames have a plcp that is not a plcp, in the sense that the first two bytes (with both padding 0 or 2) does not represent any possible plcp. I attach a patch to correctly set the RX_FLAG_FAILED_FCS_CRC bit in the status field on such situations so that such frames are not passed to the upper layers. Cheers, -FG Your patch was mangled by your email client. Maybe you should inline your patches and attach a copy of it before sending in the future. Regards, David Ellingsworth ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43: Refresh RX poison on buffer recycling
On Sun, Apr 5, 2009 at 3:54 PM, Francesco Gringoli francesco.gring...@ing.unibs.it wrote: On Apr 5, 2009, at 8:58 PM, Michael Buesch wrote: On Sunday 05 April 2009 20:01:22 Francesco Gringoli wrote: On Mar 30, 2009, at 11:35 PM, Francesco Gringoli wrote: On Mar 28, 2009, at 12:41 AM, Michael Buesch wrote: Hi Michael, I think this really is completely getting overengineered by now. I'm not going to apply such a patch unless you tell me why it's needed. Having such an incredible mechanism for an absolute corner case that happens once in a billion frames but doesn't harm anybody is not really acceptable. No problem :-) I simply sent the patch I'm using in my test environment where I get this behavior for the 0.1% of the received frames when FCSFAIL is set. Note that here we collect traces for experiments with 802.11 protocol, so we need this kind of patches. I understand that very few of us are doing such kind of experiments and users are not, I simply sent a comment about these devices. It may improve knowledge about them. Cheers, -FG While I more or less agree with Michael on this, I can also see that these changes may be useful in some environments. Thus, rather than always adding unneeded complexity to the driver, it might be better if changes like these could be placed under a general network define kernel option. Then users wishing to have strict frame checks when FCSFAIL is set have the option to do so. Regards, David Ellingsworth ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43: Refresh RX poison on buffer recycling
On Mar 28, 2009, at 12:41 AM, Michael Buesch wrote:
The RX buffer poison needs to be refreshed, if we recycle an RX
buffer,
because it might be (partially) overwritten by some DMA operations.
Cc: sta...@kernel.org
Cc: Francesco Gringoli francesco.gring...@ing.unibs.it
Signed-off-by: Michael Buesch m...@bu3sch.de
---
Francesco, please stresstest this on top of the other patch that
adds poisoning.
Hi Michael,
great work! No more crashes with the two patches. I will continue
stress testing anyway but it seems stable.
I have one more question: the hardware seems to allow frames that are
longer than 2352 bytes. If we monitor the firmware during receiving we
get up to 0x1005 bytes long frames. When such frames arrives, the
kernel drops them as the The data did not fit into one descriptor
buffer and is split over multiple buffers. I tried to increase
B43_DMA0_RX_BUFFERSIZE up to 0x1006 but I get problems with dma and
the driver keeps restarting the hardware forever. What is wrong with
increasing this value above IEEE80211_MAX_FRAME_LEN?
Many thanks,
Cheers
-FG
John, please queue as bugfix.
Index: wireless-testing/drivers/net/wireless/b43/dma.c
===
--- wireless-testing.orig/drivers/net/wireless/b43/dma.c 2009-03-27
23:15:36.0 +0100
+++ wireless-testing/drivers/net/wireless/b43/dma.c 2009-03-27
23:30:17.0 +0100
@@ -1503,20 +1503,16 @@ static void dma_rx(struct b43_dmaring *r
len = le16_to_cpu(rxhdr-frame_len);
} while (len == 0 i++ 5);
if (unlikely(len == 0)) {
- /* recycle the descriptor buffer. */
- sync_descbuffer_for_device(ring, meta-dmaaddr,
-ring-rx_buffersize);
- goto drop;
+ dmaaddr = meta-dmaaddr;
+ goto drop_recycle_buffer;
}
}
if (unlikely(b43_rx_buffer_is_poisoned(ring, skb))) {
/* Something went wrong with the DMA.
* The device did not touch the buffer and did not overwrite
the
poison. */
b43dbg(ring-dev-wl, DMA RX: Dropping poisoned buffer.\n);
- /* recycle the descriptor buffer. */
- sync_descbuffer_for_device(ring, meta-dmaaddr,
-ring-rx_buffersize);
- goto drop;
+ dmaaddr = meta-dmaaddr;
+ goto drop_recycle_buffer;
}
if (unlikely(len ring-rx_buffersize)) {
/* The data did not fit into one descriptor buffer
@@ -1530,6 +1526,7 @@ static void dma_rx(struct b43_dmaring *r
while (1) {
desc = ops-idx2desc(ring, *slot, meta);
/* recycle the descriptor buffer. */
+ b43_poison_rx_buffer(ring, meta-skb);
sync_descbuffer_for_device(ring, meta-dmaaddr,
ring-rx_buffersize);
*slot = next_slot(ring, *slot);
@@ -1548,8 +1545,7 @@ static void dma_rx(struct b43_dmaring *r
err = setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC);
if (unlikely(err)) {
b43dbg(ring-dev-wl, DMA RX: setup_rx_descbuffer() failed\n);
- sync_descbuffer_for_device(ring, dmaaddr, ring-rx_buffersize);
- goto drop;
+ goto drop_recycle_buffer;
}
unmap_descbuffer(ring, dmaaddr, ring-rx_buffersize, 0);
@@ -1559,6 +1555,11 @@ static void dma_rx(struct b43_dmaring *r
b43_rx(ring-dev, skb, rxhdr);
drop:
return;
+
+drop_recycle_buffer:
+ /* Poison and recycle the RX buffer. */
+ b43_poison_rx_buffer(ring, skb);
+ sync_descbuffer_for_device(ring, dmaaddr, ring-rx_buffersize);
}
void b43_dma_rx(struct b43_dmaring *ring)
--
Greetings, Michael.
---
Francesco Gringoli, PhD - Assistant Professor
Dept. of Electrical Engineering for Automation
University of Brescia
via Branze, 38
25123 Brescia
ITALY
Ph: ++39.030.3715843
FAX: ++39.030.380014
WWW: http://www.ing.unibs.it/~gringoli
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43: Refresh RX poison on buffer recycling
On Monday 30 March 2009 23:35:34 Francesco Gringoli wrote: I have one more question: the hardware seems to allow frames that are longer than 2352 bytes. If we monitor the firmware during receiving we get up to 0x1005 bytes long frames. When such frames arrives, the kernel drops them as the The data did not fit into one descriptor buffer and is split over multiple buffers. I tried to increase B43_DMA0_RX_BUFFERSIZE up to 0x1006 but I get problems with dma and the driver keeps restarting the hardware forever. What is wrong with increasing this value above IEEE80211_MAX_FRAME_LEN? Well... First thing is that I think the hardware wasn't ever tested with frames IEEE80211_MAX_FRAME_LEN. So there might be silicon bugs. The maximum number of bytes one descriptor can carry is 8191 bytes (not including RX headers and padding. That's 30 bytes). Third thing is that the buffer must not cross a page boundary. So that is 4096 bytes on most machines. So in practice the 4096 bytes boundary (minus 30 bytes for headers/padding) is upper bound for B43_DMA0_RX_BUFFERSIZE. -- Greetings, Michael. ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
[PATCH] b43: Refresh RX poison on buffer recycling
The RX buffer poison needs to be refreshed, if we recycle an RX buffer,
because it might be (partially) overwritten by some DMA operations.
Cc: sta...@kernel.org
Cc: Francesco Gringoli francesco.gring...@ing.unibs.it
Signed-off-by: Michael Buesch m...@bu3sch.de
---
Francesco, please stresstest this on top of the other patch that adds poisoning.
John, please queue as bugfix.
Index: wireless-testing/drivers/net/wireless/b43/dma.c
===
--- wireless-testing.orig/drivers/net/wireless/b43/dma.c2009-03-27
23:15:36.0 +0100
+++ wireless-testing/drivers/net/wireless/b43/dma.c 2009-03-27
23:30:17.0 +0100
@@ -1503,20 +1503,16 @@ static void dma_rx(struct b43_dmaring *r
len = le16_to_cpu(rxhdr-frame_len);
} while (len == 0 i++ 5);
if (unlikely(len == 0)) {
- /* recycle the descriptor buffer. */
- sync_descbuffer_for_device(ring, meta-dmaaddr,
- ring-rx_buffersize);
- goto drop;
+ dmaaddr = meta-dmaaddr;
+ goto drop_recycle_buffer;
}
}
if (unlikely(b43_rx_buffer_is_poisoned(ring, skb))) {
/* Something went wrong with the DMA.
* The device did not touch the buffer and did not overwrite
the poison. */
b43dbg(ring-dev-wl, DMA RX: Dropping poisoned buffer.\n);
- /* recycle the descriptor buffer. */
- sync_descbuffer_for_device(ring, meta-dmaaddr,
- ring-rx_buffersize);
- goto drop;
+ dmaaddr = meta-dmaaddr;
+ goto drop_recycle_buffer;
}
if (unlikely(len ring-rx_buffersize)) {
/* The data did not fit into one descriptor buffer
@@ -1530,6 +1526,7 @@ static void dma_rx(struct b43_dmaring *r
while (1) {
desc = ops-idx2desc(ring, *slot, meta);
/* recycle the descriptor buffer. */
+ b43_poison_rx_buffer(ring, meta-skb);
sync_descbuffer_for_device(ring, meta-dmaaddr,
ring-rx_buffersize);
*slot = next_slot(ring, *slot);
@@ -1548,8 +1545,7 @@ static void dma_rx(struct b43_dmaring *r
err = setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC);
if (unlikely(err)) {
b43dbg(ring-dev-wl, DMA RX: setup_rx_descbuffer() failed\n);
- sync_descbuffer_for_device(ring, dmaaddr, ring-rx_buffersize);
- goto drop;
+ goto drop_recycle_buffer;
}
unmap_descbuffer(ring, dmaaddr, ring-rx_buffersize, 0);
@@ -1559,6 +1555,11 @@ static void dma_rx(struct b43_dmaring *r
b43_rx(ring-dev, skb, rxhdr);
drop:
return;
+
+drop_recycle_buffer:
+ /* Poison and recycle the RX buffer. */
+ b43_poison_rx_buffer(ring, skb);
+ sync_descbuffer_for_device(ring, dmaaddr, ring-rx_buffersize);
}
void b43_dma_rx(struct b43_dmaring *ring)
--
Greetings, Michael.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
