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

[Wolfgang Grandegger <wg@grandegger.com>]

On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
> 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'?

No.

>>> 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
> 
> ???

Do you have seen such traces with the "device present" check in the
custom handler? I doubt.

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.

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

I disagree. It will not change a lot. Measurements may proof that I'm
wrong, though.

Wolfgang.