[PATCH 1/2] net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run

Subsystems: ibm power sriov virtual nic device driver, linux for powerpc (32-bit and 64-bit), networking drivers, the rest

STALE2554d

3 messages, 2 authors, 2019-08-22 · open the first message on its own page

[PATCH 1/2] net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run

From: Juliet Kim <hidden>
Date: 2019-08-20 21:44:45

Commit a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset") 
made the change to hold the RTNL lock during a reset to avoid deadlock 
but linkwatch_event is fired during the reset and needs the RTNL lock.  
That keeps linkwatch_event process from proceeding until the reset 
is complete. The reset process cannot tolerate the linkwatch_event 
processing after reset completes, so release the RTNL lock during the 
process to allow a chance for linkwatch_event to run during reset. 
This does not guarantee that the linkwatch_event will be processed as 
soon as link state changes, but is an improvement over the current code
where linkwatch_event processing is always delayed, which prevents 
transmissions on the device from being deactivated leading transmit 
watchdog timer to time-out. 

Release the RTNL lock before link state change and re-acquire after 
the link state change to allow linkwatch_event to grab the RTNL lock 
and run during the reset.

Fixes: a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset")

Signed-off-by: Juliet Kim <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 144 +++++++++++++++++++++++++++----------
 1 file changed, 108 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index cebd20f3128d..a3e2fbb9c5db 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -1733,11 +1733,21 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	u64 old_num_rx_queues, old_num_tx_queues;
 	u64 old_num_rx_slots, old_num_tx_slots;
 	struct net_device *netdev = adapter->netdev;
+	bool we_lock_rtnl = false;
 	int i, rc;
 
 	netdev_dbg(adapter->netdev, "Re-setting driver (%d)\n",
 		   rwi->reset_reason);
 
+	/* netif_set_real_num_xx_queues needs to take rtnl lock here
+	 * unless wait_for_reset is set, in which case the rtnl lock
+	 * has already been taken before initializing the reset
+	 */
+	if (!adapter->wait_for_reset && !we_lock_rtnl) {
+		rtnl_lock();
+		we_lock_rtnl = true;
+	}
+
 	netif_carrier_off(netdev);
 	adapter->reset_reason = rwi->reset_reason;

@@ -1751,9 +1761,27 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	if (reset_state == VNIC_OPEN &&
 	    adapter->reset_reason != VNIC_RESET_MOBILITY &&
 	    adapter->reset_reason != VNIC_RESET_FAILOVER) {
-		rc = __ibmvnic_close(netdev);
+		adapter->state = VNIC_CLOSING;
+		if (!adapter->wait_for_reset && we_lock_rtnl) {
+			we_lock_rtnl = false;
+			rtnl_unlock();
+		}
+
+		rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
 		if (rc)
-			return rc;
+			goto out;
+
+		if (!adapter->wait_for_reset && !we_lock_rtnl) {
+			rtnl_lock();
+			we_lock_rtnl = true;
+		}
+
+		if (adapter->state != VNIC_CLOSING) {
+			rc = -1;
+			goto out;
+		}
+
+		adapter->state = VNIC_CLOSED;
 	}
 
 	if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM ||

@@ -1783,30 +1811,34 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 		if (rc) {
 			netdev_err(adapter->netdev,
 				   "Couldn't initialize crq. rc=%d\n", rc);
-			return rc;
+			goto out;
 		}
 
 		rc = ibmvnic_reset_init(adapter);
-		if (rc)
-			return IBMVNIC_INIT_FAILED;
+		if (rc) {
+			rc = IBMVNIC_INIT_FAILED;
+			goto out;
+		}
 
 		/* If the adapter was in PROBE state prior to the reset,
 		 * exit here.
 		 */
-		if (reset_state == VNIC_PROBED)
-			return 0;
+		if (reset_state == VNIC_PROBED) {
+			rc = 0;
+			goto out;
+		}
 
 		rc = ibmvnic_login(netdev);
 		if (rc) {
 			adapter->state = reset_state;
-			return rc;
+			goto out;
 		}
 
 		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM ||
 		    adapter->wait_for_reset) {
 			rc = init_resources(adapter);
 			if (rc)
-				return rc;
+				goto out;
 		} else if (adapter->req_rx_queues != old_num_rx_queues ||
 			   adapter->req_tx_queues != old_num_tx_queues ||
 			   adapter->req_rx_add_entries_per_subcrq !=

@@ -1820,23 +1852,25 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 
 			rc = init_resources(adapter);
 			if (rc)
-				return rc;
+				goto out;
 
 		} else {
 			rc = reset_tx_pools(adapter);
 			if (rc)
-				return rc;
+				goto out;
 
 			rc = reset_rx_pools(adapter);
 			if (rc)
-				return rc;
+				goto out;
 		}
 		ibmvnic_disable_irqs(adapter);
 	}
 	adapter->state = VNIC_CLOSED;
 
