| Message ID | 6f264a7a-e5da-494a-a24d-1578ca422807@VA3EHSMHS016.ehs.local (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
From: John Linn <john.linn@xilinx.com> Date: Wed, 26 May 2010 11:29:18 -0600 > The code is not checking the interrupt for DMA correctly so that an > interrupt number of 0 will cause a false error. > > Signed-off-by: Brian Hill <brian.hill@xilinx.com> > Signed-off-by: John Linn <john.linn@xilinx.com> Applied.
On Thu, May 27, 2010 at 3:29 AM, John Linn <john.linn@xilinx.com> wrote: > The code is not checking the interrupt for DMA correctly so that an > interrupt number of 0 will cause a false error. > > Signed-off-by: Brian Hill <brian.hill@xilinx.com> > Signed-off-by: John Linn <john.linn@xilinx.com> > --- > drivers/net/ll_temac_main.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c > index fa7620e..0615737 100644 > --- a/drivers/net/ll_temac_main.c > +++ b/drivers/net/ll_temac_main.c > @@ -950,7 +950,7 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match) > > lp->rx_irq = irq_of_parse_and_map(np, 0); > lp->tx_irq = irq_of_parse_and_map(np, 1); > - if (!lp->rx_irq || !lp->tx_irq) { > + if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) { Personally I think this is the right thing to do. But, I thought the IRQ 0 == NO_IRQ (AKA "all-the-world's-an-x86-and-if-not-it-should-be") holy war was already fought and won (or lost, depending on your perspective)? I seem to recall giving reluctant assent to a patch from Grant a few months ago that touched MicroBlaze thus? John
On Wed, May 26, 2010 at 10:12 PM, John Williams <john.williams@petalogix.com> wrote: > On Thu, May 27, 2010 at 3:29 AM, John Linn <john.linn@xilinx.com> wrote: >> The code is not checking the interrupt for DMA correctly so that an >> interrupt number of 0 will cause a false error. >> >> Signed-off-by: Brian Hill <brian.hill@xilinx.com> >> Signed-off-by: John Linn <john.linn@xilinx.com> >> --- >> drivers/net/ll_temac_main.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c >> index fa7620e..0615737 100644 >> --- a/drivers/net/ll_temac_main.c >> +++ b/drivers/net/ll_temac_main.c >> @@ -950,7 +950,7 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match) >> >> lp->rx_irq = irq_of_parse_and_map(np, 0); >> lp->tx_irq = irq_of_parse_and_map(np, 1); >> - if (!lp->rx_irq || !lp->tx_irq) { >> + if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) { > > Personally I think this is the right thing to do. But, I thought the > IRQ 0 == NO_IRQ (AKA "all-the-world's-an-x86-and-if-not-it-should-be") > holy war was already fought and won (or lost, depending on your > perspective)? > > I seem to recall giving reluctant assent to a patch from Grant a few > months ago that touched MicroBlaze thus? I've still got the patch in my private queue. I can reapply it, test it and repost it. I think what was still a bit up in the air was the exact method to map hw irq numbers onto linux irqs. g.
On Wed, 2010-05-26 at 11:29 -0600, John Linn wrote: > The code is not checking the interrupt for DMA correctly so that an > interrupt number of 0 will cause a false error. > > Signed-off-by: Brian Hill <brian.hill@xilinx.com> > Signed-off-by: John Linn <john.linn@xilinx.com> > --- > drivers/net/ll_temac_main.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c > index fa7620e..0615737 100644 > --- a/drivers/net/ll_temac_main.c > +++ b/drivers/net/ll_temac_main.c > @@ -950,7 +950,7 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match) > > lp->rx_irq = irq_of_parse_and_map(np, 0); > lp->tx_irq = irq_of_parse_and_map(np, 1); > - if (!lp->rx_irq || !lp->tx_irq) { > + if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) { > dev_err(&op->dev, "could not determine irqs\n"); > rc = -ENOMEM; > goto nodev; Hasn't NO_IRQ been deprecated ? Linus made it clear a while back that interrupt 0 was not valid and that's the way it should be. We now have an interrupt remapping scheme on powerpc, so we ensure that 0 always mean no interrupt. Other archs might need some fixups. Which are specifically needs this patch ? Cheers, Ben.
On Sun, Jun 6, 2010 at 3:57 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Wed, 2010-05-26 at 11:29 -0600, John Linn wrote: >> The code is not checking the interrupt for DMA correctly so that an >> interrupt number of 0 will cause a false error. >> >> Signed-off-by: Brian Hill <brian.hill@xilinx.com> >> Signed-off-by: John Linn <john.linn@xilinx.com> >> --- >> drivers/net/ll_temac_main.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c >> index fa7620e..0615737 100644 >> --- a/drivers/net/ll_temac_main.c >> +++ b/drivers/net/ll_temac_main.c >> @@ -950,7 +950,7 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match) >> >> lp->rx_irq = irq_of_parse_and_map(np, 0); >> lp->tx_irq = irq_of_parse_and_map(np, 1); >> - if (!lp->rx_irq || !lp->tx_irq) { >> + if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) { >> dev_err(&op->dev, "could not determine irqs\n"); >> rc = -ENOMEM; >> goto nodev; > > Hasn't NO_IRQ been deprecated ? ARM still uses it, plus less than a handful of other arches. There is no reason for Microblaze to use NO_IRQ as -1. In fact, on ARM, as part of the device tree work I'm hoping to piggy back in irq remapping and make 0 no longer a valid irq. > Linus made it clear a while back that interrupt 0 was not valid and > that's the way it should be. > > We now have an interrupt remapping scheme on powerpc, so we ensure that > 0 always mean no interrupt. Other archs might need some fixups. Which > are specifically needs this patch ? Microblaze. BTW jlinn, for xilinx ARM support we should make sure 0 is never used as a valid IRQ from day one. It will result it far less pain in the future. g.
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c index fa7620e..0615737 100644 --- a/drivers/net/ll_temac_main.c +++ b/drivers/net/ll_temac_main.c @@ -950,7 +950,7 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match) lp->rx_irq = irq_of_parse_and_map(np, 0); lp->tx_irq = irq_of_parse_and_map(np, 1); - if (!lp->rx_irq || !lp->tx_irq) { + if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) { dev_err(&op->dev, "could not determine irqs\n"); rc = -ENOMEM; goto nodev;