diff mbox series

[net-next] Revert ibmvnic merge do_change_param_reset into do_reset

Message ID 20201106191745.1679846-1-drt@linux.ibm.com (mailing list archive)
State Accepted
Commit 9f32c27eb4fc4426eedd511697d921a932f7dba6
Delegated to: Netdev Maintainers
Headers show
Series [net-next] Revert ibmvnic merge do_change_param_reset into do_reset | expand

Commit Message

Dany Madden Nov. 6, 2020, 7:17 p.m. UTC
This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
where it restructures do_reset. There are patches being tested that
would require major rework if this is committed first. 

We will resend this after the other patches have been applied.

Signed-off-by: Dany Madden <drt@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 147 ++++++++++++++++++++---------
 1 file changed, 104 insertions(+), 43 deletions(-)

Comments

Lijun Pan Nov. 6, 2020, 7:30 p.m. UTC | #1
On 2020-11-06 13:17, Dany Madden wrote:
> This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
> where it restructures do_reset. There are patches being tested that
> would require major rework if this is committed first.
> 
> We will resend this after the other patches have been applied.

I discussed with my manager, and he has agreed not revert this one
since it is in the net-next tree and will not affect net tree for
current bug fix patches.

Sorry for the confusion.

Thanks,
Lijun

> 
> Signed-off-by: Dany Madden <drt@linux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 147 ++++++++++++++++++++---------
>  1 file changed, 104 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index f4167de30461..af4dfbe28d56 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1826,6 +1826,86 @@ 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.
> @@ -1841,12 +1921,10 @@ static int do_reset(struct ibmvnic_adapter 
> *adapter,
>  	netdev_dbg(adapter->netdev, "Re-setting driver (%d)\n",
>  		   rwi->reset_reason);
> 
> -	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();
> +	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;
> @@ -1858,37 +1936,25 @@ 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) {
> -		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;
> +		adapter->state = VNIC_CLOSING;
> 
> -			if (adapter->state != VNIC_CLOSING) {
> -				rc = -1;
> -				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;
> 
> -			adapter->state = VNIC_CLOSED;
> +		if (adapter->state != VNIC_CLOSING) {
> +			rc = -1;
> +			goto out;
>  		}
> -	}
> 
> -	if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
> -		release_resources(adapter);
> -		release_sub_crqs(adapter, 1);
> -		release_crq_queue(adapter);
> +		adapter->state = VNIC_CLOSED;
>  	}
> 
>  	if (adapter->reset_reason != VNIC_RESET_NON_FATAL) {
> @@ -1897,9 +1963,7 @@ static int do_reset(struct ibmvnic_adapter 
> *adapter,
>  		 */
>  		adapter->state = VNIC_PROBED;
> 
> -		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
> -			rc = init_crq_queue(adapter);
> -		} else if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
> +		if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
>  			rc = ibmvnic_reenable_crq_queue(adapter);
>  			release_sub_crqs(adapter, 1);
>  		} else {
> @@ -1939,11 +2003,7 @@ static int do_reset(struct ibmvnic_adapter 
> *adapter,
>  			goto out;
>  		}
> 
> -		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 ||
> +		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 ||
> @@ -2004,9 +2064,7 @@ static int do_reset(struct ibmvnic_adapter 
> *adapter,
>  	rc = 0;
> 
>  out:
> -	/* requestor of VNIC_RESET_CHANGE_PARAM should still hold the rtnl 
> lock */
> -	if (!(adapter->reset_reason == VNIC_RESET_CHANGE_PARAM))
> -		rtnl_unlock();
> +	rtnl_unlock();
> 
>  	return rc;
>  }
> @@ -2140,7 +2198,10 @@ static void __ibmvnic_reset(struct work_struct 
> *work)
>  		}
>  		spin_unlock_irqrestore(&adapter->state_lock, flags);
> 
> -		if (adapter->force_reset_recovery) {
> +		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) {
>  			/* Transport event occurred during previous reset */
>  			if (adapter->wait_for_reset) {
>  				/* Previous was CHANGE_PARAM; caller locked */
Jakub Kicinski Nov. 6, 2020, 7:42 p.m. UTC | #2
On Fri, 06 Nov 2020 13:30:25 -0600 ljp wrote:
> On 2020-11-06 13:17, Dany Madden wrote:
> > This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
> > where it restructures do_reset. There are patches being tested that
> > would require major rework if this is committed first.
> > 
> > We will resend this after the other patches have been applied.  
> 
> I discussed with my manager, and he has agreed not revert this one
> since it is in the net-next tree and will not affect net tree for
> current bug fix patches.

We merge net into net-next periodically (~every week or so) so if you
keep making changes to both branches I will have to deal with the
fallout.

I'm assuming that the resolution for the current conflict which Stephen
Rothwell sent from linux-next is correct. Please confirm.

I will resolve it like he did when Linus pulls from net (hopefully
later today).

But if you know you have more fixes I'd rather revert this, get all the
relevant fixed into net, wait for net to be merged into net-next and
then redo the refactoring.

Hope that makes sense.
Lijun Pan Nov. 6, 2020, 8:15 p.m. UTC | #3
On 2020-11-06 13:42, Jakub Kicinski wrote:
> On Fri, 06 Nov 2020 13:30:25 -0600 ljp wrote:
>> On 2020-11-06 13:17, Dany Madden wrote:
>> > This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
>> > where it restructures do_reset. There are patches being tested that
>> > would require major rework if this is committed first.
>> >
>> > We will resend this after the other patches have been applied.
>> 
>> I discussed with my manager, and he has agreed not revert this one
>> since it is in the net-next tree and will not affect net tree for
>> current bug fix patches.
> 
> We merge net into net-next periodically (~every week or so) so if you
> keep making changes to both branches I will have to deal with the
> fallout.
> 
> I'm assuming that the resolution for the current conflict which Stephen
> Rothwell sent from linux-next is correct. Please confirm.

That fix is correct.

> 
> I will resolve it like he did when Linus pulls from net (hopefully
> later today).
> 
> But if you know you have more fixes I'd rather revert this, get all the
> relevant fixed into net, wait for net to be merged into net-next and
> then redo the refactoring.
> 
> Hope that makes sense.
Lijun Pan Nov. 6, 2020, 9:02 p.m. UTC | #4
On 2020-11-06 13:42, Jakub Kicinski wrote:
> On Fri, 06 Nov 2020 13:30:25 -0600 ljp wrote:
>> On 2020-11-06 13:17, Dany Madden wrote:
>> > This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
>> > where it restructures do_reset. There are patches being tested that
>> > would require major rework if this is committed first.
>> >
>> > We will resend this after the other patches have been applied.
>> 
>> I discussed with my manager, and he has agreed not revert this one
>> since it is in the net-next tree and will not affect net tree for
>> current bug fix patches.
> 
> We merge net into net-next periodically (~every week or so) so if you
> keep making changes to both branches I will have to deal with the
> fallout.
> 
> I'm assuming that the resolution for the current conflict which Stephen
> Rothwell sent from linux-next is correct. Please confirm.
> 
> I will resolve it like he did when Linus pulls from net (hopefully
> later today).
> 
> But if you know you have more fixes I'd rather revert this, get all the
> relevant fixed into net, wait for net to be merged into net-next and
> then redo the refactoring.
> 
> Hope that makes sense.

Jakub,

We had further discussion in the team based on your comments above.
You can revert it for now.
Thanks for your efforts.

Lijun
Jakub Kicinski Nov. 7, 2020, 1:30 a.m. UTC | #5
On Fri,  6 Nov 2020 14:17:45 -0500 Dany Madden wrote:
> This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
> where it restructures do_reset. There are patches being tested that
> would require major rework if this is committed first. 
> 
> We will resend this after the other patches have been applied.
> 
> Signed-off-by: Dany Madden <drt@linux.ibm.com>

Applied, thanks.
patchwork-bot+netdevbpf@kernel.org Nov. 7, 2020, 1:40 a.m. UTC | #6
Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Fri,  6 Nov 2020 14:17:45 -0500 you wrote:
> This reverts commit 16b5f5ce351f8709a6b518cc3cbf240c378305bf
> where it restructures do_reset. There are patches being tested that
> would require major rework if this is committed first.
> 
> We will resend this after the other patches have been applied.
> 
> Signed-off-by: Dany Madden <drt@linux.ibm.com>
> 
> [...]

Here is the summary with links:
  - [net-next] Revert ibmvnic merge do_change_param_reset into do_reset
    https://git.kernel.org/netdev/net-next/c/9f32c27eb4fc

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index f4167de30461..af4dfbe28d56 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1826,6 +1826,86 @@  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.
@@ -1841,12 +1921,10 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 	netdev_dbg(adapter->netdev, "Re-setting driver (%d)\n",
 		   rwi->reset_reason);
 
-	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();
+	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;
@@ -1858,37 +1936,25 @@  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) {
-		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;
+		adapter->state = VNIC_CLOSING;
 
-			if (adapter->state != VNIC_CLOSING) {
-				rc = -1;
-				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;
 
-			adapter->state = VNIC_CLOSED;
+		if (adapter->state != VNIC_CLOSING) {
+			rc = -1;
+			goto out;
 		}
-	}
 
-	if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
-		release_resources(adapter);
-		release_sub_crqs(adapter, 1);
-		release_crq_queue(adapter);
+		adapter->state = VNIC_CLOSED;
 	}
 
 	if (adapter->reset_reason != VNIC_RESET_NON_FATAL) {
@@ -1897,9 +1963,7 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 		 */
 		adapter->state = VNIC_PROBED;
 
-		if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM) {
-			rc = init_crq_queue(adapter);
-		} else if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
+		if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
 			rc = ibmvnic_reenable_crq_queue(adapter);
 			release_sub_crqs(adapter, 1);
 		} else {
@@ -1939,11 +2003,7 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 			goto out;
 		}
 
-		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 ||
+		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 ||
@@ -2004,9 +2064,7 @@  static int do_reset(struct ibmvnic_adapter *adapter,
 	rc = 0;
 
 out:
-	/* requestor of VNIC_RESET_CHANGE_PARAM should still hold the rtnl lock */
-	if (!(adapter->reset_reason == VNIC_RESET_CHANGE_PARAM))
-		rtnl_unlock();
+	rtnl_unlock();
 
 	return rc;
 }
@@ -2140,7 +2198,10 @@  static void __ibmvnic_reset(struct work_struct *work)
 		}
 		spin_unlock_irqrestore(&adapter->state_lock, flags);
 
-		if (adapter->force_reset_recovery) {
+		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) {
 			/* Transport event occurred during previous reset */
 			if (adapter->wait_for_reset) {
 				/* Previous was CHANGE_PARAM; caller locked */