Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup()

[Veaceslav Falico <vfalico@redhat.com>]

On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote:
>From: Veaceslav Falico <vfalico@redhat.com>
>Date: Thu, 7 Mar 2013 11:03:25 +0100
>
>> @@ -680,9 +681,21 @@ static int netconsole_netdev_event(struct
>> notifier_block *this,
>>  				 * rtnl_lock already held
>>  				 */
>>  				if (nt->np.dev) {
>> +					/*
>> +					 * we still might sleep in
>> + * __netpoll_cleanup(), so release
>> +					 * the lock and restart
>
>Quite a bit of email corruption of this patch.

Sorry, somehow messed it.

>
>Also, this code block is probably too deeply indented to be sane,
>consider creating a small helper function to call instead.

It gets quite ugly if I try to move it to another function. However, maybe
something like that will work - it's effectively the same code, just that
I've moved the long part out of the if () { } block. Looks a lot more
readable, though one line still breaks 80chars limit. I've reworked the
subject/commit message too.

Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic

__netpoll_cleanup() is called in netconsole_netdev_event() while holding a
spinlock. Release/acquire the spinlock before/after it and restart the
loop.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
    drivers/net/netconsole.c |   22 +++++++++++++++-------
    1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 37add21..38eaa8c 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
    		goto done;
    
    	spin_lock_irqsave(&target_list_lock, flags);
+restart:
    	list_for_each_entry(nt, &target_list, list) {
    		netconsole_target_get(nt);
    		if (nt->np.dev == dev) {
@@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct notifier_block *this,
    				/*
    				 * rtnl_lock already held
    				 */
-				if (nt->np.dev) {
-					__netpoll_cleanup(&nt->np);
-					dev_put(nt->np.dev);
-					nt->np.dev = NULL;
+				if (!nt->np.dev) {
+					nt->enabled = 0;
+					stopped = true;
+					break;
    				}
-				nt->enabled = 0;
-				stopped = true;
-				break;
+				/*
+				 * we might sleep in __netpoll_cleanup()
+				 */
+				spin_unlock_irqrestore(&target_list_lock, flags);
+				__netpoll_cleanup(&nt->np);
+				spin_lock_irqsave(&target_list_lock, flags);
+				dev_put(nt->np.dev);
+				nt->np.dev = NULL;
+				netconsole_target_put(nt);
+				goto restart;
    			}
    		}
    		netconsole_target_put(nt);
-- 
1.7.1