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

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

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