| 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 |
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 */
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.
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.
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
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.
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 --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 */
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(-)