p54pci: skb_over_panic, soft lockup, stall under flood

Hi,

Sorry for my lack of experience in all aspects - first time submitting!!!

In trying to get p54pci driver to be stable on my platform and hardware
- here is a generic patch that seems to accomplish that. Since the ViewSonic V210 uses the IT8152 pci bridge - some attention was needed to get dma related allocation in the first physical 64M. I have verified that the dma related allocation is in the first 64M and dmabounce is not being used - just for those wondering if that was part of the problems.

Platform: ViewSonic V210 arm pxa255
Kernel 2.6.30.5 eabi
Wireless Drivers from compat-wireless-2009-09-30 and what I applied the below patch to. Firmware used: FW rev 2.13.12.0 - Softmac protocol 5.9

Wireless card: GemTek WL-850FJB minipci card.

phy0: p54 detected a LM86 firmware
p54: rx_mtu reduced from 3240 to 2376
phy0: FW rev 2.13.12.0 - Softmac protocol 5.9
phy0: cryptographic accelerator WEP:YES, TKIP:YES, CCMP:YES
phy0: hwaddr 00:90:4b:c1:06:bc, MAC:isl3890 RF:Frisbee
phy0: Selected rate control algorithm 'minstrel'

device pci info (lspci -v):

00:06.0 Network controller: Intersil Corporation ISL3886 [Prism Javelin/Prism Xbow] (rev 01)
Subsystem: Intersil Corporation Device 0000
Flags: bus master, medium devsel, latency 56, IRQ 217
Memory at 11000000 (32-bit, non-prefetchable) [size=8K]
Capabilities: [dc] Power Management version 1
Kernel driver in use: p54pci
Kernel modules: prism54, p54pci

Reasons for patch was to solve the below problems.

1. p54p_check_rx_ring - skb_over_panic: Under a ping flood or just left running for a bit would panic with a skb_over_panic. Investigation showed for some odd reason the device/firmware instead of writing a length in the data rx_ring (desc->len) had instead written the whole dma address (host->host_addr) into location of the len/flag (host->len and host->flags) spot and the same dma address that was in the ring. Added the following condition in p54p_check_rx_ring to trap that condition and trim the skb reset the len and flags only. By the way - I used haret to see if it I could prove it happening under wince - located the dma memory that was being used for rings - and also happening under windows ce with the len/flag being set to the same as the host dma. Scanning the ring at 1000 times per second (I think) In a flood or iperf. Would see an occasional len/flag location get set to the same host address in that ring - may only happen a few times every minute. Under normal operation maybe a few times a day.

   if(unlikely(len == (desc->host_addr & 0xffff)
   && (desc->flags == ((desc->host_addr & 0xffff0000) >> 16))) )

2. p54p_refill_rx_ring - eventual stall: Has the potential in very busy (flood) to over run the last rx data processed ring index corrupting the next rings - causing some havoc of getting some 13 indexes difference between priv->rx_idx_data and ring_control host_idx on a 8 index ring. This appears to eventually fill up the TX queue - returning a -ENOSPC in p54_assign_address (txrx.c) because of ring corruption missing some TX releases. Changed p54p_refill_rx_ring to take a index parm and use that as the last processed ring index - instead of the using the ring_control device_idx.

3. p54p_check_rx_ring - eventual stall: On ping flood - Control P54_CONTROL_TYPE_TXDONE rx packets that are skb reused - seem to cause a
problem on the next time around with the same index. Even though the
length was not the same was still being seen as a P54_CONTROL_TYPE_TXDONE packet again. Side affects varied - one being the main end result same as the #2 listed above TX not being released and returning a -ENOSPC in p54_assign_address (txrx.c) - stall. Problem went away if did not reuse the skb but unmap it and dev_kfree_skb if return was zero from p54_rx. Still unclear why this would be - but had no problems with patch afterwards.

4. p54p_check_rx_ring - soft lockup in p54p_refill_rx_ring. This only occurred when 5 minute iperf on a fast wireless network - Or 1 to 2 days of unit left up. Discovered that the device had lost it's mind and set the ring_control->device_index[ring_index] exactly 0xFF or 255 less than it should be (ram issue??) don't know. Happens on three of my devices the same way. If left to continue - the p54p_refill_rx_ring while loop goes negative and soft lockup. Trap and return if device_idx - (*index) greater than ring_index. Error is only tripped the one time - meaning the next time p54p_check_rx_ring is called the device index is back to what it should have been.

