netdev - Re: [PATCH] natsemi: netpoll fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 Mar 2007 16:45:58 +0300
From:	Sergei Shtylyov <sshtylyov@...mvista.com>
To:	Mark Brown <broonie@...ena.org.uk>
Cc:	Mark Huth <mhuth@...sta.com>, jgarzik@...ox.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] natsemi: netpoll fixes

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@...mvista.com> for spotting the
> issue, Mark Huth <mhuth@...sta.com> for a simpler method and Simon
> Blake <simon@...ylink.co.nz> for testing resources.

> Signed-Off-By: Mark Brown <broonie@...ena.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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html