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

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

Le 13/02/2012 11:14, Wolfgang Grandegger a écrit :
>>> 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.

I suppose that the reason why you say that is because the 0xff value 
doesn't have got the special meaning of "potential unplug" (is it?), so 
I could understand why it's not appropriate to check it, in that common 
handler (perhaps the returned value is different in some other archs too).

But I don't agree with that potential infinite loop, testing the SR_RBS 
bit.

So, since we already are looping until SJA1000_MAX_IRQ, why not changing 
this:

                 if (isrc & IRQ_RI) {
                        /* receive interrupt */
                        while (status & SR_RBS) {
                                 sja1000_rx(dev);
                                 status = priv->read_reg(priv, REG_SR);
                         }

into something like that:

                 if (isrc & IRQ_RI) {
                        /* receive interrupt */
                        if (status & SR_RBS)
                                 sja1000_rx(dev);


In that case, even the 0xff value would not loop infinitely... Testing 
the device presence will be of the driver responsibility and could speed 
up unplug detection.

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