[PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes
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. Add check for pcierr_recovery which skips code already executed for the "Frozen" state. Signed-off-by: David Christensen --- drivers/net/ethernet/broadcom/tg3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 7a3b22b35238..ebff1fc0d8ce 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)) + /* Could be second call or maybe we don't have netdev yet */ + if (!netdev || tp->pcierr_recovery || !netif_running(netdev)) goto done; /* We needn't recover from permanent error */ -- 2.18.2
Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes
From: David Christensen Date: Wed, 17 Jun 2020 11:51:17 -0700 > 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. > > Add check for pcierr_recovery which skips code already executed for the > "Frozen" state. > > Signed-off-by: David Christensen Applied and queued up for -stable, thanks.
Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes
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.
Add check for pcierr_recovery which skips code already executed for the
"Frozen" state.
Signed-off-by: David Christensen
---
drivers/net/ethernet/broadcom/tg3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c
b/drivers/net/ethernet/broadcom/tg3.c
index 7a3b22b35238..ebff1fc0d8ce 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))
+ /* Could be second call or maybe we don't have netdev yet */
+ if (!netdev || tp->pcierr_recovery || !netif_running(netdev))
goto done;
/* We needn't recover from permanent error */
It appears this change is not sufficient. There's another failure case
when a TX timeout occurs, causing tg3_reset_task() to run, followed by a
call to tg3_io_error_detected() by the EEH driver. Not all occurences
of this sequence cause a failure, but some do. Here's an abbreviated
version of tg3_reset_task():
static void tg3_reset_task(struct work_struct *work)
{
...
tg3_netif_stop(tp); /* >>> Calls tg3_napi_disable() */
tg3_full_lock(tp, 1);
if (tg3_flag(tp, TX_RECOVERY_PENDING)) {
tp->write32_tx_mbox = tg3_write32_tx_mbox;
tp->write32_rx_mbox = tg3_write_flush_reg32;
tg3_flag_set(tp, MBOX_WRITE_REORDER);
tg3_flag_clear(tp, TX_RECOVERY_PENDING);
}
tg3_halt(tp, RESET_KIND_SHUTDOWN, 0);
err = tg3_init_hw(tp, true);
if (err)
goto out;
tg3_netif_start(tp); /* >>> Calls tg3_napi_enable() */
out:
...
}
In the working case, tg3_init_hw() returns successfully, resulting in
every instance of napi_disable() being followed by an instance of
napi_enable().
In the failing case, tg3_hw_init() returns an error. (This is not
surprising since the system is now preventing the adapter from accessing
its MMIO registers. I'm curious why it doesn't always fail.) When
tg3_hw_init() fails, tg3_netif_start() is not called, and we end up with
two sequential calls to napi_disable(), resulting in multiple hung task
messages.
A more complete log file attached below.
Can you suggest another solution or does it make sense to reconsider my
v1 patch for the issue?
Dave
<>
Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes
On Fri, Jul 24, 2020 at 5:19 PM David Christensen wrote: > In the working case, tg3_init_hw() returns successfully, resulting in > every instance of napi_disable() being followed by an instance of > napi_enable(). > > In the failing case, tg3_hw_init() returns an error. (This is not > surprising since the system is now preventing the adapter from accessing > its MMIO registers. I'm curious why it doesn't always fail.) When > tg3_hw_init() fails, tg3_netif_start() is not called, and we end up with > two sequential calls to napi_disable(), resulting in multiple hung task > messages. > If the driver fails to initialize the chip completely, the tg3_flags should indicate we are in this failed state. We already have TG3_FLAG_INIT_COMPLETE. Perhaps, we can expand the use of this flag to cover the scenario that you described above. We can clear TG3_FLAG_INIT_COMPLETE before calling tg3_halt() and only set it back when tg3_hw_init() completes successfully. This is the rough idea, but a more detailed analysis on how this flag is used needs to be done first. Assuming this works, the EEH handler can check TG3_FLAG_INIT_COMPLETE to see if we should call tg3_netif_stop(). Another way to fix it is to call dev_close() if tg3_reset_task() fails to re-initialize the device.
Re: [PATCH v2] tg3: driver sleeps indefinitely when EEH errors exceed eeh_max_freezes
On Wed, Jun 17, 2020 at 11:51 AM David Christensen 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. > > Add check for pcierr_recovery which skips code already executed for the > "Frozen" state. > > Signed-off-by: David Christensen Reviewed-by: Michael Chan Thanks.
