Message ID | 1371244506-18969-1-git-send-email-nsujir@broadcom.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On 2013/06/14 14:15, Nithin Nayak Sujir wrote: [...] > @@ -17796,6 +17799,10 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) > rc = PCI_ERS_RESULT_RECOVERED; > > done: > + if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) { > + tg3_napi_enable(tp); > + dev_close(netdev); > + } > rtnl_unlock(); > > return rc; > @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) > if (err) { > tg3_full_unlock(tp); > netdev_err(netdev, "Cannot restart hardware after reset.\n"); > + tg3_napi_enable(tp); > + dev_close(netdev); > goto done; > } Are these two hunks needed? 1) These functions do not call tg3_netif_stop() or tg3_napi_disable() 2) an error in tg3_io_resume() does not trigger device removal in handle_eeh_events(). In fact the ->resume callback has no return value. > > -- > 1.8.1.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/06/17 14:28, Benjamin Poirier wrote: > On 2013/06/14 14:15, Nithin Nayak Sujir wrote: > [...] > > @@ -17796,6 +17799,10 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) > > rc = PCI_ERS_RESULT_RECOVERED; > > > > done: > > + if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) { > > + tg3_napi_enable(tp); > > + dev_close(netdev); > > + } > > rtnl_unlock(); > > > > return rc; > > @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) > > if (err) { > > tg3_full_unlock(tp); > > netdev_err(netdev, "Cannot restart hardware after reset.\n"); > > + tg3_napi_enable(tp); > > + dev_close(netdev); > > goto done; > > } > > Are these two hunks needed? > 1) These functions do not call tg3_netif_stop() or tg3_napi_disable() Ok, I see why this is relevant, since the slot_reset and resume callbacks are always called after the error_detected callback. > 2) an error in tg3_io_resume() does not trigger device removal in > handle_eeh_events(). In fact the ->resume callback has no return value. Nevertheless, this hunk > > @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) > > if (err) { > > tg3_full_unlock(tp); > > netdev_err(netdev, "Cannot restart hardware after reset.\n"); > > + tg3_napi_enable(tp); > > + dev_close(netdev); > > goto done; > > } duplicates the error handling code already in tg3_restart_hw(). > > > > > -- > > 1.8.1.4 > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-06-17 at 14:28 -0400, Benjamin Poirier wrote: > On 2013/06/14 14:15, Nithin Nayak Sujir wrote: > [...] > > @@ -17796,6 +17799,10 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) > > rc = PCI_ERS_RESULT_RECOVERED; > > > > done: > > + if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) { > > + tg3_napi_enable(tp); > > + dev_close(netdev); > > + } > > rtnl_unlock(); > > > > return rc; > > @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) > > if (err) { > > tg3_full_unlock(tp); > > netdev_err(netdev, "Cannot restart hardware after reset.\n"); > > + tg3_napi_enable(tp); > > + dev_close(netdev); > > goto done; > > } > > Are these two hunks needed? > 1) These functions do not call tg3_netif_stop() or tg3_napi_disable() > 2) an error in tg3_io_resume() does not trigger device removal in > handle_eeh_events(). In fact the ->resume callback has no return value. > The normal sequence is: error_detected(), slot_reset(), resume() In error_detected(), chip will be shutdown and NAPI will be disabled if netif_running state is true. When everything works correctly, the chip will be re-enabled in resume() and NAPI re-enabled. If we run into any error in this sequence, the sequence will not complete normally. In this case, if netif_running state is true, we know that the NAPI state has been disabled earlier in error_detected(), and we need to properly close the device. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-06-17 at 14:56 -0400, Benjamin Poirier wrote: > Nevertheless, this hunk > > > > @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) > > > if (err) { > > > tg3_full_unlock(tp); > > > netdev_err(netdev, "Cannot restart hardware after reset.\n"); > > > + tg3_napi_enable(tp); > > > + dev_close(netdev); > > > goto done; > > > } > > duplicates the error handling code already in tg3_restart_hw(). Very good point. We'll modify the patch and re-send. Thanks a lot Benjamin. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 28a645f..bfe1831 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -17747,10 +17747,13 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev, tg3_full_unlock(tp); done: - if (state == pci_channel_io_perm_failure) + if (state == pci_channel_io_perm_failure) { + tg3_napi_enable(tp); + dev_close(netdev); err = PCI_ERS_RESULT_DISCONNECT; - else + } else { pci_disable_device(pdev); + } rtnl_unlock(); @@ -17796,6 +17799,10 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) rc = PCI_ERS_RESULT_RECOVERED; done: + if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) { + tg3_napi_enable(tp); + dev_close(netdev); + } rtnl_unlock(); return rc; @@ -17826,6 +17833,8 @@ static void tg3_io_resume(struct pci_dev *pdev) if (err) { tg3_full_unlock(tp); netdev_err(netdev, "Cannot restart hardware after reset.\n"); + tg3_napi_enable(tp); + dev_close(netdev); goto done; }