[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45F6AB16.1090300@ru.mvista.com>
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
Powered by blists - more mailing lists