delay via-rhine irq initialisation.

delay via-rhine irq initialisation.

[Dave Jones]

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.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 07263cd..37b3efb 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -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",
-- 
http://www.codemonkey.org.uk

Re: delay via-rhine irq initialisation.

[Jeff Garzik]

Dave Jones wrote:
> 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.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
> index 07263cd..37b3efb 100644
> --- a/drivers/net/via-rhine.c
> +++ b/drivers/net/via-rhine.c
> @@ -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;
> +	}
> +

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

Re: delay via-rhine irq initialisation.

[Chuck Ebbert]

On 12/22/2007 10:36 PM, Jeff Garzik wrote:
> Dave Jones wrote:
>> With CONFIG_DEBUG_SHIRQ set, 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_SHIRQ is 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.