On Fri, Sep 23, 2011 at 11:10:17AM +0200, Markus Plessing wrote:
> Hi Oliver,
>
> looks quite good for me - more comments inline.
>
> Am 14.09.2011 15:01, schrieb Oliver Hartkopp:
> >On 09/13/11 09:56, Marc Kleine-Budde wrote:
> >
> >>On 09/12/2011 08:44 PM, Oliver Hartkopp wrote:
> >
> >>>+static irqreturn_t ems_pcmcia_interrupt(int irq, void *dev_id)
> >>>+{
> >>>+ struct ems_pcmcia_card *card = dev_id;
> >>>+ struct net_device *dev;
> >>>+ irqreturn_t retval = IRQ_NONE;
> >>>+ int i, again;
> >>>+
> >>>+ /* Card not present */
> >>>+ if (readw(card->base_addr) != 0xAA55)
> >>>+ return IRQ_HANDLED;
> >>
> >>this looks fishy. Is it possible, that there is no card here? Why return
> >>IRQ_HANDLED in this case?
> >>
> >This has been introduced to handle PCMCIA card ejects under heavy load.
> >Probably IRQ_NONE could be returned here also - but if we come to this point
> >everything is removed anyway.
> >
>
> This is right, the CPC-Card may be removed unsave and on heavy load
> there may be unhandled interrupts. The IRQ_HANDLED is not mandatory
> here, I think this refers to our experience in embedded development
> where the interrupt flag has to be cleared (handled) even on error.
>
> It's up to you what you think what's the best return value here.I still say IRQ_HANDLED is plain wrong. You can't ack an IRQ if you _know_ the card isn't there. I wonder if that isn't obvious? Return IRQ_NONE here and you will at least be notified that there was this unwanted interrupt. At least, pcmcia network drivers do this, too. Have you maybe checked other pcmcia drivers? And you should really start sending this driver to the pcmcia- and netdev-lists so they can suggest handling the error-printouts there... Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Socketcan-core mailing list Socketcan-core@lists.berlios.de https://lists.berlios.de/mailman/listinfo/socketcan-core
