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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 13.02.2012 12:08, Wolfgang Grandegger wrote:

> On 02/13/2012 12:06 PM, Marc Kleine-Budde wrote:
>> On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote:
>>> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
>>>> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
>>>>
>>>>
>>>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>>>> index ebbcfca..f7526a7 100644
>>>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>                  n++;
>>>>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>>>>
>>>>>>> +               /* check for absent controller due to hw unplug */
>>>>>>> +               if (status == 0xFF)
>>>>>>> +                       break;
>>>>>>> +
>>>>>>>                  if (isrc&  IRQ_WUI)
>>>>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>>>>
>>>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>                          netif_wake_queue(dev);
>>>>>>>                  }
>>>>>>>                  if (isrc&  IRQ_RI) {
>>>>>>> -                       /* receive interrupt */
>>>>>>> -                       while (status&  SR_RBS) {
>>>>>>> +                       /* receive interrupt / check for absent controller */
>>>>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>>>>                                  sja1000_rx(dev);
>>>>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>>>>                          }
>>>>>>>
>>>>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>>>>
>>>>>> I confirm that this patch works too...
>>>>>> So I think I should be able to post a new version of the peak_pcmcia
>>>>>> during that day (the previous should work but some calls to
>>>>>> pcmcia_dev_present() are no more useful...)
>>>>>
>>>>> But that fix should not go to the common interrupt handler, if possible.
>>>>
>>>>
>>>> Do you have an idea how to handle the while() statement without copying the
>>>> entire interrupt handler code for the devices that might be unplugged?
>>>>
>>>> IMHO this patch is pretty cheap.
>>>
>>> I think it's enough to add a read in the custom pcmcia isr before
>>> calling sja1000_interrupt(). Maybe the race window is a bit lower but we
>>> are not able to react when the unplug happens anyway.


Does this mean, it is 'less bad'?

>> It's not about reacting when an unlug happens, it's about not having the
>> interrupt handler to loop forever. At least we must limit the inner
>> while loop.


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

???

Sending nothing is preferred to sending wrong data :-)

> 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).

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.

Regards,
Oliver