diff mbox series

[net-next] ibmvnic: merge do_change_param_reset into do_reset

Message ID 20201031094645.17255-1-ljp@linux.ibm.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [net-next] ibmvnic: merge do_change_param_reset into do_reset | expand

Commit Message

Lijun Pan Oct. 31, 2020, 9:46 a.m. UTC
Commit b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so
linkwatch_event can run") introduced do_change_param_reset function to
solve the rtnl lock issue. Majority of the code in do_change_param_reset
duplicates do_reset. Also, we can handle the rtnl lock issue in do_reset
itself. Hence merge do_change_param_reset back into do_reset to clean up
the code.

Fixes: b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run")
Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 147 +++++++++--------------------
 1 file changed, 43 insertions(+), 104 deletions(-)

Comments

Jakub Kicinski Nov. 3, 2020, 11:09 p.m. UTC | #1
On Sat, 31 Oct 2020 04:46:45 -0500 Lijun Pan wrote:
> Commit b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so
> linkwatch_event can run") introduced do_change_param_reset function to
> solve the rtnl lock issue. Majority of the code in do_change_param_reset
> duplicates do_reset. Also, we can handle the rtnl lock issue in do_reset
> itself. Hence merge do_change_param_reset back into do_reset to clean up
> the code.
> 
> Fixes: b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run")
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>

Applied, thanks!
drt Nov. 4, 2020, 9:56 p.m. UTC | #2
On 2020-11-03 15:09, Jakub Kicinski wrote:
> On Sat, 31 Oct 2020 04:46:45 -0500 Lijun Pan wrote:
>> Commit b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so
>> linkwatch_event can run") introduced do_change_param_reset function to
>> solve the rtnl lock issue. Majority of the code in 
>> do_change_param_reset
>> duplicates do_reset. Also, we can handle the rtnl lock issue in 
>> do_reset
>> itself. Hence merge do_change_param_reset back into do_reset to clean 
>> up
>> the code.
>> 
>> Fixes: b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so 
>> linkwatch_event can run")
>> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> 
> Applied, thanks!

Hi Jakub,

Thank you for applying this patch so promptly. However, I would like to 
ask that this patch be withdrawn.

1. It needs more time in testing.
2. There are a number of bug fix patches being tested. This patch would 
require rework of those patches.
3. As the lead maintainer for ibmvnic, I failed to communicate this to 
Lijun. I will do better going forward.

Please revert this commit. We will resubmit this patch later.

I sincerely apologize for any trouble this may have caused.

Regards,
Dany
Jakub Kicinski Nov. 4, 2020, 10:14 p.m. UTC | #3
On Wed, 04 Nov 2020 13:56:09 -0800 drt wrote:
> On 2020-11-03 15:09, Jakub Kicinski wrote:
> > On Sat, 31 Oct 2020 04:46:45 -0500 Lijun Pan wrote:  
> >> Commit b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so
> >> linkwatch_event can run") introduced do_change_param_reset function to
> >> solve the rtnl lock issue. Majority of the code in 
> >> do_change_param_reset
> >> duplicates do_reset. Also, we can handle the rtnl lock issue in 
> >> do_reset
> >> itself. Hence merge do_change_param_reset back into do_reset to clean 
> >> up
> >> the code.
> >> 
> >> Fixes: b27507bb59ed ("net/ibmvnic: unlock rtnl_lock in reset so 
> >> linkwatch_event can run")
> >> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>  
> > 
> > Applied, thanks!  
> 
> Hi Jakub,
> 
> Thank you for applying this patch so promptly. However, I would like to 
> ask that this patch be withdrawn.
> 
> 1. It needs more time in testing.
> 2. There are a number of bug fix patches being tested. This patch would 
> require rework of those patches.
> 3. As the lead maintainer for ibmvnic, I failed to communicate this to 
> Lijun. I will do better going forward.
> 
> Please revert this commit. We will resubmit this patch later.
> 
> I sincerely apologize for any trouble this may have caused.

No worries, please just send a revert patch with this information in
the commit message, and I'll apply right away.

BTW feel free to send a note if you need more time to review a
particular patch. Because of the volume of patches we get, I try to
apply things after a day or two. Otherwise the queue gets unmanageable.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 8148f796a807..9b35cb46a97c 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1822,86 +1822,6 @@  static int ibmvnic_set_mac(struct net_device *netdev, void *p)
 	return rc;
 }
 
