netdev - [PATCH 4/2] 8139cp: Do not re-enable RX interrupts in cp_tx

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]

Message-ID: <1442578664.97487.41.camel@infradead.org>
Date:	Fri, 18 Sep 2015 13:17:44 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	netdev@...r.kernel.org
Cc:	Francois Romieu <romieu@...zoreil.com>
Subject: [PATCH 4/2] 8139cp: Do not re-enable RX interrupts in
 cp_tx_timeout()

If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.

This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.

Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.

Signed-off-by: David Woodhouse <David.Woodhouse@...el.com>
---
It might seem a little odd to deliberately schedule NAPI when we know
there's going to be nothing there since we just cleared the rings.

But given that we only get here if the hardware is playing silly
buggers, I don't much like the idea of preserving the existing IntrMask
that we read from the hardware. And there's no pretty way of
determining whether NAPI is already scheduled. So just ensuring that
it *is* scheduled seems simplest. It's not like we're going to
care about the performance implications of one fruitless poll when
we've just suffered a TX timeout and reset the hardware.

In future I think I might want to fix cp_tx_timeout() to *only* reset
the TX ring anyway. Especially as I'm entirely unsure about its locking
w.r.t. cp_rx_poll()... what prevents cp_rx_poll() from running while
we're in the middle of screwing around with the RX rings?

 drivers/net/ethernet/realtek/8139cp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c
b/drivers/net/ethernet/realtek/8139cp.c
index f1054ad..d12fc50 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1269,9 +1269,10 @@ static void cp_tx_timeout(struct net_device
*dev)
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
 	__cp_set_rx_mode(dev);
-	cp_enable_irq(cp);
+	cpw16_f(IntrMask, cp_norx_intr_mask);

 	netif_wake_queue(dev);
+	napi_schedule_irqoff(&cp->napi);

 	spin_unlock_irqrestore(&cp->lock, flags);
 }
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@...el.com                              Intel Corporation

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5691 bytes)