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

[Wolfgang Grandegger <wg@grandegger.com>]

On 02/15/2012 12:52 PM, Stephane Grosjean wrote:
> 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.

Well, but it's unusual (and slow) to check any hardware access.

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

Here is the entry from the kernel's MAINTAINER file with the relevant
mailing list address:

PCMCIA SUBSYSTEM
P:      Linux PCMCIA Team
L:      linux-pcmcia@lists.infradead.org
W:      http://lists.infradead.org/mailman/listinfo/linux-pcmcia
T:      git
git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git
S:      Maintained
F:      Documentation/pcmcia/
F:      drivers/pcmcia/
F:      include/pcmcia/


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

The pre_irq and post_irq callbacks have been introduced to allow device
specific interrupt handling/acknowledgement before and after the common
handler. Have a look to the peak_pci driver to understand what I mean.

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

From http://lxr.linux.no/#linux+v3.2.6/include/linux/irqreturn.h:

  IRQ_NONE            interrupt was not from this device

Wolfgang.