-/**
- * do_change_param_reset returns zero if we are able to keep processing reset
- * events, or non-zero if we hit a fatal error and must halt.
- */
-static int do_change_param_reset(struct ibmvnic_adapter *adapter,
-				 struct ibmvnic_rwi *rwi,
-				 u32 reset_state)
-{
-	struct net_device *netdev = adapter->netdev;
-	int i, rc;
-
-	netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n",
-		   rwi->reset_reason);
-
-	netif_carrier_off(netdev);
-	adapter->reset_reason = rwi->reset_reason;
-
-	ibmvnic_cleanup(netdev);
-
-	if (reset_state == VNIC_OPEN) {
-		rc = __ibmvnic_close(netdev);
-		if (rc)
-			return rc;
-	}
-
-	release_resources(adapter);
-	release_sub_crqs(adapter, 1);
-	release_crq_queue(adapter);
-
-	adapter->state = VNIC_PROBED;
-
-	rc = init_crq_queue(adapter);
-
-	if (rc) {
-		netdev_err(adapter->netdev,
-			   "Couldn't initialize crq. rc=%d\n", rc);
-		return rc;
-	}
-
-	rc = ibmvnic_reset_init(adapter, true);
-	if (rc)
-		return IBMVNIC_INIT_FAILED;
-
-	/* If the adapter was in PROBE state prior to the reset,
-	 * exit here.
-	 */
-	if (reset_state == VNIC_PROBED)
-		return 0;
-
-	rc = ibmvnic_login(netdev);
-	if (rc) {
-		adapter->state = reset_state;
-		return rc;
-	}
-
-	rc = init_resources(adapter);
-	if (rc)
-		return rc;
-
-	ibmvnic_disable_irqs(adapter);
-
-	adapter->state = VNIC_CLOSED;
-
-	if (reset_state == VNIC_CLOSED)
-		return 0;
-
-	rc = __ibmvnic_open(netdev);
-	if (rc)
-		return IBMVNIC_OPEN_FAILED;
-
-	/* refresh device's multicast list */
-	ibmvnic_set_multi(netdev);
-
-	/* kick napi */
-	for (i = 0; i < adapter->req_rx_queues; i++)
-		napi_schedule(&adapter->napi[i]);
-
-	return 0;
-}
-
 /**
  * do_reset returns zero if we are able to keep processing reset events, or
  * non-zero if we hit a fatal error and must halt.
@@ -1917,10 +1837,12 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 	netdev_dbg(adapter->netdev, "Re-setting driver (%d)\n",
 		   rwi->reset_reason);
 
-	rtnl_lock();
+	adapter->reset_reason = rwi->reset_reason;
+	/* requestor of VNIC_RESET_CHANGE_PARAM already has the rtnl lock */
+	if (!(adapter->reset_reason == VNIC_RESET_CHANGE_PARAM))
+		rtnl_lock();
 
 	netif_carrier_off(netdev);
-	adapter->reset_reason = rwi->reset_reason;
 
 	old_num_rx_queues = adapter->req_rx_queues;
 	old_num_tx_queues = adapter->req_tx_queues;
@@ -1932,25 +1854,37 @@  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) {
-		adapter->state = VNIC_CLOSING;
+		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+			rc = __ibmvnic_close(netdev);
+			if (rc)
+				goto out;
+		} else {
+			adapter->state = VNIC_CLOSING;
 
-		/* 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
-		 * a reset.
-		 */
-		rtnl_unlock();
-		rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
-		rtnl_lock();
-		if (rc)
-			goto 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
+			 * a reset.
+			 */
+			rtnl_unlock();
+			rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
+			rtnl_lock();
+			if (rc)
+				goto out;
 
-		if (adapter->state != VNIC_CLOSING) {
-			rc = -1;
-			goto out;
+			if (adapter->state != VNIC_CLOSING) {
+				rc = -1;
+				goto out;
+			}
+
+			adapter->state = VNIC_CLOSED;
 		}
+	}
 
-		adapter->state = VNIC_CLOSED;
+	if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+		release_resources(adapter);
+		release_sub_crqs(adapter, 1);
+		release_crq_queue(adapter);
 	}
 
 	if (adapter->reset_reason != VNIC_RESET_NON_FATAL) {
@@ -1959,7 +1893,9 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 		 */
 		adapter->state = VNIC_PROBED;
 
-		if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
+		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+			rc = init_crq_queue(adapter);
+		} else if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
 			rc = ibmvnic_reenable_crq_queue(adapter);
 			release_sub_crqs(adapter, 1);
 		} else {
@@ -1999,7 +1935,11 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 			goto out;
 		}
 
-		if (adapter->req_rx_queues != old_num_rx_queues ||
+		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+			rc = init_resources(adapter);
+			if (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 !=
 		    old_num_rx_slots ||
@@ -2060,7 +2000,9 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 	rc = 0;
 
 out:
-	rtnl_unlock();
+	/* requestor of VNIC_RESET_CHANGE_PARAM should still hold the rtnl lock */
+	if (!(adapter->reset_reason == VNIC_RESET_CHANGE_PARAM))
+		rtnl_unlock();
 
 	return rc;
 }
@@ -2194,10 +2136,7 @@  static void __ibmvnic_reset(struct work_struct *work)
 		}
 		spin_unlock_irqrestore(&adapter->state_lock, flags);
 
-		if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) {
-			/* CHANGE_PARAM requestor holds rtnl_lock */
-			rc = do_change_param_reset(adapter, rwi, reset_state);
-		} else if (adapter->force_reset_recovery) {
+		if (adapter->force_reset_recovery) {
 			/* Transport event occurred during previous reset */
 			if (adapter->wait_for_reset) {
 				/* Previous was CHANGE_PARAM; caller locked */