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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 15.02.2012 08:03, Wolfgang Grandegger wrote:

> Hi Oliver,
> 
> we start paving common code to partially cure pcmcia related problems,
> that's exactly what I do not like.


It's PCMCIA and PCIeC and Parallel port, etc. Any pluggable sja1000 hardware.

> 
> On 02/14/2012 05:41 PM, Oliver Hartkopp wrote:
>> On 14.02.2012 11:16, Stephane Grosjean wrote:
>>
>>>
>>> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
>>>
>>>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
> 
> In case of unplug, already the above read may return 0xff.


Right. But it does not get a bad effect as we quit from this bad condition
three lines later. No previously retrieved isrc variable is referenced then.
Please re-check the patched sja1000_interrupt() as a whole.

> 
>>>>           n++;
>>>>           status = priv->read_reg(priv, REG_SR);
>>>> +        /* check for absent controller due to hw unplug */
>>>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>> +            break;
> 
> Once...
> 
>>>>           if (isrc&  IRQ_WUI)
>>>>               netdev_warn(dev, "wakeup interrupt\n");
>>>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>               while (status&  SR_RBS) {
>>>>                   sja1000_rx(dev);
>>>>                   status = priv->read_reg(priv, REG_SR);
>>>> +                /* check for absent controller */
>>>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>> +                    break;
> 
> ... twice... What about the other accesses?


Which ones do you have in mind?

The really only thing that could happen, that exactly ONE CAN frame can be
damaged on read while accessing the SJA1000 registers. This can not be solved
unless you check for sja1000_probe_chip() at the end of reading the frame and
before pushing the skbuff to the rx queue.

But this would add indeed a new ioport access each time we read a CAN frame.

I don't think that we should do this additional effort - even if it's probably
the right(TM) thing to do 8-)

My patch doesn't do any additional ioport accesses in a 'good' condition.

(..)

>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...
>>

>> So if we add the 'return IRQ_NONE' to the above patch the private check for
>> present hardware becomes obsolete as the private ISRs terminate properly when
>> they get an IRQ_NONE.
>>
>> Right?
> 
> If it's clear that the interrupt came from that device, we should return
> IRQ_HANDLED otherwise it's treated as spurious.


Can this usually happen in the case of unplugging devices?

Or we need some kind of wrapper for the sja1000 isr that returns

IRQ_NONE
IRQ_HANDLED
IRQ_PLUGOUT

while HANDLED & PLUGOUT would lead to IRQ_HANDLED on the top level.

> I would accept *one* "if
> (status == 0xFF ..." check in a good place.


Sorry but you need an additional check within the while statement.
There are two potential occurrences where we read the status. Why checking
only one time? As i wrote above: Please re-check the entire patched function.

Regards,
Oliver