On Wed, 2007-07-25 at 08:27 -0700, Kok, Auke wrote:
> Fernando Luis Vázquez Cao wrote:
> > I made an interesting finding while testing the two patches below.
> >
> > http://lkml.org/lkml/2007/7/19/685
> > http://lkml.org/lkml/2007/7/19/687
> >
> > These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
> > that the request_irq prints a warning if after calling the handler it
> > returned IRQ_HANDLED .
> >
> > The code looks like this:
> >
> > int request_irq(unsigned int irq, irq_handler_t handler,
> > unsigned long irqflags, const char *devname, void *dev_id)
> > .....
> > if (irqflags & IRQF_DISABLED) {
> > unsigned long flags;
> >
> > local_irq_save(flags);
> > retval = handler(irq, dev_id);
> > local_irq_restore(flags);
> > } else
> > retval = handler(irq, dev_id);
> > if (retval == IRQ_HANDLED) {
> > printk(KERN_WARNING
> > "%s (IRQ %d) handled a spurious interrupt\n",
> > devname, irq);
> > }
> > .....
> >
> > I discovered that the e1000 driver handles the "fake" interrupt, which,
> > in principle, is not correct because it obviously isn't a real interrupt
> > and it could have been an interrupt coming from another device that is
> > sharing the IRQ line.
> >
> > The problem is that the interrupt handler assumes that if ICR!=0 it was
> > its device who generated the interrupt and, consequently, it should be
> > handled. But, unfortunately, that is not always the case. If the network
> > link is active when we open the device (e1000_open) the ICR will have
> > the E1000_ICR_LSC bit set (by the way, is this the expected behavior?).
>
> yes. is it really a problem though?
It seems we may end up handling spurious interrupts or interrupts coming
from another devices.
> > This means that _any_ interrupt coming in after allocating our interrupt
> > (e1000_request_irq) will be handled, no matter where it came from.
>
> we actually generate this LSC interrupt ourselves in the driver, to make sure
> that we cascade into the watchdog which then enables or disables the link
> code
> based on the link status change. This allows us to _not_ do any link checking
> in
> _open and makes things a bit more simple.
I am not referring to the LSC interrupt the driver itself generates in
e1000_open just before returning. The ICR is masked (ICR==0) after
executing the driver probe (e1000_probe), but when we enter e1000_open
the E1000_ICR_LSC bit will already be set, before the function even
starts executing. I also observed that when the link is active the line
/* fire a link status change interrupt to start the watchdog */
E1000_WRITE_REG(&adapter->hw, ICS, E1000_ICS_LSC);
is redundant because the E1000_ICS_LSC bit is already set. In fact, the
irq handler gets invoked twice in a row with the interrupt cause being a
link status change.
> > The solution I came up with is clearing the ICR before calling
> > request_irq. I have to admit that I am not familiar enough with this
> > driver, so it is quite likely that this is not the right fix. I would
> > appreciate your comments on this.
>
> Clearing the ICR before requesting an irq might not work - at the same time
> the
> device could generate another LSC irq...
Is it not possible to prevent the device from generating interrupts until we
call request_irq?
Thank you for your feedback!
- Fernando
> Of course, we probably should just schedule some delayed work to run our
> watchdog in e1000_open, but I haven't checked if that actually works.
>
>
> Auke
>
> > Signed-off-by: Fernando Luis Vazquez Cao <[EMAIL PROTECTED]>
> > ---
> >
> > diff -urNp linux-2.6.22-orig/drivers/net/e1000/e1000_main.c
> > linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c
> > --- linux-2.6.22-orig/drivers/net/e1000/e1000_main.c 2007-07-19
> > 18:18:53.000000000 +0900
> > +++ linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c 2007-07-25
> > 17:22:54.000000000 +0900
> > @@ -1378,6 +1378,17 @@ e1000_alloc_queues(struct e1000_adapter
> > }
> >
> > /**
> > + * e1000_clear_interrupts
> > + * @adapter: address of board private structure
> > + *
> > + * Mask interrupts
> > + **/
> > +static void
> > +e1000_clear_interrupts(struct e1000_adapter *adapter) {
> > + E1000_READ_REG(&adapter->hw, ICR);
> > +}
> > +
> > +/**
> > * e1000_open - Called when a network interface is made active
> > * @netdev: network interface device structure
> > *
> > @@ -1431,6 +1442,9 @@ e1000_open(struct net_device *netdev)
> > * so we have to setup our clean_rx handler before we do so. */
> > e1000_configure(adapter);
> >
> > + /* Discard any possible pending interrupts. */
> > + e1000_clear_interrupts(adapter);
> > +
> > err = e1000_request_irq(adapter);
> > if (err)
> > goto err_req_irq;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
