Re: [patch] genirq: fix simple and fasteoi irq handlers
2007/8/3, Jarek Poplawski <[EMAIL PROTECTED]>:
> Hi,
>
> I can't guarantee this is all needed to fix this bug, but I think
> this patch is necessary here.
>
> Regards,
> Jarek P.
> >
>
> Subject: genirq: fix simple and fasteoi irq handlers
>
> After the "genirq: do not mask interrupts by default" patch interrupts
> should be disabled not immediately upon request, but after they happen.
> But, handle_simple_irq() and handle_fasteoi_irq() can skip this once or
> more if an irq is just serviced (IRQ_INPROGRESS), possibly disrupting a
> driver's work.
>
> The main reason of problems here, pointing the broken patch and making
> the first patch which can fix this was done by Marcin Slusarz.
> Additional test patches of Thomas Gleixner and Ingo Molnar tested by
> Marcin Slusarz helped to narrow possible reasons even more. Thanks.
>
> PS: this patch fixes only one evident error here, but there could be
> more places affected by above-mentioned change in irq handling.
>
>
> Signed-off-by: Jarek Poplawski <[EMAIL PROTECTED]>
>
> ---
>
> diff -Nurp 2.6.23-rc1-/kernel/irq/chip.c 2.6.23-rc1/kernel/irq/chip.c
> --- 2.6.23-rc1-/kernel/irq/chip.c 2007-07-09 01:32:17.0 +0200
> +++ 2.6.23-rc1/kernel/irq/chip.c2007-08-02 20:42:38.0 +0200
> @@ -295,12 +295,11 @@ handle_simple_irq(unsigned int irq, stru
>
> spin_lock(&desc->lock);
>
> - if (unlikely(desc->status & IRQ_INPROGRESS))
> - goto out_unlock;
> kstat_cpu(cpu).irqs[irq]++;
>
> action = desc->action;
> - if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
> + if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
> +IRQ_DISABLED {
> if (desc->chip->mask)
> desc->chip->mask(irq);
> desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> @@ -392,18 +391,16 @@ handle_fasteoi_irq(unsigned int irq, str
>
> spin_lock(&desc->lock);
>
> - if (unlikely(desc->status & IRQ_INPROGRESS))
> - goto out;
> -
> desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> kstat_cpu(cpu).irqs[irq]++;
>
> /*
> -* If its disabled or no action available
> +* If it's running, disabled or no action available
> * then mask it and get out of here:
> */
> action = desc->action;
> - if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
> + if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
> +IRQ_DISABLED {
> desc->status |= IRQ_PENDING;
> if (desc->chip->mask)
> desc->chip->mask(irq);
>
This patch didn't fix my NIC (tried on 2.6.22.1)
Marcin
-
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] genirq: fix simple and fasteoi irq handlers
On Fri, Aug 03, 2007 at 01:57:00PM +0200, Marcin Ślusarz wrote: ... > I'll test this patch tomorrow (and confirm that the last one from Ingo > works fine) and report results on monday (sorry, no internet at home > since I moved out of city :|). So, you are a lucky guy! I have only no internet at home. ...and time for dreaming about moving out of a city... Cheers, Jarek P. - 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] genirq: fix simple and fasteoi irq handlers
2007/8/3, Jarek Poplawski <[EMAIL PROTECTED]>: > On Fri, Aug 03, 2007 at 10:04:08AM +0200, Ingo Molnar wrote: > > > > * Jarek Poplawski <[EMAIL PROTECTED]> wrote: > > > > > I can't guarantee this is all needed to fix this bug, but I think this > > > patch is necessary here. > > > > hmmm ... very interesting! Now _this_ is something we'd like to see > > tested. Could you send a patch to Marcin that also undoes the workaround > > we have in place now, so that he could check whether ne2k-pci works fine > > with your fix alone? > > I'm not sure this is needed... Marcin got this patch, I hope, and I > don't have another possibility to contact with him. Since he managed > with this bisection and all the previous patches I don't think there > could be any problems, so: > > Marcin! I'd be very glad if you could test this patch alone; this > should apply without any problems to 2.6.21 (with some offset) and > later "vanilla" versions (or try to revert Ingo's last patch with > patch -p1 -R). Please, contact me on any problems (alas not during > the weekend...). I'll test this patch tomorrow (and confirm that the last one from Ingo works fine) and report results on monday (sorry, no internet at home since I moved out of city :|). Marcin - 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] genirq: fix simple and fasteoi irq handlers
On Fri, Aug 03, 2007 at 10:04:08AM +0200, Ingo Molnar wrote: > > * Jarek Poplawski <[EMAIL PROTECTED]> wrote: > > > I can't guarantee this is all needed to fix this bug, but I think this > > patch is necessary here. > > hmmm ... very interesting! Now _this_ is something we'd like to see > tested. Could you send a patch to Marcin that also undoes the workaround > we have in place now, so that he could check whether ne2k-pci works fine > with your fix alone? I'm not sure this is needed... Marcin got this patch, I hope, and I don't have another possibility to contact with him. Since he managed with this bisection and all the previous patches I don't think there could be any problems, so: Marcin! I'd be very glad if you could test this patch alone; this should apply without any problems to 2.6.21 (with some offset) and later "vanilla" versions (or try to revert Ingo's last patch with patch -p1 -R). Please, contact me on any problems (alas not during the weekend...). Thanks, Jarek P. PS: of course, I'm very curious of this testing too, but, on the other hand, as I've written earlier, I think this patch is needed for logical reasons only, and it really doesn't look like it could make any damage here. - 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] genirq: fix simple and fasteoi irq handlers
* Ingo Molnar <[EMAIL PROTECTED]> wrote: > * Jarek Poplawski <[EMAIL PROTECTED]> wrote: > > > I can't guarantee this is all needed to fix this bug, but I think > > this patch is necessary here. > > hmmm ... very interesting! Now _this_ is something we'd like to see > tested. Could you send a patch to Marcin that also undoes the > workaround we have in place now, so that he could check whether > ne2k-pci works fine with your fix alone? or it would be nice if Marcin could test pure 2.6.22 plus your fix (without any other patches applied). Ingo - 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] genirq: fix simple and fasteoi irq handlers
* Jarek Poplawski <[EMAIL PROTECTED]> wrote: > I can't guarantee this is all needed to fix this bug, but I think this > patch is necessary here. hmmm ... very interesting! Now _this_ is something we'd like to see tested. Could you send a patch to Marcin that also undoes the workaround we have in place now, so that he could check whether ne2k-pci works fine with your fix alone? Ingo - 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/
[patch] genirq: fix simple and fasteoi irq handlers
On Thu, Aug 02, 2007 at 10:11:26PM +0200, Ingo Molnar wrote:
>
> * Gabriel C <[EMAIL PROTECTED]> wrote:
>
> > I get a warning on each boot now with this patch ..
> >
> > [ 63.686613] WARNING: at kernel/irq/resend.c:70 check_irq_resend()
...
> we are still trying to figure out what happens with ne2k-pci. The
> message will vanish soon.
Hi,
I can't guarantee this is all needed to fix this bug, but I think
this patch is necessary here.
Regards,
Jarek P.
>
Subject: genirq: fix simple and fasteoi irq handlers
After the "genirq: do not mask interrupts by default" patch interrupts
should be disabled not immediately upon request, but after they happen.
But, handle_simple_irq() and handle_fasteoi_irq() can skip this once or
more if an irq is just serviced (IRQ_INPROGRESS), possibly disrupting a
driver's work.
The main reason of problems here, pointing the broken patch and making
the first patch which can fix this was done by Marcin Slusarz.
Additional test patches of Thomas Gleixner and Ingo Molnar tested by
Marcin Slusarz helped to narrow possible reasons even more. Thanks.
PS: this patch fixes only one evident error here, but there could be
more places affected by above-mentioned change in irq handling.
Signed-off-by: Jarek Poplawski <[EMAIL PROTECTED]>
---
diff -Nurp 2.6.23-rc1-/kernel/irq/chip.c 2.6.23-rc1/kernel/irq/chip.c
--- 2.6.23-rc1-/kernel/irq/chip.c 2007-07-09 01:32:17.0 +0200
+++ 2.6.23-rc1/kernel/irq/chip.c2007-08-02 20:42:38.0 +0200
@@ -295,12 +295,11 @@ handle_simple_irq(unsigned int irq, stru
spin_lock(&desc->lock);
- if (unlikely(desc->status & IRQ_INPROGRESS))
- goto out_unlock;
kstat_cpu(cpu).irqs[irq]++;
action = desc->action;
- if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+ if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
+IRQ_DISABLED {
if (desc->chip->mask)
desc->chip->mask(irq);
desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
@@ -392,18 +391,16 @@ handle_fasteoi_irq(unsigned int irq, str
spin_lock(&desc->lock);
- if (unlikely(desc->status & IRQ_INPROGRESS))
- goto out;
-
desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
kstat_cpu(cpu).irqs[irq]++;
/*
-* If its disabled or no action available
+* If it's running, disabled or no action available
* then mask it and get out of here:
*/
action = desc->action;
- if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+ if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
+IRQ_DISABLED {
desc->status |= IRQ_PENDING;
if (desc->chip->mask)
desc->chip->mask(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/