5. p54p_open - 1 out of 10 boots will produce device does not
respond! or Cannot boot firmware!. Minor - but frustrating all the
same.
Always rmmod p54pci and then modprobe p54pci works. It seems if get a error on p54p_open trying again works. And if p54_read_eeprom fails - trying again works.

The below was applied to compat-wireless-2009-09-30:

Thanks,

Quintin.

Signed-off-by Quintin Pitts <[email protected]>

---

drivers/net/wireless/p54/p54pci.c +6 -2

@@ -131,7 +131,7 @@ static int p54p_upload_firmware(struct i
 
 static void p54p_refill_rx_ring(struct ieee80211_hw *dev,
 	int ring_index, struct p54p_desc *ring, u32 ring_limit,
-	struct sk_buff **rx_buf)
+	struct sk_buff **rx_buf, u32 index)
 {
 	struct p54p_priv *priv = dev->priv;
 	struct p54p_ring_control *ring_control = priv->ring_control;

@@ -139,7 +139,11 @@ static void p54p_refill_rx_ring(struct i
 
 	idx = le32_to_cpu(ring_control->host_idx[ring_index]);
 	limit = idx;
-	limit -= le32_to_cpu(ring_control->device_idx[ring_index]);
+/*
+ *           Use last processed index instead of device_idx
+ *           so we don't corrupt our ring 
+ */
+	limit -= le32_to_cpu(index);
 	limit = ring_limit - limit;
 
 	i = idx % ring_limit;

drivers/net/wireless/p54/p54pci.c +20 -11

 					 le32_to_cpu(desc->host_addr),
 					 priv->common.rx_mtu + 32,
 					 PCI_DMA_FROMDEVICE);
-			rx_buf[i] = NULL;
-			desc->host_addr = 0;
-		} else {
-			skb_trim(skb, 0);
-			desc->len = cpu_to_le16(priv->common.rx_mtu + 32);
-		}
+		if(ret==0)
+			dev_kfree_skb(skb);
+		rx_buf[i] = NULL;
+		desc->host_addr = 0;
+		} /* end of desc->len skb corrupt crash test */
 
 		i++;
 		i %= ring_limit;
 	}
 
-	p54p_refill_rx_ring(dev, ring_index, ring, ring_limit, rx_buf);
+	p54p_refill_rx_ring(dev, ring_index, ring, ring_limit, rx_buf, (*index));
 }
 
 /* caller must hold priv->lock */

@@ -428,10 +464,10 @@ static int p54p_open(struct ieee80211_hw
 	priv->rx_idx_mgmt = priv->tx_idx_mgmt = 0;
 
 	p54p_refill_rx_ring(dev, 0, priv->ring_control->rx_data,
-		ARRAY_SIZE(priv->ring_control->rx_data), priv->rx_buf_data);
+		ARRAY_SIZE(priv->ring_control->rx_data), priv->rx_buf_data, 0);
 
 	p54p_refill_rx_ring(dev, 2, priv->ring_control->rx_mgmt,
-		ARRAY_SIZE(priv->ring_control->rx_mgmt), priv->rx_buf_mgmt);
+		ARRAY_SIZE(priv->ring_control->rx_mgmt), priv->rx_buf_mgmt, 0);
 
 	P54P_WRITE(ring_control_base, cpu_to_le32(priv->ring_control_dma));
 	P54P_READ(ring_control_base);

@@ -550,9 +586,26 @@ static int __devinit p54p_probe(struct p
 	}
 
 	err = p54p_open(dev);
-	if (err)
-		goto err_free_common;
+	if (err) {
+                
+		printk(KERN_DEBUG "%s: p54p_open failed - trying again\n",__func__);
+                msleep(10);
+		err = p54p_open(dev);
+		if (err)
+			goto err_free_common;
+        }
 	err = p54_read_eeprom(dev);
