On Sat, 5 Sep 2020 22:55:37 -0400 Michael Chan wrote:
> From: Vasundhara Volam <vasundhara-v.vo...@broadcom.com>
>
> bnxt_fw_reset_task() which runs from a workqueue can race with
> bnxt_remove_one(). For example, if firmware reset and VF FLR are
> happening at about the same time.
>
> bnxt_remove_one() already cancels the workqueue and waits for it
> to finish, but we need to do this earlier before the devlink
> reporters are destroyed. This will guarantee that
> the devlink reporters will always be valid when bnxt_fw_reset_task()
> is still running.
>
> Fixes: b148bb238c02 ("bnxt_en: Fix possible crash in bnxt_fw_reset_task().")
> Reviewed-by: Edwin Peer <edwin.p...@broadcom.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.vo...@broadcom.com>
> Signed-off-by: Michael Chan <michael.c...@broadcom.com>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 619eb55..8eb73fe 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -11779,6 +11779,10 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> if (BNXT_PF(bp))
> bnxt_sriov_disable(bp);
>
> + clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
> + bnxt_cancel_sp_work(bp);
> + bp->sp_event = 0;
> +
> bnxt_dl_fw_reporters_destroy(bp, true);
> if (BNXT_PF(bp))
> devlink_port_type_clear(&bp->dl_port);
> @@ -11786,9 +11790,6 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> unregister_netdev(dev);
> bnxt_dl_unregister(bp);
> bnxt_shutdown_tc(bp);
> - clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
> - bnxt_cancel_sp_work(bp);
> - bp->sp_event = 0;
>
> bnxt_clear_int_mode(bp);
> bnxt_hwrm_func_drv_unrgtr(bp);
devlink can itself scheduler a recovery via:
bnxt_fw_fatal_recover() -> bnxt_fw_reset()
no? Maybe don't make the devlink recovery path need to go via a
workqueue?
