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

[Wolfgang Grandegger <wg@grandegger.com>]

On 02/15/2012 09:05 AM, Oliver Hartkopp wrote:
> 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-)

Well, see my comment below.

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

Why do we care so much about improper handling of the hardware. It is
*illegal* to unplug the device while it's under operation and if it
happens we cannot guarantee valid behavior and data anyway. The system
should not hang, of course, everything else is not that dramatic, I
think. Mark, what is your opinion?

Wolfgang.