Re: [PATCH] natsemi: netpoll fixes

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Mark Brown wrote:

> Subject: natsemi: Fix NAPI for interrupt sharing

> The interrupt status register for the natsemi chips is clear on read and
> was read unconditionally from both the interrupt and from the NAPI poll
> routine, meaning that if the interrupt service routine was called (for 
> example, due to a shared interrupt) while a NAPI poll was scheduled
> interrupts could be missed.  This patch fixes that by ensuring that the
> interrupt status register is only read by the interrupt handler when
> interrupts are enabled from the chip.
> 
> It also reverts a workaround for this problem from the netpoll hook and
> improves the trace for interrupt events.
> 
> Thanks to Sergei Shtylyov <sshtylyov@ru.mvista.com> for spotting the
> issue, Mark Huth <mhuth@mvista.com> for a simpler method and Simon
> Blake <simon@citylink.co.nz> for testing resources.

> Signed-Off-By: Mark Brown <broonie@sirena.org.uk>

> Index: linux-2.6/drivers/net/natsemi.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/natsemi.c	2007-03-11 02:32:43.000000000 +0000
> +++ linux-2.6/drivers/net/natsemi.c	2007-03-13 00:12:29.000000000 +0000
[...]
> @@ -2131,17 +2133,23 @@
>  		       dev->name, np->intr_status,
>  		       readl(ioaddr + IntrMask));
>  
> -	if (!np->intr_status)
> -		return IRQ_NONE;
> -
> -	prefetch(&np->rx_skbuff[np->cur_rx % RX_RING_SIZE]);
> +	if (np->intr_status) {
> +		prefetch(&np->rx_skbuff[np->cur_rx % RX_RING_SIZE]);
>  
> -	if (netif_rx_schedule_prep(dev)) {
>  		/* Disable interrupts and register for poll */
> -		natsemi_irq_disable(dev);
> -		__netif_rx_schedule(dev);
> +		if (netif_rx_schedule_prep(dev)) {
> +			natsemi_irq_disable(dev);
> +			__netif_rx_schedule(dev);
> +		} else
> +			printk(KERN_WARNING
> +		       	       "%s: Ignoring interrupt, status %#08x, mask %#08x.\n",
> +			       dev->name, np->intr_status,
> +			       readl(ioaddr + IntrMask));
> +
> +		return IRQ_HANDLED;
>  	}
> -	return IRQ_HANDLED;
> +
> +	return IRQ_NONE;
>  }

    The only "complaint" I have is that this "restructuring" seems 
unnecessary: the only real change it does is an addition of else to the if 
statement.

WBR, Sergei