-	if (reset_state == VNIC_CLOSED)
-		return 0;
+	if (reset_state == VNIC_CLOSED) {
+		rc = 0;
+		goto out;
+	}
 
 	rc = __ibmvnic_open(netdev);
 	if (rc) {

@@ -1845,7 +1879,8 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 		else
 			adapter->state = reset_state;
 
-		return 0;
+		rc = 0;
+		goto out;
 	}
 
 	/* refresh device's multicast list */

@@ -1859,18 +1894,42 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 	    adapter->reset_reason != VNIC_RESET_CHANGE_PARAM)
 		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev);
 
+	if (!adapter->wait_for_reset && we_lock_rtnl) {
+		rtnl_unlock();
+		we_lock_rtnl = false;
+	}
+
 	return 0;
+
+out:
+	if (!adapter->wait_for_reset && we_lock_rtnl) {
+		rtnl_unlock();
+		we_lock_rtnl = false;
+	}
+
+	return rc;
 }
 
 static int do_hard_reset(struct ibmvnic_adapter *adapter,
 			 struct ibmvnic_rwi *rwi, u32 reset_state)
 {
 	struct net_device *netdev = adapter->netdev;
+	bool we_lock_rtnl = false;
 	int rc;
 
 	netdev_dbg(adapter->netdev, "Hard resetting driver (%d)\n",
 		   rwi->reset_reason);
 
+	/* netif_set_real_num_xx_queues needs to take rtnl lock here
+	 * unless wait_for_reset is set, in which case the rtnl lock
+	 * has already been taken before initializing the reset
+	 */
+	if (!adapter->wait_for_reset && !we_lock_rtnl) {
+		rtnl_lock();
+		we_lock_rtnl = true;
+	}
+
+	adapter->force_reset_recovery = false;
 	netif_carrier_off(netdev);
 	adapter->reset_reason = rwi->reset_reason;

@@ -1889,34 +1948,39 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 	if (rc) {
 		netdev_err(adapter->netdev,
 			   "Couldn't initialize crq. rc=%d\n", rc);
-		return rc;
+		goto out;
 	}
 
 	rc = ibmvnic_init(adapter);
 	if (rc)
-		return rc;
+		goto out;
 
 	/* If the adapter was in PROBE state prior to the reset,
 	 * exit here.
 	 */
-	if (reset_state == VNIC_PROBED)
-		return 0;
+	if (reset_state == VNIC_PROBED) {
+		rc = 0;
+		goto out;
+	}
 
 	rc = ibmvnic_login(netdev);
 	if (rc) {
 		adapter->state = VNIC_PROBED;
-		return 0;
+		rc = 0;
+		goto out;
 	}
 
 	rc = init_resources(adapter);
 	if (rc)
-		return rc;
+		goto out;
 
 	ibmvnic_disable_irqs(adapter);
 	adapter->state = VNIC_CLOSED;
 
-	if (reset_state == VNIC_CLOSED)
-		return 0;
+	if (reset_state == VNIC_CLOSED) {
+		rc = 0;
+		goto out;
+	}
 
 	rc = __ibmvnic_open(netdev);
 	if (rc) {

@@ -1925,10 +1989,25 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
 		else
 			adapter->state = reset_state;
 
-		return 0;
+		rc = 0;
+		goto out;
+	}
+
+	if (!adapter->wait_for_reset && we_lock_rtnl) {
+		rtnl_unlock();
+		we_lock_rtnl = false;
 	}
 
 	return 0;
+
+out:
+	if (!adapter->wait_for_reset && we_lock_rtnl) {
+		rtnl_unlock();
+		we_lock_rtnl = false;
+	}
+
+	return rc;
+
 }
 
 static struct ibmvnic_rwi *get_next_rwi(struct ibmvnic_adapter *adapter)

