Re: [PATCH RFC] e1000: clear ICR before requesting an IRQ line
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(>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.c2007-07-19
> > 18:18:53.0 +0900
> > +++ linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c 2007-07-25
> > 17:22:54.0 +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(>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
Re: [PATCH RFC] e1000: clear ICR before requesting an IRQ line
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?
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.
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...
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.c2007-07-19
18:18:53.0 +0900
+++ linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c 2007-07-25
17:22:54.0 +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(>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/
Re: [PATCH RFC] e1000: clear ICR before requesting an IRQ line
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?
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.
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...
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.c2007-07-19
18:18:53.0 +0900
+++ linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c 2007-07-25
17:22:54.0 +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/
Re: [PATCH RFC] e1000: clear ICR before requesting an IRQ line
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.c2007-07-19
18:18:53.0 +0900
+++ linux-2.6.22-pendirq/drivers/net/e1000/e1000_main.c 2007-07-25
17:22:54.0 +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/
