Dear All, I'm experiencing lost frames and id 0, 8 byte all zero packets now that we have equipment sending back to back packets on a 250Kb network. I have plenty of hardware attached to Gumstix Overos so I'll test your patch once the kernel is rebuilt.
Cheers Mike. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of christian pellegrin Sent: Thursday, 2 September 2010 02:36 To: Benny B. Simonsen Cc: [email protected] Subject: Re: [Socketcan-users] MCP 2515 RX problems Hi, first of all sorry for the late response. Just now I managed to put together the needed hardware (you know, in Italy it's near impossible to do anything not related to holidays in august ;-) ) I had a quick look at your patch and it's similar to the one I prepared and I'm attaching below. I think both of them solve the problem with out of order frame. What is worrying me is that packet with all zeros. I never saw something similar. I'll put my system on test during the next night to see if I can reproduce it. I guess you put the code to reset RX flags in case of error to try to solve this glitch, didn't you? Anyway I will try your patch tomorrow. diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c index 177f2e1..e8c5e37 100644 --- a/drivers/net/can/mcp251x.c +++ b/drivers/net/can/mcp251x.c @@ -773,8 +773,9 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) struct mcp251x_priv *priv = dev_id; struct spi_device *spi = priv->spi; struct net_device *net = priv->net; + u8 intf; #ifdef STATS - static int irqs, runs, ov0, ov1, rx0, rx1; + static int irqs, runs; static int ee[8], ii[8]; static unsigned long last; unsigned long now; @@ -788,7 +789,7 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) now = jiffies; if (now >= last + HZ) { last = now; - printk("%lu: i %d r %d r0 %d r1 %d o0 %d o1 %d err", now, irqs, runs, rx0, rx1, ov0, ov1); + printk("%lu: i %d r %d err", now, irqs, runs); for(i = 0; i < 8; i++) { printk(",%d", ee[i]); ee[i] = 0; @@ -799,24 +800,20 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) ii[i] = 0; } printk("\n"); - irqs = runs = rx0 = rx1 = ov0 = ov1 = 0; + irqs = runs = 0; } } irqs += 1; #endif mutex_lock(&priv->mcp_lock); - while (!priv->force_quit) { + intf = mcp251x_read_reg(spi, CANINTF); + do { enum can_state new_state; - u8 intf = mcp251x_read_reg(spi, CANINTF); - u8 eflag; + u8 eflag = 0; int can_id = 0, data1 = 0; #ifdef STATS runs += 1; - if (intf & CANINTF_RX0IF) - rx0 += 1; - if (intf & CANINTF_RX1IF) - rx1 += 1; { int i; for(i = 0; i < 8; i++) { @@ -828,25 +825,31 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) if (intf & CANINTF_RX0IF) { mcp251x_hw_rx(spi, 0); - /* Free one buffer ASAP */ - mcp251x_write_bits(spi, CANINTF, intf & CANINTF_RX0IF, - 0x00); + + /* check that a packet didn't come while emptying RX0 */ + if ((intf & CANINTF_RX1IF) == 0) + intf |= mcp251x_read_reg(spi, CANINTF); } if (intf & CANINTF_RX1IF) mcp251x_hw_rx(spi, 1); + /* This is needed to close a race condition that causes + * packet reordening: we must assure that we never empty + * RX0 but not RX1. If this happens the next packet will + * land in RX0. As a conseuqence on the next do { } while + * spin we will have packets out of order. + */ + intf |= CANINTF_RX1IF; + mcp251x_write_bits(spi, CANINTF, intf, 0x00); - eflag = mcp251x_read_reg(spi, EFLG); - mcp251x_write_reg(spi, EFLG, 0x00); + if (intf && CANINTF_ERRIF) { + eflag = mcp251x_read_reg(spi, EFLG); + mcp251x_write_reg(spi, EFLG, 0x00); + } #ifdef STATS - if (eflag & EFLG_RX0OVR) - ov0 += 1; - - if (eflag & EFLG_RX1OVR) - ov1 += 1; { int i; for(i = 0; i < 8; i++) { @@ -924,9 +927,6 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) } } - if (intf == 0) - break; - if (intf & (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)) { net->stats.tx_packets++; net->stats.tx_bytes += priv->tx_len - 1; @@ -936,7 +936,8 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id) } netif_wake_queue(net); } - } + intf = mcp251x_read_reg(spi, CANINTF); + } while (!priv->force_quit && intf); mutex_unlock(&priv->mcp_lock); return IRQ_HANDLED; } On Sat, Aug 28, 2010 at 2:20 PM, Benny B. Simonsen <[email protected]> wrote: > > > 2010/8/6 christian pellegrin <[email protected]> >> >> On Fri, Aug 6, 2010 at 3:56 PM, Wolfgang Grandegger >> <[email protected]> >> wrote: >> > To be clear, out-of-order messages are also not OK. >> >> Yes I was looking at this with some interleave analysis. I think the >> problem lies in the following race: >> >> // suppose we come here with RXB0 filled with packet X >> u8 intf = mcp251x_read_reg(spi, CANINTF); >> u8 eflag; >> int can_id = 0, data1 = 0; >> >> // packet X+1 arrives and lands in RXB1 >> >> if (intf & CANINTF_RX0IF) { >> mcp251x_hw_rx(spi, 0); >> /* Free one buffer ASAP */ >> mcp251x_write_bits(spi, CANINTF, intf & >> CANINTF_RX0IF, >> 0x00); >> } >> >> // packet X+2 arrives and lands in RXB0 // unfortunately variable >> intf is old and doesn't show that we have something in RXB1 // (*1) >> if (intf & CANINTF_RX1IF) >> mcp251x_hw_rx(spi, 1); >> >> now on the next turn we will have packet X+2 in RXB0 and X+1 in RXB1 >> so we read them out-of-order. The easiest way to solve the problem is >> to take out the line: >> >> /* Free one buffer ASAP */ >> mcp251x_write_bits(spi, CANINTF, intf & >> CANINTF_RX0IF, >> 0x00); >> >> but we risk to lose more packets. Perhaps also rereading intf in (*1) >> could work but we must be careful when we reset CANINTF register to >> not lose an interrupt for RXB0. I will do some tests but after next >> week because I don't have the access to the hardware until then. >> >> -- >> Christian Pellegrin, see http://www.evolware.org/chri/ "Real >> Programmers don't play tennis, or any other sport which requires you >> to change clothes. Mountain climbing is OK, and Real Programmers wear >> their climbing boots to work in case a mountain should suddenly >> spring up in the middle of the computer room." > > > Hi Christian, > Did you have a chance to look on the problem and / or the patch I send > to the list? > BR > Benny -- Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room." _______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users _______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users
