Re: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configuration

[Thinh Tran <thinhtr@linux.vnet.ibm.com>]

Hi Manish,
Do you have any other suggestion or using the extra variable, 
nic_stopped, in v2 patch is efficient enough to fix the issue?

Thanks,
Thinh Tran

On 9/28/2022 4:33 PM, Thinh Tran wrote:
> Hi Manish,
> 
> I think we need the extra variable, nic_stopped, or perhaps defining a 
> different state to determine the disablement of NAPI and freed IRQs.
> Since device gets the error, the NIC state is set to 
> BNX2X_STATE_CLOSING_WAIT4_HALT (0x4000)
> 
> 14132 static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
> 14133 {
> 14134         bp->state = BNX2X_STATE_CLOSING_WAIT4_HALT;
> 14135
> 
> and stays in this state in EEH handler and driver reset code paths, when 
> both trying to disabling the NAPI.
> 
> On 9/21/2022 3:11 PM, Thinh Tran wrote:
>> Hi Manish,
>> Thanks for reviewing the patch.
>>
>>
>> On 9/19/2022 8:53 AM, Manish Chopra wrote:
>>> Hello Thinh,
>>>
>>>> -----Original Message-----
>>>> From: Thinh Tran <thinhtr@linux.vnet.ibm.com>
>>>> Sent: Saturday, September 17, 2022 1:21 AM
>>>> To: kuba@kernel.org
>>>> Cc: netdev@vger.kernel.org; Ariel Elior <aelior@marvell.com>;
>>>> davem@davemloft.net; Manish Chopra <manishc@marvell.com>; Sudarsana
>>>> Reddy Kalluru <skalluru@marvell.com>; pabeni@redhat.com;
>>>> edumazet@google.com; Thinh Tran <thinhtr@linux.vnet.ibm.com>
>>>> Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch 
>>>> configuration
>>>>
>>>> External Email
>>>>
>>>> ----------------------------------------------------------------------
>>>> As the BCM57810 and other I/O adapters are connected through a PCIe
>>>> switch, the bnx2x driver causes unexpected system hang/crash while 
>>>> handling
>>>> PCIe switch errors, if its error handler is called after other 
>>>> drivers' handlers.
>>>>
>>>> In this case, after numbers of bnx2x_tx_timout(), the
>>>> bnx2x_nic_unload() is  called, frees up resources and calls
>>>> bnx2x_napi_disable(). Then when EEH calls its error handler, the
>>>> bnx2x_io_error_detected() and
>>>> bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the
>>>> resources.
>>>>
>>>> Signed-off-by: Thinh Tran <thinhtr@linux.vnet.ibm.com>
>>>>
>>>>   v2:
>>>>     - Check the state of the NIC before calling disable nappi
>>>>       and freeing the IRQ
>>>>     - Prevent recurrence of TX timeout by turning off the carrier,
>>>>       calling netif_carrier_off() in bnx2x_tx_timeout()
>>>>     - Check and bail out early if fp->page_pool already freed
>>>> ---
>>>>   drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  2 +
>>>>   .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c   | 27 +++++++++----
>>>>   .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h   |  3 ++
>>>>   .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 38 
>>>> +++++++++----------
>>>> .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c  | 17 +++++----
>>>>   5 files changed, 53 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>>>> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>>>> index dd5945c4bfec..11280f0eb75b 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>>>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>>>> @@ -1509,6 +1509,8 @@ struct bnx2x {
>>>>       bool            cnic_loaded;
>>>>       struct cnic_eth_dev    *(*cnic_probe)(struct net_device *);
>>>>
>>>> +    bool            nic_stopped;
>>>> +
>>>
>>> There is an already 'state' variable which holds the NIC state 
>>> whether it was opened or closed. > Perhaps we could use that easily 
>>> in bnx2x_io_slot_reset() to prevent 
>> multiple NAPI disablement
>>> instead of adding a newer one. Please see below for ex -
>>>
>> In my early debug, I did checked for bp->state variable but I was 
>> unsure what the NIC state was safe to disable the NAPI.
>> Thanks for the hint.
>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c 
>>> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>>> index 962253db25b8..c8e9b47208ed 100644
>>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>>> @@ -14256,13 +14256,16 @@ static pci_ers_result_t 
>>> bnx2x_io_slot_reset(struct pci_dev *pdev)
>>>                  }
>>>                  bnx2x_drain_tx_queues(bp);
>>>                  bnx2x_send_unload_req(bp, UNLOAD_RECOVERY);
>>> -               bnx2x_netif_stop(bp, 1);
>>> -               bnx2x_del_all_napi(bp);
>>>
>>> -               if (CNIC_LOADED(bp))
>>> -                       bnx2x_del_all_napi_cnic(bp);
>>> +               if (bp->state == BNX2X_STATE_OPEN) {
>>> +                       bnx2x_netif_stop(bp, 1);
>>> +                       bnx2x_del_all_napi(bp);
>>>
>>> -               bnx2x_free_irq(bp);
>>> +                       if (CNIC_LOADED(bp))
>>> +                               bnx2x_del_all_napi_cnic(bp);
>>> +
>>> +                       bnx2x_free_irq(bp);
>>> +               }
>>>
>>>                  /* Report UNLOAD_DONE to MCP */
>>>                  bnx2x_send_unload_done(bp, true);
>>>
> Using the v2 patch, I checked the NIC state at places before and after 
> disabling the NAPI,
>          BNX2X_ERR("before state flag = 0x%x \n", bp->state);
>          if (!bp->nic_stopped) {
>                  BNX2X_ERR("after state flag = 0x%x \n", bp->state);
>          bnx2x_netif_stop(bp, 1);
>          bnx2x_del_all_napi(bp);
>                  ....
> 
> the NIC state is always BNX2X_STATE_CLOSING_WAIT4_HALT (0x4000) when 
> device get error injection.
> 
> 1- When EEH handler code path was called first, the 
> bnx2x_io_slot_reset() does disablement the NAPI and done.
>     [28197.100795] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset 
> initializing...
>     [28197.100959] bnx2x 0201:50:00.0: enabling device (0140 -> 0142)
>     [28197.107249] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset 
> --> driver unload
>     [28199.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout 
> waiting for queue[1]: txdata->tx_pkt_prod(7270) != 
> txdata->tx_pkt_cons(7269)
>     [28201.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout 
> waiting for queue[3]: txdata->tx_pkt_prod(10977) != 
> txdata->tx_pkt_cons(10969)
>     [28203.106280] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout 
> waiting for queue[4]: txdata->tx_pkt_prod(9532) != 
> txdata->tx_pkt_cons(9514)
>     [28205.106284] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout 
> waiting for queue[6]: txdata->tx_pkt_prod(2359) != 
> txdata->tx_pkt_cons(2331)
>     [28205.314280] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- 
> state flag = 0x4000   <---- check NIC state
>     [28205.314282] bnx2x: [bnx2x_io_slot_reset:14248(eth3)]after -- 
> state flag = 0x4000         <---- check NIC state and disable the NAPI
>     [28205.405009] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 
> 'recovered'
> ........ snip ......
>     [28213.525014] EEH: Finished:'slot_reset' with aggregate recovery 
> state:'recovered'
>     [28213.525016] EEH: Notify device driver to resume
>     [28213.525017] EEH: Beginning: 'resume'
>     [28213.525018] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume()
>     [28225.115287] bnx2x 0201:50:00.0 eth3: using MSI-X  IRQs: sp 101 
> fp[0] 103 ... fp[7] 110
>     [28225.696425] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 
> 'none'
> 
> 
> 2- When the driver reset code path was called first, the 
> bnx2x_chip_cleanup() does the disablement the NAPI, but NIC is resumed 
> after the EEH handler code path was called.
>     [13376.774278] bnx2x: [bnx2x_state_wait:312(eth3)]timeout waiting 
> for state 2
>      [13376.774280] bnx2x: [bnx2x_func_stop:9118(eth3)]FUNC_STOP ramrod 
> failed. Running a dry transaction
>     [13376.774339] bnx2x: [bnx2x_chip_cleanup:9467(eth3)]before state 
> flag = 0x4000
>     [13376.774341] bnx2x: [bnx2x_chip_cleanup:9469(eth3)]after state 
> flag = 0x4000
>     [13376.774344] bnx2x: [bnx2x_igu_int_disable:891(eth3)]BUG! Proper 
> val not read from IGU!
>     [13391.774280] bnx2x: [bnx2x_fw_command:3044(eth3)]FW failed to 
> respond!
>     ........ snip .....
>     [13442.118385] PCI 0201:50:00.0#500000: EEH: Invoking 
> bnx2x->slot_reset()
>     [13442.118388] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset 
> initializing...
>     [13442.118509] bnx2x 0201:50:00.0: enabling device (0140 -> 0142)
>     [13442.124825] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset 
> --> driver unload
>     [13442.154290] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- 
> state flag = 0x4000    <--- check NIC state
>     [13442.274291] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 
> 'recovered'
>     ....... snip .........
>     [13442.424290] EEH: Finished:'slot_reset' with aggregate recovery 
> state:'recovered'
>     [13442.424291] EEH: Notify device driver to resume
>     [13442.424292] EEH: Beginning: 'resume'
>     [13442.424293] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume()
>     [13443.015280] bnx2x 0201:50:00.0 eth3: using MSI-X  IRQs: sp 61 
> fp[0] 63 ... fp[7] 75
>     [13443.616423] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 
> 'none'
> 
> 
>>> Thanks,
>>> Manish
>>
>> It requires a specific system setup, I will test with v3 patch when 
>> the system is available.
>>
>> Thanks,
>> Thinh Tran
> 
> Any suggestions?
> Thanks,
> Thinh Tran