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

[Wolfgang Grandegger <wg@grandegger.com>]

Hi Stephane,

On 02/14/2012 10:14 AM, Stephane Grosjean wrote:
> 
> Le 13/02/2012 21:23, Wolfgang Grandegger a écrit :
>> On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
>>> 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.
> 
> Well if I may interfere: since the driver "remove()" entry point is
> called and the netdev interface is destroyed next, nobody can truly says
> if these frames have time to be processed until the client application.
> The problem is not here.
> 
>> 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.
> 
> cannot say that, this is arch dependent.
> 
>>>> 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).
> 
> cannot say that: all the tests I made @1xMbps bitrate show that the
> issue at least occurs 3/4 of time in the sja1000_interrupt, not in the
> custom isr. This also is obviously arch dependent.

What check did you use in the custom isr? A dummy register read != 0xff?
If that is really the case, I would prefer Oliver's simple fix in
sja1000_interrupt().

Wolfgang.