delay via-rhine irq initialisation.
Subsystems (2)
With CONFIG_DEBUG_SHIRQ set, via-rhine complains during init.
(See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=377721 for a report).
Does this diff look right?
(I don't have a via-rhine handy to test with)
We may be able to get away with moving the request_irq to just after the alloc_tbufs(), but I feel if a real interrupt occured, this diff would stand more chance of doing the right thing.
Comments?
Dave
Delay irq registration until after we've allocated ring buffers, otherwise DEBUG_SHIRQ will complain.
drivers/net/via-rhine.c +12 -8
@@ -1151,24 +1151,28 @@ static int rhine_open(struct net_device *dev)void __iomem *ioaddr = rp->base;int rc; - rc =request_irq (rp->pdev->irq, &rhine_interrupt,IRQF_SHARED , dev->name, - dev); -if (rc) -return rc; -if (debug >1 )printk (KERN_DEBUG "%s: rhine_open() irq %d.\n" , dev->name, rp->pdev->irq); rc =alloc_ring (dev); -if (rc) { -free_irq (rp->pdev->irq, dev); +if (rc)return rc; - } +alloc_rbufs (dev);alloc_tbufs (dev);rhine_chip_reset (dev);init_registers (dev); + + rc =request_irq (rp->pdev->irq, &rhine_interrupt,IRQF_SHARED , dev->name, + dev); +if (rc) { +free_rbufs (dev); +free_tbufs (dev); +free_ring (dev); +return rc; + } +if (debug >2 ) printk(KERN_DEBUG "%s: Done rhine_open(), status %4.4x " "MII status: %4.4x.\n",
Signature · 1 line
http://www.codemonkey.org.uk
Quoted 47 lines · Dave Jones
With
CONFIG_DEBUG_SHIRQset, via-rhine complains during init.
(See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=377721 for a report).Does this diff look right?
(I don't have a via-rhine handy to test with)We may be able to get away with moving the
request_irqto just after thealloc_tbufs(), but I feel if a real interrupt occured, this diff would stand more chance of doing the right thing.Comments?
Dave
Delay irq registration until after we've allocated ring buffers, otherwise
DEBUG_SHIRQwill complain.Signed-off-by Dave Jones <[email protected]>drivers/net/via-rhine.c +2
-8@@ -1151,24 +1151,28 @@ static int rhine_open(struct net_device *dev)void __iomem *ioaddr = rp->base;int rc; - rc =request_irq (rp->pdev->irq, &rhine_interrupt,IRQF_SHARED , dev->name, - dev); -if (rc) -return rc; -if (debug >1 )printk (KERN_DEBUG "%s: rhine_open() irq %d.\n" , dev->name, rp->pdev->irq); rc =alloc_ring (dev); -if (rc) { -free_irq (rp->pdev->irq, dev); +if (rc)return rc; - } +alloc_rbufs (dev);alloc_tbufs (dev);rhine_chip_reset (dev);init_registers (dev);
drivers/net/via-rhine.c static int rhine_open(struct net_device *dev) · line 1175
Quoted 10 lines
drivers/net/via-rhine.c +10
-0+ + rc =request_irq (rp->pdev->irq, &rhine_interrupt,IRQF_SHARED , dev->name, + dev); +if (rc) { +free_rbufs (dev); +free_tbufs (dev); +free_ring (dev); +return rc; + } +
Absolutely we want to do this. DEBUG_SHIRQ is only one of many reasons -- quite simply, you are fixing an order-of-init bug that leads to races and other badness.
I would request that the error cleanup be done in the standard style for net drivers (indeed, most drivers):
rc = request_irq(...);
if (rc)
goto err_out;
...
return 0;
err_out:
...
return rc;Also I would change the subject to something like "fix order of init bugs" or "fix races" or whatnot.
We must assume, when writing drivers, that an interrupt will be delivered _immediately_ once request_irq() is called -- possibly even before request_irq() has returned its return code.
Regards,
Jeff
Quoted 9 lines · Jeff Garzik
Quoted 3 lines · Dave Jones
With
CONFIG_DEBUG_SHIRQset, via-rhine complains during init. (See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=377721 for a report).Absolutely we want to do this.
DEBUG_SHIRQis only one of many reasons -- quite simply, you are fixing an order-of-init bug that leads to races and other badness.
Such bugs seem to be more common than was thought. hisax_fcpcipnp has the same problem -- it assigns function pointers for driver interrupt processing after it requests an interrupt.