| Message ID | 20200615190119.382589-1-drc@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes | expand |
On Mon, Jun 15, 2020 at 12:01 PM David Christensen <drc@linux.vnet.ibm.com> wrote: > > The driver function tg3_io_error_detected() calls napi_disable twice, > without an intervening napi_enable, when the number of EEH errors exceeds > eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock. > > The function is called once with the PCI state pci_channel_io_frozen and > then called again with the state pci_channel_io_perm_failure when the > number of EEH failures in an hour exceeds eeh_max_freezes. > > Protecting the calls to napi_enable/napi_disable with a new state > variable prevents the long sleep. This works, but I think a simpler fix is to check tp->pcierr_recovery in tg3_io_error_detected() and skip most of the tg3 calls (including the one that disables NAPI) if the flag is true.
On 6/15/20 1:45 PM, Michael Chan wrote: > On Mon, Jun 15, 2020 at 12:01 PM David Christensen > <drc@linux.vnet.ibm.com> wrote: >> >> The driver function tg3_io_error_detected() calls napi_disable twice, >> without an intervening napi_enable, when the number of EEH errors exceeds >> eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock. >> >> The function is called once with the PCI state pci_channel_io_frozen and >> then called again with the state pci_channel_io_perm_failure when the >> number of EEH failures in an hour exceeds eeh_max_freezes. >> >> Protecting the calls to napi_enable/napi_disable with a new state >> variable prevents the long sleep. > > This works, but I think a simpler fix is to check tp->pcierr_recovery > in tg3_io_error_detected() and skip most of the tg3 calls (including > the one that disables NAPI) if the flag is true. This might be the smallest change that would work. Does it make sense to the reader? diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 7a3b22b35238..1f37c69d213d 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -18168,8 +18168,8 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev, rtnl_lock(); - /* We probably don't have netdev yet */ - if (!netdev || !netif_running(netdev)) + /* May be second call or maybe we don't have netdev yet */ + if (tp->pcierr_recovery || !netdev || !netif_running(netdev)) goto done; /* We needn't recover from permanent error */
On Mon, Jun 15, 2020 at 3:21 PM David Christensen <drc@linux.vnet.ibm.com> wrote: > > On 6/15/20 1:45 PM, Michael Chan wrote: > > On Mon, Jun 15, 2020 at 12:01 PM David Christensen > > <drc@linux.vnet.ibm.com> wrote: > >> > >> The driver function tg3_io_error_detected() calls napi_disable twice, > >> without an intervening napi_enable, when the number of EEH errors exceeds > >> eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock. > >> > >> The function is called once with the PCI state pci_channel_io_frozen and > >> then called again with the state pci_channel_io_perm_failure when the > >> number of EEH failures in an hour exceeds eeh_max_freezes. > >> > >> Protecting the calls to napi_enable/napi_disable with a new state > >> variable prevents the long sleep. > > > > This works, but I think a simpler fix is to check tp->pcierr_recovery > > in tg3_io_error_detected() and skip most of the tg3 calls (including > > the one that disables NAPI) if the flag is true. > > This might be the smallest change that would work. Does it make sense > to the reader? > > diff --git a/drivers/net/ethernet/broadcom/tg3.c > b/drivers/net/ethernet/broadcom/tg3.c > index 7a3b22b35238..1f37c69d213d 100644 > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c > @@ -18168,8 +18168,8 @@ static pci_ers_result_t > tg3_io_error_detected(struct pci_dev *pdev, > > rtnl_lock(); > > - /* We probably don't have netdev yet */ > - if (!netdev || !netif_running(netdev)) > + /* May be second call or maybe we don't have netdev yet */ > + if (tp->pcierr_recovery || !netdev || !netif_running(netdev)) Dereferencing tp needs to be done after checking netdev. If we don't have netdev, tp won't be valid. Other than that, I think the logic looks good and is quite clear. Thanks.
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 7a3b22b35238..953f535e0ceb 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -7373,22 +7373,34 @@ static void tg3_napi_disable(struct tg3 *tp) { int i; + if (!tp->napi_enabled) + return; + for (i = tp->irq_cnt - 1; i >= 0; i--) napi_disable(&tp->napi[i].napi); + + tp->napi_enabled = false; } static void tg3_napi_enable(struct tg3 *tp) { int i; + if (tp->napi_enabled) + return; + for (i = 0; i < tp->irq_cnt; i++) napi_enable(&tp->napi[i].napi); + + tp->napi_enabled = true; } static void tg3_napi_init(struct tg3 *tp) { int i; + tp->napi_enabled = false; + netif_napi_add(tp->dev, &tp->napi[0].napi, tg3_poll, 64); for (i = 1; i < tp->irq_cnt; i++) netif_napi_add(tp->dev, &tp->napi[i].napi, tg3_poll_msix, 64); @@ -7400,6 +7412,8 @@ static void tg3_napi_fini(struct tg3 *tp) for (i = 0; i < tp->irq_cnt; i++) netif_napi_del(&tp->napi[i].napi); + + tp->napi_enabled = false; } static inline void tg3_netif_stop(struct tg3 *tp) @@ -18194,10 +18208,8 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev, done: if (state == pci_channel_io_perm_failure) { - if (netdev) { - tg3_napi_enable(tp); + if (netdev) dev_close(netdev); - } err = PCI_ERS_RESULT_DISCONNECT; } else { pci_disable_device(pdev); diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h index 6953d0546acb..0681f4b9ec79 100644 --- a/drivers/net/ethernet/broadcom/tg3.h +++ b/drivers/net/ethernet/broadcom/tg3.h @@ -3430,6 +3430,7 @@ struct tg3 { u32 ape_hb; unsigned long ape_hb_interval; unsigned long ape_hb_jiffies; + bool napi_enabled; }; /* Accessor macros for chip and asic attributes
The driver function tg3_io_error_detected() calls napi_disable twice, without an intervening napi_enable, when the number of EEH errors exceeds eeh_max_freezes, resulting in an indefinite sleep while holding rtnl_lock. The function is called once with the PCI state pci_channel_io_frozen and then called again with the state pci_channel_io_perm_failure when the number of EEH failures in an hour exceeds eeh_max_freezes. Protecting the calls to napi_enable/napi_disable with a new state variable prevents the long sleep. Signed-off-by: David Christensen <drc@linux.vnet.ibm.com> --- drivers/net/ethernet/broadcom/tg3.c | 18 +++++++++++++++--- drivers/net/ethernet/broadcom/tg3.h | 1 + 2 files changed, 16 insertions(+), 3 deletions(-)