+	if (err)
+	{
+                printk(KERN_DEBUG "%s: p54_read_eeprom failed - trying again\n",__func__);
+		p54p_stop(dev);

Quoted 6 lines

In trying to get p54pci driver to be stable on my platform and hardware
- here is a generic patch that seems to accomplish that. Since the ViewSonic V210 uses the IT8152 pci bridge - some attention was needed to get dma related allocation in the first physical 64M. I have verified that the dma related allocation is in the first 64M and dmabounce is not being used - just for those wondering if that was part of the problems.

Quoted 6 lines · On Sunday 11 October 2009 17:31:42 Larry Finger wrote:

Quoted 6 lines

In trying to get p54pci driver to be stable on my platform and hardware
- here is a generic patch that seems to accomplish that. Since the ViewSonic V210 uses the IT8152 pci bridge - some attention was needed to get dma related allocation in the first physical 64M. I have verified that the dma related allocation is in the first 64M and dmabounce is not being used - just for those wondering if that was part of the problems.

it8152 was an important bit:

http://lkml.indiana.edu/hypermail/linux/kernel/0702.1/0645.html

the commit sparked a discussion about it8152 pci reliability. It doesn't look good:

(commit author):
"I have no idea if it's possible to get a reliable PCI bus or not with this chip. Right now, we only use it for it's built-in OHCI USB host controller and UART. You're making me hope I never have to use it for interfacing a PCI card!"

( http://lkml.indiana.edu/hypermail/linux/kernel/0702.1/1907.html )

[...]

"Well on the system on a board we were trying, using the development baseboard from the same supplier, by simply doing a ping flood through the onboard rtl8139 I managed to get corrupted ethernet packets fairly frequently. "
( http://lkml.indiana.edu/hypermail/linux/kernel/0702.1/1917.html )

the sad conclusion is that: no matter what fixes you throw at the driver (BTW: isl38xx isn't 100% pci v2.1 compliance either) your chances of getting the device (with the softmac fw) working properly with that board are next to... not, unless you can magically fix the pci-bridge issues.

Quoted 4 lines · Quintin Pitts

Hi,

Sorry for my lack of experience in all aspects - first time submitting!!!

Everyone that goes through this "right of passage" gets somewhat discouraged by the response. My advice is to hang in.

My first advice is for you to run every submitted patch through the check at scripts/checkpatch.pl. This one shows 95 errors and 7 warnings in 136 lines. Most of the errors are due to "DOS line endings". We really hate carriage returns - a really useless occupier of space unless it is _NOT_ followed by \n!

As I understand it, this patch is to fix the driver to work around firmware errors. If that is correct, please state that clearly. If only partially correct, then indicate which parts are to fix firmware errors, and which are to fix driver errors. Has your analysis included thinking about where the driver might delay to avoid firmware problems.

I will let Christian comment more on the technical merits of the patch as he understands the device much better than I do.

Larry

Quoted 6 lines · Larry Finger

On 10/11/2009 09:28 AM, Quintin Pitts wrote:
As I understand it, this patch is to fix the driver to work around firmware errors. If that is correct, please state that clearly. If only partially correct, then indicate which parts are to fix firmware errors, and which are to fix driver errors. Has your analysis included thinking about where the driver might delay to avoid firmware problems.

I think Christian has hit the nail on the head.
Mostly flaky hardware or implementation (it8152 pci bridge) when pushed.

Quoted 9 lines · On Monday 12 October 2009 02:09:22 Quintin Pitts wrote:

Quoted 6 lines · Larry Finger

On 10/11/2009 09:28 AM, Quintin Pitts wrote:
As I understand it, this patch is to fix the driver to work around firmware errors. If that is correct, please state that clearly. If only partially correct, then indicate which parts are to fix firmware errors, and which are to fix driver errors. Has your analysis included thinking about where the driver might delay to avoid firmware problems.

I think Christian has hit the nail on the head.
Mostly flaky hardware or implementation (it8152 pci bridge) when pushed.

hmm, flaky hardware or simply incomplete linux support. you've said that it works with win ce,
so I suppose the code is missing some important bits.

e.g.:

00:06.0 Network controller: Intersil Corporation ISL3886 [Prism Javelin/Prism Xbow] (rev 01)
Subsystem: Intersil Corporation Device 0000
Flags: bus master, medium devsel, latency 56, IRQ 217

by the looks of it, something could wrong with the latency timer value. (what's lspci -vvnnxxx output for the card?)

I've attached a minimal patch which c&p some latency-timer related logic from the original prism54 driver code to p54pci.
Can you please give this a try?

I don't have a p54pci available for testing right now.

Regards,
Chr