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 <[email protected]>
---
drivers/net/ethernet/broadcom/tg3.c | 18 +++++++++++++++---
drivers/net/ethernet/broadcom/tg3.h | 1 +
2 files changed, 16 insertions(+), 3 deletions(-)
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
--
2.18.2
On Mon, Jun 15, 2020 at 12:01 PM David Christensen
<[email protected]> 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
> <[email protected]> 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
<[email protected]> wrote:
>
> On 6/15/20 1:45 PM, Michael Chan wrote:
> > On Mon, Jun 15, 2020 at 12:01 PM David Christensen
> > <[email protected]> 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.
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 <[email protected]>
---
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
On Wed, Jun 17, 2020 at 11:51 AM David Christensen
<[email protected]> 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 <[email protected]>
Reviewed-by: Michael Chan <[email protected]>
Thanks.
From: David Christensen <[email protected]>
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 <[email protected]>
Applied and queued up for -stable, thanks.
> 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 <[email protected]>
> ---
> 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
On Fri, Jul 24, 2020 at 5:19 PM David Christensen
<[email protected]> 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.