RE: [PATCH AUTOSEL 5.13 09/21] qed: fix possible unpaired spin_{un}lock_bh in _qed_mcp_cmd_and_union()

[Justin He <Justin.He@arm.com>]

Hi, Sasha

> -----Original Message-----
> From: Sasha Levin <sashal@kernel.org>
> Sent: Tuesday, July 27, 2021 9:19 PM
> To: linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Cc: Justin He <Justin.He@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>;
> David S . Miller <davem@davemloft.net>; Sasha Levin <sashal@kernel.org>;
> netdev@vger.kernel.org
> Subject: [PATCH AUTOSEL 5.13 09/21] qed: fix possible unpaired
> spin_{un}lock_bh in _qed_mcp_cmd_and_union()
>
> From: Jia He <justin.he@arm.com>
If possible, please stop taking this commit to any stable version because
it has been reverted by later commit.

This patch is harmless but pointless.

Sorry for the inconvenience.

--
Cheers,
Justin (Jia He)


>
> [ Upstream commit 6206b7981a36476f4695d661ae139f7db36a802d ]
>
> Liajian reported a bug_on hit on a ThunderX2 arm64 server with FastLinQ
> QL41000 ethernet controller:
>  BUG: scheduling while atomic: kworker/0:4/531/0x00000200
>   [qed_probe:488()]hw prepare failed
>   kernel BUG at mm/vmalloc.c:2355!
>   Internal error: Oops - BUG: 0 [#1] SMP
>   CPU: 0 PID: 531 Comm: kworker/0:4 Tainted: G W 5.4.0-77-generic #86-
> Ubuntu
>   pstate: 00400009 (nzcv daif +PAN -UAO)
>  Call trace:
>   vunmap+0x4c/0x50
>   iounmap+0x48/0x58
>   qed_free_pci+0x60/0x80 [qed]
>   qed_probe+0x35c/0x688 [qed]
>   __qede_probe+0x88/0x5c8 [qede]
>   qede_probe+0x60/0xe0 [qede]
>   local_pci_probe+0x48/0xa0
>   work_for_cpu_fn+0x24/0x38
>   process_one_work+0x1d0/0x468
>   worker_thread+0x238/0x4e0
>   kthread+0xf0/0x118
>   ret_from_fork+0x10/0x18
>
> In this case, qed_hw_prepare() returns error due to hw/fw error, but in
> theory work queue should be in process context instead of interrupt.
>
> The root cause might be the unpaired spin_{un}lock_bh() in
> _qed_mcp_cmd_and_union(), which causes botton half is disabled incorrectly.
>
> Reported-by: Lijian Zhang <Lijian.Zhang@arm.com>
> Signed-off-by: Jia He <justin.he@arm.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_mcp.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> index cd882c453394..caeef25c89bb 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> @@ -474,14 +474,18 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>
>               spin_lock_bh(&p_hwfn->mcp_info->cmd_lock);
>
> -             if (!qed_mcp_has_pending_cmd(p_hwfn))
> +             if (!qed_mcp_has_pending_cmd(p_hwfn)) {
> +                     spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>                       break;
> +             }
>
>               rc = qed_mcp_update_pending_cmd(p_hwfn, p_ptt);
> -             if (!rc)
> +             if (!rc) {
> +                     spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>                       break;
> -             else if (rc != -EAGAIN)
> +             } else if (rc != -EAGAIN) {
>                       goto err;
> +             }
>
>               spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>
> @@ -498,6 +502,8 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>               return -EAGAIN;
>       }
>
> +     spin_lock_bh(&p_hwfn->mcp_info->cmd_lock);
> +
>       /* Send the mailbox command */
>       qed_mcp_reread_offsets(p_hwfn, p_ptt);
>       seq_num = ++p_hwfn->mcp_info->drv_mb_seq;
> @@ -524,14 +530,18 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>
>               spin_lock_bh(&p_hwfn->mcp_info->cmd_lock);
>
> -             if (p_cmd_elem->b_is_completed)
> +             if (p_cmd_elem->b_is_completed) {
> +                     spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>                       break;
> +             }
>
>               rc = qed_mcp_update_pending_cmd(p_hwfn, p_ptt);
> -             if (!rc)
> +             if (!rc) {
> +                     spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>                       break;
> -             else if (rc != -EAGAIN)
> +             } else if (rc != -EAGAIN) {
>                       goto err;
> +             }
>
>               spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>       } while (++cnt < max_retries);
> @@ -554,6 +564,7 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>               return -EAGAIN;
>       }
>
> +     spin_lock_bh(&p_hwfn->mcp_info->cmd_lock);
>       qed_mcp_cmd_del_elem(p_hwfn, p_cmd_elem);
>       spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>
> --
> 2.30.2

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.