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
