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

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

Le 15/02/2012 08:03, Wolfgang Grandegger a écrit :
> Hi Oliver,
>
> we start paving common code to partially cure pcmcia related problems,
> that's exactly what I do not like.


I think it's much more than a pcmcia related problem: IMHO, I think it's 
right in some common code to do some 'basic' checks on returned values, 
especially when these are returned from custom callbacks and these 
values are so critical. For me, it's like one has to check the kmalloc() 
return value, for example. But I admit this could not be the right(TM) 
way of doing things in the Kernel.


>> An alternative would be to replace the 'break' statements above with
>>
>> 	return IRQ_NONE;
>>
>> like
>>
>> 		/* check for absent controller due to hw unplug */
>> 		if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> 			return IRQ_NONE;
>>
>> Don't know if this is 'good style' - but the while statement allows it ;-)
>>
>>> Moreover, regarding to this patch, should the peak_pcmcia custom isr always
>>> test for the card presence? I mean, this costs at least 2xtwo ioread8 for each
>>> hw irq!
>>
>> I think the earlier we know the better.


Ok, so I keep testing the presence of the card in the peak_pcmcia driver.

If I may ask something else: didn't find any "linux-pcmcia" mailing list 
(or something like that) in vger majordomo lists. Does someone know if 
there is such a list? By default, which ml should be posted the 
peak_pcmcia.c too?


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


Hmmm... I don't know what is the real goal of these pre_irq() and 
post_iqr() callbacks but I imagine that, for example, the "pre_irq()" 
could acquire a lock while the post_irq() would release it... So, I 
would prefer to call the post_irq() anytime (that is, no direct "return 
IRQ_xxx" but a 'break'  or a 'goto'). Does it make sense?


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


I would also have say that before, but I checked that IRQ_NONE may be 
processed as spurious, so I don't even know, for the moment. LDD3 
chapter 10 also says:

> Interrupt handlers should return a value indicating whether there was 
> actually an
> interrupt to handle. If the handler found that its device did, indeed, 
> need attention, it
> should return IRQ_HANDLED; otherwise the return value should be IRQ_NONE.

So, what does this "need attention" stands for? Since the device is 
unplugged, we could say that it didn't need any attention anymore... 
But, on the other hand, the ems_pcmcia driver does return IRQ_HANDLED 
when its private check fails to detect the card and it has been 
approved, so, where is the truth? Near the perfection? ;-)


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