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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Sep 2022 15:11:55 -0500
From:   Thinh Tran <thinhtr@...ux.vnet.ibm.com>
To:     Manish Chopra <manishc@...vell.com>,
        "kuba@...nel.org" <kuba@...nel.org>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <aelior@...vell.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Sudarsana Reddy Kalluru <skalluru@...vell.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Alok Prasad <palok@...vell.com>
Subject: Re: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch
 configuration

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@...ux.vnet.ibm.com>
>> Sent: Saturday, September 17, 2022 1:21 AM
>> To: kuba@...nel.org
>> Cc: netdev@...r.kernel.org; Ariel Elior <aelior@...vell.com>;
>> davem@...emloft.net; Manish Chopra <manishc@...vell.com>; Sudarsana
>> Reddy Kalluru <skalluru@...vell.com>; pabeni@...hat.com;
>> edumazet@...gle.com; Thinh Tran <thinhtr@...ux.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@...ux.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);
> 
> Thanks,
> Manish

It requires a specific system setup, I will test with v3 patch when the 
system is available.

Thanks,
Thinh Tran