| Message ID | 20081006172653.GA26427@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
There is a simple problem with the patch which is that an "IRQ 0" can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. The problem is the check against an unsigned value for interrupts (is there any reason why you would need 4 billion interrupts possible instead of just 2 billion?) although I must say, the patch will work, and probably 99.9999999% of people will never see a problem with it :)
On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: > There is a simple problem with the patch which is that an "IRQ 0" can and does > actually exist on a bunch of platforms, at least to the best of my knowledge. > > Checking for -1 (which means for definite, no irq at all, because it is > totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227
Anton Vorontsov wrote: > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: >> There is a simple problem with the patch which is that an "IRQ 0" can and does >> actually exist on a bunch of platforms, at least to the best of my knowledge. >> >> Checking for -1 (which means for definite, no irq at all, because it is >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. > > This was discussed years ago. > > http://lkml.org/lkml/2005/11/22/159 > http://lkml.org/lkml/2005/11/22/227 > Would this break any existing platforms? If so, can those be fixed together or does it become a much bigger problem that way? Thanks.
Tejun Heo wrote: > Anton Vorontsov wrote: >> On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: >>> There is a simple problem with the patch which is that an "IRQ 0" can and does >>> actually exist on a bunch of platforms, at least to the best of my knowledge. >>> >>> Checking for -1 (which means for definite, no irq at all, because it is >>> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. >> This was discussed years ago. >> >> http://lkml.org/lkml/2005/11/22/159 >> http://lkml.org/lkml/2005/11/22/227 >> > > Would this break any existing platforms? If so, can those be fixed > together or does it become a much bigger problem that way? > Pata_of_platform stacks upon pata_platform. This patch fixes problem concerning definition of "no irq" without touch any other place. So far I can't see any new problem.
On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote: > Anton Vorontsov wrote: > > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: > >> There is a simple problem with the patch which is that an "IRQ 0" can and does > >> actually exist on a bunch of platforms, at least to the best of my knowledge. > >> > >> Checking for -1 (which means for definite, no irq at all, because it is > >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. > > > > This was discussed years ago. > > > > http://lkml.org/lkml/2005/11/22/159 > > http://lkml.org/lkml/2005/11/22/227 > > > > Would this break any existing platforms? Nope. The driver is only available for PPC platforms. As time goes by one can change `depend on PPC_OF' to just `depends on OF', so that the driver will be also available for SPARC. And still it will work, because SPARC also understands VIRQ0 as invalid VIRQ. Thanks,
> > This was discussed years ago. > > > > http://lkml.org/lkml/2005/11/22/159 > > http://lkml.org/lkml/2005/11/22/227 > > > > Would this break any existing platforms? If so, can those be fixed > together or does it become a much bigger problem that way? Zero means no IRQ. Any platform with bits of code left over exposing IRQ 0 is already not supported by lots of driver code including libata. As IRQs are unsigned using -1 is asking for trouble -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, 2008-10-07 at 13:26 +0400, Anton Vorontsov wrote: > On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote: > > Anton Vorontsov wrote: > > > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: > > >> There is a simple problem with the patch which is that an "IRQ 0" can and does > > >> actually exist on a bunch of platforms, at least to the best of my knowledge. > > >> > > >> Checking for -1 (which means for definite, no irq at all, because it is > > >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct. > > > > > > This was discussed years ago. > > > > > > http://lkml.org/lkml/2005/11/22/159 > > > http://lkml.org/lkml/2005/11/22/227 > > > > > > > Would this break any existing platforms? > > Nope. > > The driver is only available for PPC platforms. > > As time goes by one can change `depend on PPC_OF' to just `depends on > OF', so that the driver will be also available for SPARC. And still > it will work, because SPARC also understands VIRQ0 as invalid VIRQ. > Yup, I agree. I'll pick it up in my next batch. Cheers, Ben.
On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: > Zero means no IRQ. Any platform with bits of code left over exposing IRQ > 0 is already not supported by lots of driver code including libata. ...and must implement some kind of interrupt remapping crap just to work around this bogus design decision.
On Wed, 08 Oct 2008 09:40:54 +0100 David Woodhouse <dwmw2@infradead.org> wrote: > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ > > 0 is already not supported by lots of driver code including libata. > > ...and must implement some kind of interrupt remapping crap just to work > around this bogus design decision. I'll leave you to argue with Linus about that, but since that was the decision back in 2005 (for good C reasons) we can safely rely on it.
On Wed, 8 Oct 2008, Alan Cox wrote: > On Wed, 08 Oct 2008 09:40:54 +0100 > David Woodhouse <dwmw2@infradead.org> wrote: > > > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: > > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ > > > 0 is already not supported by lots of driver code including libata. > > > > ...and must implement some kind of interrupt remapping crap just to work > > around this bogus design decision. > > I'll leave you to argue with Linus about that, but since that was the > decision back in 2005 (for good C reasons) we can safely rely on it. `git grep NO_IRQ include arch/*/include' is still quite enlightening... With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010
> > I'll leave you to argue with Linus about that, but since that was the > > decision back in 2005 (for good C reasons) we can safely rely on it. > > `git grep NO_IRQ include arch/*/include' is still quite enlightening... Good guide to platform code we should delete really
On Wed, Oct 08, 2008 at 11:59:59AM +0200, Geert Uytterhoeven wrote: > On Wed, 8 Oct 2008, Alan Cox wrote: > > On Wed, 08 Oct 2008 09:40:54 +0100 > > David Woodhouse <dwmw2@infradead.org> wrote: > > > > > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: > > > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ > > > > 0 is already not supported by lots of driver code including libata. > > > > > > ...and must implement some kind of interrupt remapping crap just to work > > > around this bogus design decision. > > > > I'll leave you to argue with Linus about that, but since that was the > > decision back in 2005 (for good C reasons) we can safely rely on it. > > `git grep NO_IRQ include arch/*/include' is still quite enlightening... > In the case of PCI where IRQ is unsigned int, that's certainly bogus. The problem originates on platforms where a 1:1 mapping between vector and IRQ exists, where 0 is a valid value. This then needs to be remapped in to an IRQ cookie that has a non-0 value in order to be assigned to dev->irq. Despite whether this is bogus or not, there's not much to be done about it. Those of us with vectored IRQ platforms generally have enough sources that the use of IRQ-0 doesn't matter, and it's not worth the headache of setting up the remapping crap. As an example, on SH, IRQ-0 is mapped to vector 0x200. It is '0' for symbolic reasons only. It's just as easy to pad out irq_desc and treat it as an off-by-1 to work around all of code that assumes NO_IRQ == 0. There is enough code in the kernel today that makes the non-zero IRQ cookie assumption that platforms that do otherwise are simply broken.
Anton Vorontsov wrote: > When no irq specified the pata_of_platform fills the irq_res with -1, > which is wrong to do for two reasons: > > 1. By definition, 'no irq' should be IRQ 0, not some negative integer; > 2. pata_platform checks for irq_res.start > 0, but since irq_res.start > is unsigned type, the check will be true for `-1'. > > Reported-by: Steven A. Falco <sfalco@harris.com> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> applied to #tj-upstream (will soon be sent upstream)
Tejun Heo wrote: > Anton Vorontsov wrote: >> When no irq specified the pata_of_platform fills the irq_res with -1, >> which is wrong to do for two reasons: >> >> 1. By definition, 'no irq' should be IRQ 0, not some negative integer; >> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start >> is unsigned type, the check will be true for `-1'. >> >> Reported-by: Steven A. Falco <sfalco@harris.com> >> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> > > applied to #tj-upstream (will soon be sent upstream) I have returned! muhahahahahaha.... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Jeff Garzik wrote: > Tejun Heo wrote: >> Anton Vorontsov wrote: >>> When no irq specified the pata_of_platform fills the irq_res with -1, >>> which is wrong to do for two reasons: >>> >>> 1. By definition, 'no irq' should be IRQ 0, not some negative integer; >>> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start >>> is unsigned type, the check will be true for `-1'. >>> >>> Reported-by: Steven A. Falco <sfalco@harris.com> >>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> >> >> applied to #tj-upstream (will soon be sent upstream) > > I have returned! muhahahahahaha.... Ah.. great. Hope you enjoyed the vacation. Can you please pull from #tj-upstream? git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git tj-upstream http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=tj-upstream Nothing much. Rafael's skip-spin-off patchset + Elias's small fix + ATA_HORKAGE_ATAPI_MOD16_DMA + pata_of_platform irq fix. Gwendal posted patches to enable FIS based switching when NCQ is disabled on sata_mv but I think this should be put into separate branch and stay in linux-next till Mark Lord comes back and reviews it as I don't know much about the driver and don't know of anyone other than Gwendal and Mark who do. sata_nv detection problem is still not resolved. I figured out generic and ck804 but nf2/3 is still malfunctioning. I asked for more tests and am waiting for results. Eh... I guess that's about it. Enjoy your INBOX. :-P
On Mon, 2008-10-13 at 15:56 +0900, Tejun Heo wrote: > Anton Vorontsov wrote: > > When no irq specified the pata_of_platform fills the irq_res with -1, > > which is wrong to do for two reasons: > > > > 1. By definition, 'no irq' should be IRQ 0, not some negative integer; > > 2. pata_platform checks for irq_res.start > 0, but since irq_res.start > > is unsigned type, the check will be true for `-1'. > > > > Reported-by: Steven A. Falco <sfalco@harris.com> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> > > applied to #tj-upstream (will soon be sent upstream) Hrm... I applied it to powerpc.git too :-) Hopefully the merge will sort it out ! Cheers, Ben.
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c index 408da30..1f18ad9 100644 --- a/drivers/ata/pata_of_platform.c +++ b/drivers/ata/pata_of_platform.c @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev, ret = of_irq_to_resource(dn, 0, &irq_res); if (ret == NO_IRQ) - irq_res.start = irq_res.end = -1; + irq_res.start = irq_res.end = 0; else irq_res.flags = 0;
When no irq specified the pata_of_platform fills the irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start > 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco <sfalco@harris.com> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- Resending again... drivers/ata/pata_of_platform.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)