| Message ID | 1243404615-25879-2-git-send-email-vapier@gentoo.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Mike Frysinger <vapier@gentoo.org> Date: Wed, 27 May 2009 02:10:11 -0400 > From: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > Signed-off-by: Bryan Wu <cooloney@kernel.org> I'm not applying this. Either take the time to make this interrupt handler cope with shared interrupts (the most preferred) or take the time to write an excruciatingly long winded and explicit commit log message explaining why that's not possible. And I want the commit log message path to be as painful as possible so that you choose to fix this driver's IRQ handler to behave sanely instead. :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 05:01, David Miller wrote: > From: Mike Frysinger <vapier@gentoo.org> >> From: Michael Hennerich <michael.hennerich@analog.com> >> >> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> >> Signed-off-by: Mike Frysinger <vapier@gentoo.org> >> Signed-off-by: Bryan Wu <cooloney@kernel.org> > > I'm not applying this. > > Either take the time to make this interrupt handler cope with shared > interrupts (the most preferred) or take the time to write an > excruciatingly long winded and explicit commit log message explaining > why that's not possible. > > And I want the commit log message path to be as painful as possible so > that you choose to fix this driver's IRQ handler to behave sanely > instead. :-) the hardware has a dedicated internal IRQ line for the Blackfin MAC. it makes no sense to mark it as shared. there is nothing it could possibly be shared with. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Fri, 29 May 2009 06:49:42 -0400 > the hardware has a dedicated internal IRQ line for the Blackfin MAC. > it makes no sense to mark it as shared. there is nothing it could > possibly be shared with. Then why bother messing with the flag away at all? It doesn't fix anything. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>From: David Miller [mailto:davem@davemloft.net] >From: Mike Frysinger <vapier.adi@gmail.com> >Date: Fri, 29 May 2009 06:49:42 -0400 > >> the hardware has a dedicated internal IRQ line for the Blackfin MAC. >> it makes no sense to mark it as shared. there is nothing it could >> possibly be shared with. > >Then why bother messing with the flag away at all? It doesn't fix >anything. Well - it fixes a bug seen with CONFIG_DEBUG_SHIRQ. CONFIG_DEBUG_SHIRQ generates spurious fake interrupts and checks if the handler can cope with them. This is basically a good thing - however only for real shared interrupts. Our handler isn't prepared to handle such fake interrupts. So two options: 1) Set the proper IRQF flags. 2) Fix the ISR to handle spurious interrupts. This patch chose option 1) -Michael -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Hennerich, Michael" <Michael.Hennerich@analog.com> Date: Fri, 29 May 2009 12:12:49 +0100 > Our handler isn't prepared to handle such fake interrupts. > So two options: > 1) Set the proper IRQF flags. > 2) Fix the ISR to handle spurious interrupts. > > This patch chose option 1) And to go back to my initial reply, I'm saying to do the "right thing" and go with #2 if you can :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 07:14, David Miller wrote: > From: "Hennerich, Michael" <Michael.Hennerich@analog.com> >> Our handler isn't prepared to handle such fake interrupts. >> So two options: >> 1) Set the proper IRQF flags. >> 2) Fix the ISR to handle spurious interrupts. >> >> This patch chose option 1) > > And to go back to my initial reply, I'm saying to do the > "right thing" and go with #2 if you can :-) and the answer is that doesnt make sense. why implement extra over head in an interrupt handler to support an operating mode the hardware can never support. if the IRQ could actually be shared, then sure, we should check things. but it cant, so you're suggesting we waste time in an IRQ handler for absolutely no gain. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Fri, 29 May 2009 07:22:05 -0400 > why implement extra over head in an interrupt handler to support an > operating mode the hardware can never support. What overhead? It should be pretty easy to see if the device is really indicating an interrupt or not :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 07:28, David Miller wrote: > From: Mike Frysinger <vapier.adi@gmail.com> >> why implement extra over head in an interrupt handler to support an >> operating mode the hardware can never support. > > What overhead? It should be pretty easy to see if the device > is really indicating an interrupt or not :-) which would involve reading system mmrs which are clocked at the system frequency and thus make the core stall -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Fri, 29 May 2009 07:45:05 -0400 > On Fri, May 29, 2009 at 07:28, David Miller wrote: >> From: Mike Frysinger <vapier.adi@gmail.com> >>> why implement extra over head in an interrupt handler to support an >>> operating mode the hardware can never support. >> >> What overhead? It should be pretty easy to see if the device >> is really indicating an interrupt or not :-) > > which would involve reading system mmrs which are clocked at the > system frequency and thus make the core stall And the core doesn't stall reading in these cache lines that the chip has just DMA'd to? I think you're throwing the baby out with the bath water :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 18:09, David Miller wrote: > From: Mike Frysinger <vapier.adi@gmail.com> >> On Fri, May 29, 2009 at 07:28, David Miller wrote: >>> From: Mike Frysinger <vapier.adi@gmail.com> >>>> why implement extra over head in an interrupt handler to support an >>>> operating mode the hardware can never support. >>> >>> What overhead? It should be pretty easy to see if the device >>> is really indicating an interrupt or not :-) >> >> which would involve reading system mmrs which are clocked at the >> system frequency and thus make the core stall > > And the core doesn't stall reading in these cache lines that the chip > has just DMA'd to? the difference is that one of these is required in order for anything to get done and the other is always useless noise. also, the Blackfin core does do speculative data fetching on external memory, but not MMRs, so the stalling due to the data cache line fills will be mitigated unlike the useless MMR reads. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Fri, 29 May 2009 18:13:02 -0400 > the difference is that one of these is required in order for anything > to get done and the other is always useless noise. also, the Blackfin > core does do speculative data fetching on external memory, but not > MMRs, so the stalling due to the data cache line fills will be > mitigated unlike the useless MMR reads. You only need to read the register if the descriptor status is not ready yet, and you're just spinning in that case anyways. I doubt it makes any real difference if implemented properly. But I guess it's more fun to speculate than to actually try it out. :-/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 18:18, David Miller wrote: > From: Mike Frysinger <vapier.adi@gmail.com> >> the difference is that one of these is required in order for anything >> to get done and the other is always useless noise. also, the Blackfin >> core does do speculative data fetching on external memory, but not >> MMRs, so the stalling due to the data cache line fills will be >> mitigated unlike the useless MMR reads. > > You only need to read the register if the descriptor status is not > ready yet, and you're just spinning in that case anyways. > > I doubt it makes any real difference if implemented properly. > > But I guess it's more fun to speculate than to actually try it out. > :-/ and i guess it's more fun to waste time attempting to support an operating mode that the hardware has never and most likely will never support -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Fri, 29 May 2009 18:21:41 -0400 > On Fri, May 29, 2009 at 18:18, David Miller wrote: >> From: Mike Frysinger <vapier.adi@gmail.com> >>> the difference is that one of these is required in order for anything >>> to get done and the other is always useless noise. also, the Blackfin >>> core does do speculative data fetching on external memory, but not >>> MMRs, so the stalling due to the data cache line fills will be >>> mitigated unlike the useless MMR reads. >> >> You only need to read the register if the descriptor status is not >> ready yet, and you're just spinning in that case anyways. >> >> I doubt it makes any real difference if implemented properly. >> >> But I guess it's more fun to speculate than to actually try it out. >> :-/ > > and i guess it's more fun to waste time attempting to support an > operating mode that the hardware has never and most likely will never > support I guess I'll apply your patch :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2009 at 18:45, David Miller wrote: > From: Mike Frysinger <vapier.adi@gmail.com> >> On Fri, May 29, 2009 at 18:18, David Miller wrote: >>> From: Mike Frysinger <vapier.adi@gmail.com> >>>> the difference is that one of these is required in order for anything >>>> to get done and the other is always useless noise. also, the Blackfin >>>> core does do speculative data fetching on external memory, but not >>>> MMRs, so the stalling due to the data cache line fills will be >>>> mitigated unlike the useless MMR reads. >>> >>> You only need to read the register if the descriptor status is not >>> ready yet, and you're just spinning in that case anyways. >>> >>> I doubt it makes any real difference if implemented properly. >>> >>> But I guess it's more fun to speculate than to actually try it out. >>> :-/ >> >> and i guess it's more fun to waste time attempting to support an >> operating mode that the hardware has never and most likely will never >> support > > I guess I'll apply your patch :-) thanks ... i thought you might force me to implement it anyways even though i really really dont want it ;) -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c index 9f971ed..1905532 100644 --- a/drivers/net/bfin_mac.c +++ b/drivers/net/bfin_mac.c @@ -1108,7 +1108,7 @@ static int __devinit bfin_mac_probe(struct platform_device *pdev) /* now, enable interrupts */ /* register irq handler */ rc = request_irq(IRQ_MAC_RX, bfin_mac_interrupt, - IRQF_DISABLED | IRQF_SHARED, "EMAC_RX", ndev); + IRQF_DISABLED, "EMAC_RX", ndev); if (rc) { dev_err(&pdev->dev, "Cannot request Blackfin MAC RX IRQ!\n"); rc = -EBUSY;