@@ -1965,26 +2044,16 @@ static void __ibmvnic_reset(struct work_struct *work)
 {
 	struct ibmvnic_rwi *rwi;
 	struct ibmvnic_adapter *adapter;
-	bool we_lock_rtnl = false;
 	u32 reset_state;
 	int rc = 0;
 
 	adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
 
-	/* netif_set_real_num_xx_queues needs to take rtnl lock here
-	 * unless wait_for_reset is set, in which case the rtnl lock
-	 * has already been taken before initializing the reset
-	 */
-	if (!adapter->wait_for_reset) {
-		rtnl_lock();
-		we_lock_rtnl = true;
-	}
 	reset_state = adapter->state;
 
 	rwi = get_next_rwi(adapter);
 	while (rwi) {
 		if (adapter->force_reset_recovery) {
-			adapter->force_reset_recovery = false;
 			rc = do_hard_reset(adapter, rwi, reset_state);
 		} else {
 			rc = do_reset(adapter, rwi, reset_state);

@@ -1995,6 +2064,11 @@ static void __ibmvnic_reset(struct work_struct *work)
 			break;
 
 		rwi = get_next_rwi(adapter);
+
+		if (rwi && (rwi->reset_reason == VNIC_RESET_FAILOVER ||
+			    rwi->reset_reason == VNIC_RESET_MOBILITY) &&
+			!adapter->force_reset_recovery)
+			adapter->force_reset_recovery = true;
 	}
 
 	if (adapter->wait_for_reset) {

@@ -2009,8 +2083,6 @@ static void __ibmvnic_reset(struct work_struct *work)
 	}
 
 	adapter->resetting = false;
-	if (we_lock_rtnl)
-		rtnl_unlock();
 }
 
 static int ibmvnic_reset(struct ibmvnic_adapter *adapter,

-- 
2.16.4

[PATCH 2/2] net/ibmvnic: prevent more than one thread from running in reset

From: Juliet Kim <hidden>
Date: 2019-08-20 21:46:31

The current code allows more than one thread to run in reset. This can 
corrupt struct adapter data. Check adapter->resetting before performing 
a reset, if there is another reset running delay (100 msec) before trying 
again.

Signed-off-by: Juliet Kim <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 19 ++++++++++++++++++-
 drivers/net/ethernet/ibm/ibmvnic.h |  3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index a3e2fbb9c5db..7db7a44eeeb9 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -2049,6 +2049,13 @@ static void __ibmvnic_reset(struct work_struct *work)
 
 	adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
 
+	if (adapter->resetting) {
+		schedule_delayed_work(&adapter->ibmvnic_delayed_reset,
+				      IBMVNIC_RESET_DELAY);
+		return;
+	}
+
+	adapter->resetting = true;
 	reset_state = adapter->state;
 
 	rwi = get_next_rwi(adapter);

@@ -2085,6 +2092,15 @@ static void __ibmvnic_reset(struct work_struct *work)
 	adapter->resetting = false;
 }
 
+static void __ibmvnic_delayed_reset(struct work_struct *work)
+{
+	struct ibmvnic_adapter *adapter;
+
+	adapter = container_of(work, struct ibmvnic_adapter,
+			       ibmvnic_delayed_reset.work);
+	__ibmvnic_reset(&adapter->ibmvnic_reset);
+}
+
 static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 			 enum ibmvnic_reset_reason reason)
 {

@@ -2137,7 +2153,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 	rwi->reset_reason = reason;
 	list_add_tail(&rwi->list, &adapter->rwi_list);
 	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
-	adapter->resetting = true;
 	netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", reason);
 	schedule_work(&adapter->ibmvnic_reset);

@@ -4910,6 +4925,8 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	spin_lock_init(&adapter->stats_lock);
 
 	INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset);
+	INIT_DELAYED_WORK(&adapter->ibmvnic_delayed_reset,
+			  __ibmvnic_delayed_reset);
 	INIT_LIST_HEAD(&adapter->rwi_list);
 	spin_lock_init(&adapter->rwi_lock);
 	init_completion(&adapter->init_done);

diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 70bd286f8932..0e1fadbcffc0 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h

@@ -38,6 +38,8 @@
 #define IBMVNIC_MAX_LTB_SIZE ((1 << (MAX_ORDER - 1)) * PAGE_SIZE)
 #define IBMVNIC_BUFFER_HLEN 500
 
+#define IBMVNIC_RESET_DELAY 100
+
 static const char ibmvnic_priv_flags[][ETH_GSTRING_LEN] = {
 #define IBMVNIC_USE_SERVER_MAXES 0x1
 	"use-server-maxes"

@@ -1076,6 +1078,7 @@ struct ibmvnic_adapter {
 	spinlock_t rwi_lock;
 	struct list_head rwi_list;
 	struct work_struct ibmvnic_reset;
+	struct delayed_work ibmvnic_delayed_reset;
 	bool resetting;
 	bool napi_enabled, from_passive_init;

-- 
2.16.4

Re: [PATCH 1/2] net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run

From: David Miller <davem@davemloft.net>
Date: 2019-08-22 04:08:19

From: Juliet Kim <redacted>
Date: Tue, 20 Aug 2019 17:31:19 -0400

Commit a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset") 
made the change to hold the RTNL lock during a reset to avoid deadlock 
but linkwatch_event is fired during the reset and needs the RTNL lock.  
That keeps linkwatch_event process from proceeding until the reset 
is complete. The reset process cannot tolerate the linkwatch_event 
processing after reset completes, so release the RTNL lock during the 
process to allow a chance for linkwatch_event to run during reset. 
This does not guarantee that the linkwatch_event will be processed as 
soon as link state changes, but is an improvement over the current code
where linkwatch_event processing is always delayed, which prevents 
transmissions on the device from being deactivated leading transmit 
watchdog timer to time-out. 

Release the RTNL lock before link state change and re-acquire after 
the link state change to allow linkwatch_event to grab the RTNL lock 
and run during the reset.

Fixes: a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset")
Signed-off-by: Juliet Kim <redacted>

Conditional locking, especialy such extensive use of conditional
locking as is being done here, is strongly discouraged and is always
indicative of bad design.

Please try to rework this change such that the code paths that want
to lock things a certain way are %100 segregated functionally into
different code paths and functions.

Or feel free to find a cleaner way to fix this.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help