Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean <s.grosjean@peak-system.com>]

Le 13/02/2012 21:23, Wolfgang Grandegger a écrit :
> On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
>> What happens, if inside this inner while() statement the unplug happens?
>>
>> Do we get two correct CAN frames and then we get eight CAN frames like this:
>>
>> 12345677 4 AA BB CC DD
>> 12345678 6 AA BB CC DD EE FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>
>> ???
> Do you have seen such traces with the "device present" check in the
> custom handler? I doubt.

Well if I may interfere: since the driver "remove()" entry point is 
called and the netdev interface is destroyed next, nobody can truly says 
if these frames have time to be processed until the client application. 
The problem is not here.

> First it is unlikely, that the unplug will happen when data from the
> device gets processed. And if, the check in the while loop will not
> change a lot, in contrast to the check in the custom handler. If it
> happens while processing the data, we will get grap... but well...
>
>> Sending nothing is preferred to sending wrong data :-)
> You cannot avoid sending crap under any circumstances. Also be aware
> that the while loop will normally process just *one* message, and rarely
> more.

cannot say that, this is arch dependent.

>>> Of course, the loop should be limited in sja1000_interrupt anyway.
>>
>> IMO checking for absent hardware in the custom isr is fine and should fix most
>> of the problems (as we can already see in the ems_pcmcia driver).

cannot say that: all the tests I made @1xMbps bitrate show that the 
issue at least occurs 3/4 of time in the sja1000_interrupt, not in the 
custom isr. This also is obviously arch dependent.

>> But adding the patch which is checking two times for status != 0xFF reduces
>> the race conditions to almost zero. Having a while() statement that
>> potentially produces wrong data doesn't heal the problem IMO.
> I disagree. It will not change a lot. Measurements may proof that I'm
> wrong, though.

What about the fix Oliver talked about in one his first e-mail:

> The used status register has a bit for bus-off which is "1" in the bus-off
> case. Assuming RX interrupts can only occur in a 'bus-on' state, this bit can
> be assumed to be '0'.

IMHO, this is the best proposal: while the sja1000_interrupt() requests 
for the SR, it seems normal that it does some semantic checks about what 
it got.

So the fix could be something like this instead:

         while ((isrc = priv->read_reg(priv, REG_IR)) && (n < 
SJA1000_MAX_IRQ)) {
                 n++;
                 status = priv->read_reg(priv, REG_SR);

+               if (status & (SR_RBS|SR_BS) == (SR_RBS|SR_BS)) {
+                       /* what? something is rotten in my kingdom... */
+                       n = 0;
+                       break; /* or goto post_irq; as you want... */
+               }

                 if (isrc & IRQ_RI) {
                         /* receive interrupt */
                         while (status & SR_RBS) {
                                 sja1000_rx(dev);
                                 status = priv->read_reg(priv, REG_SR);
+                               if (status & (SR_RBS|SR_BS) == 
(SR_RBS|SR_BS)) {
+                                       /* what? something is rotten in 
my kingdom, here also... */
+                                       n = 0;
+                                       goto post_irq;
                                 }
                         }
                 }
         }

+post_irq:
         if (priv->post_irq)
                 priv->post_irq(priv);


Of course, in order to definitely say asta lavista to the potential 
infinite loop, we can use a for(;;) loop instead of the while()...

What are your opinion?

Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com