* [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
@ 2026-02-23 21:49 ` Bart Van Assche
2026-02-23 22:23 ` Jakub Kicinski
2026-02-23 21:49 ` [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up() Bart Van Assche
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 21:49 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Bart Van Assche, Shantiprasad Shettar, netdev
Pass the same argument to netdev_lock() and netdev_unlock(). This patch
prepares for enabling the Clang thread-safety analysis functionality. No
functional change intended.
Cc: Shantiprasad Shettar <shantiprasad.shettar@broadcom.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index e062d5d400da..950708575268 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17121,7 +17121,7 @@ static int bnxt_resume(struct device *device)
}
resume_exit:
- netdev_unlock(bp->dev);
+ netdev_unlock(dev);
bnxt_ulp_start(bp, rc);
if (!rc)
bnxt_reenable_sriov(bp);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
2026-02-23 21:49 ` [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze Bart Van Assche
@ 2026-02-23 21:49 ` Bart Van Assche
2026-02-23 22:17 ` Michael Chan
2026-02-23 21:49 ` [PATCH 20/62] octeontx2-pf: Fix locking in an error path Bart Van Assche
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 21:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Bart Van Assche, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
bnxt_dl_reload_down() calls rtnl_lock() and netdev_lock() if it returns
0. Hence, bnxt_dl_reload_up() should invoke the corresponding unlock
calls. This issue has been detected by the clang thread-sanitizer.
Compile-tested only.
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Shantiprasad Shettar <shantiprasad.shettar@broadcom.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: netdev@vger.kernel.org
Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 15de802bbac4..1e9a3454bb29 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -562,6 +562,8 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
break;
}
default:
+ netdev_unlock(bp->dev);
+ rtnl_unlock();
return -EOPNOTSUPP;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 20/62] octeontx2-pf: Fix locking in an error path
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
2026-02-23 21:49 ` [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze Bart Van Assche
2026-02-23 21:49 ` [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up() Bart Van Assche
@ 2026-02-23 21:49 ` Bart Van Assche
2026-02-23 21:49 ` [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze Bart Van Assche
2026-02-23 21:49 ` [PATCH 22/62] mctp i3c: Fix locking in error paths Bart Van Assche
4 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 21:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Bart Van Assche, Naveen Mamindlapalli, Jakub Kicinski,
Sunil Goutham, Geetha sowjanya, Subbaraya Sundeep, hariprasad,
Bharat Bhushan, netdev
Only unlock pf->mbox.lock if it has been locked by otx2_do_set_vf_vlan().
This bug has been detected by the Clang thread-safety analyzer.
Cc: Naveen Mamindlapalli <naveenm@marvell.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Sunil Goutham <sgoutham@marvell.com>
Cc: Geetha sowjanya <gakula@marvell.com>
Cc: Subbaraya Sundeep <sbhatta@marvell.com>
Cc: hariprasad <hkelam@marvell.com>
Cc: Bharat Bhushan <bbhushan2@marvell.com>
Cc: netdev@vger.kernel.org
Fixes: f0c2982aaf98 ("octeontx2-pf: Add support for SR-IOV management functions")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
index ee623476e5ff..8c9f08ed90fd 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
@@ -2588,7 +2588,7 @@ static int otx2_do_set_vf_vlan(struct otx2_nic *pf, int vf, u16 vlan, u8 qos,
config = &pf->vf_configs[vf];
if (!vlan && !config->vlan)
- goto out;
+ return err;
mutex_lock(&pf->mbox.lock);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
` (2 preceding siblings ...)
2026-02-23 21:49 ` [PATCH 20/62] octeontx2-pf: Fix locking in an error path Bart Van Assche
@ 2026-02-23 21:49 ` Bart Van Assche
2026-02-23 21:49 ` [PATCH 22/62] mctp i3c: Fix locking in error paths Bart Van Assche
4 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 21:49 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Bart Van Assche, Manish Chopra, netdev
Make the implementation of this function compatible with clang's
compile-time thread-safety analysis by moving error-handling code to the
end of this function. No functionality has been changed.
Cc: Manish Chopra <manishc@marvell.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 56 ++++++++++++-----------
1 file changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
index 7e37fe631a58..462e758c5890 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -467,7 +467,7 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
int rc = 0;
/* Wait until the mailbox is non-occupied */
- do {
+ for (;;) {
/* Exit the loop if there is no pending command, or if the
* pending command is completed during this iteration.
* The spinlock stays locked until the command is sent.
@@ -486,18 +486,14 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
+ if (++cnt >= QED_DRV_MB_MAX_RETRIES)
+ goto retries_exceeded_1;
+
if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP))
usleep_range(QED_MCP_RESP_ITER_US,
QED_MCP_RESP_ITER_US * 2);
else
udelay(QED_MCP_RESP_ITER_US);
- } while (++cnt < QED_DRV_MB_MAX_RETRIES);
-
- if (cnt >= QED_DRV_MB_MAX_RETRIES) {
- DP_NOTICE(p_hwfn,
- "The MFW mailbox is occupied by an uncompleted command. Failed to send command 0x%08x [param 0x%08x].\n",
- p_mb_params->cmd, p_mb_params->param);
- return -EAGAIN;
}
/* Send the mailbox command */
@@ -513,7 +509,7 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
/* Wait for the MFW response */
- do {
+ for (;;) {
/* Exit the loop if the command is already completed, or if the
* command is completed during this iteration.
* The spinlock stays locked until the list element is removed.
@@ -537,24 +533,9 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
goto err;
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
- } while (++cnt < QED_DRV_MB_MAX_RETRIES);
-
- if (cnt >= QED_DRV_MB_MAX_RETRIES) {
- DP_NOTICE(p_hwfn,
- "The MFW failed to respond to command 0x%08x [param 0x%08x].\n",
- p_mb_params->cmd, p_mb_params->param);
- qed_mcp_print_cpu_info(p_hwfn, p_ptt);
-
- 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);
- if (!QED_MB_FLAGS_IS_SET(p_mb_params, AVOID_BLOCK))
- qed_mcp_cmd_set_blocking(p_hwfn, true);
-
- qed_hw_err_notify(p_hwfn, p_ptt,
- QED_HW_ERR_MFW_RESP_FAIL, NULL);
- return -EAGAIN;
+ if (++cnt >= QED_DRV_MB_MAX_RETRIES)
+ goto retries_exceeded_2;
}
qed_mcp_cmd_del_elem(p_hwfn, p_cmd_elem);
@@ -576,6 +557,29 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
err:
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
return rc;
+
+retries_exceeded_1:
+ DP_NOTICE(p_hwfn,
+ "The MFW mailbox is occupied by an uncompleted command. Failed to send command 0x%08x [param 0x%08x].\n",
+ p_mb_params->cmd, p_mb_params->param);
+ return -EAGAIN;
+
+retries_exceeded_2:
+ DP_NOTICE(p_hwfn,
+ "The MFW failed to respond to command 0x%08x [param 0x%08x].\n",
+ p_mb_params->cmd, p_mb_params->param);
+ qed_mcp_print_cpu_info(p_hwfn, p_ptt);
+
+ 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);
+
+ if (!QED_MB_FLAGS_IS_SET(p_mb_params, AVOID_BLOCK))
+ qed_mcp_cmd_set_blocking(p_hwfn, true);
+
+ qed_hw_err_notify(p_hwfn, p_ptt,
+ QED_HW_ERR_MFW_RESP_FAIL, NULL);
+ return -EAGAIN;
}
static int qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 22/62] mctp i3c: Fix locking in error paths
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
` (3 preceding siblings ...)
2026-02-23 21:49 ` [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze Bart Van Assche
@ 2026-02-23 21:49 ` Bart Van Assche
4 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 21:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Bart Van Assche, Leo Yang, Paolo Abeni, Jeremy Kerr,
Matt Johnston, netdev
Only unlock mi->lock if it has been locked. This bug has been detected by
the Clang thread-safety analyzer.
Cc: Leo Yang <Leo-Yang@quantatw.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Matt Johnston <matt@codeconstruct.com.au>
Cc: netdev@vger.kernel.org
Fixes: 2d2d4f60ed26 ("mctp i3c: fix MCTP I3C driver multi-thread issue")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/mctp/mctp-i3c.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/mctp/mctp-i3c.c b/drivers/net/mctp/mctp-i3c.c
index 6d2bbae7477b..d4a29912c7e1 100644
--- a/drivers/net/mctp/mctp-i3c.c
+++ b/drivers/net/mctp/mctp-i3c.c
@@ -112,7 +112,7 @@ static int mctp_i3c_read(struct mctp_i3c_device *mi)
if (!skb) {
stats->rx_dropped++;
rc = -ENOMEM;
- goto err;
+ goto free_skb;
}
skb->protocol = htons(ETH_P_MCTP);
@@ -170,8 +170,11 @@ static int mctp_i3c_read(struct mctp_i3c_device *mi)
mutex_unlock(&mi->lock);
return 0;
+
err:
mutex_unlock(&mi->lock);
+
+free_skb:
kfree_skb(skb);
return rc;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze
[not found] <20260223220102.2158611-1-bart.vanassche@linux.dev>
@ 2026-02-23 22:00 ` Bart Van Assche
0 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 22:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long, linux-kernel,
Marco Elver, Christoph Hellwig, Steven Rostedt, Nick Desaulniers,
Nathan Chancellor, Kees Cook, Jann Horn, Bart Van Assche,
Manish Chopra, netdev
From: Bart Van Assche <bvanassche@acm.org>
Make the implementation of this function compatible with clang's
compile-time thread-safety analysis by moving error-handling code to the
end of this function. No functionality has been changed.
Cc: Manish Chopra <manishc@marvell.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 56 ++++++++++++-----------
1 file changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
index 7e37fe631a58..462e758c5890 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -467,7 +467,7 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
int rc = 0;
/* Wait until the mailbox is non-occupied */
- do {
+ for (;;) {
/* Exit the loop if there is no pending command, or if the
* pending command is completed during this iteration.
* The spinlock stays locked until the command is sent.
@@ -486,18 +486,14 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
+ if (++cnt >= QED_DRV_MB_MAX_RETRIES)
+ goto retries_exceeded_1;
+
if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP))
usleep_range(QED_MCP_RESP_ITER_US,
QED_MCP_RESP_ITER_US * 2);
else
udelay(QED_MCP_RESP_ITER_US);
- } while (++cnt < QED_DRV_MB_MAX_RETRIES);
-
- if (cnt >= QED_DRV_MB_MAX_RETRIES) {
- DP_NOTICE(p_hwfn,
- "The MFW mailbox is occupied by an uncompleted command. Failed to send command 0x%08x [param 0x%08x].\n",
- p_mb_params->cmd, p_mb_params->param);
- return -EAGAIN;
}
/* Send the mailbox command */
@@ -513,7 +509,7 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
/* Wait for the MFW response */
- do {
+ for (;;) {
/* Exit the loop if the command is already completed, or if the
* command is completed during this iteration.
* The spinlock stays locked until the list element is removed.
@@ -537,24 +533,9 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
goto err;
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
- } while (++cnt < QED_DRV_MB_MAX_RETRIES);
-
- if (cnt >= QED_DRV_MB_MAX_RETRIES) {
- DP_NOTICE(p_hwfn,
- "The MFW failed to respond to command 0x%08x [param 0x%08x].\n",
- p_mb_params->cmd, p_mb_params->param);
- qed_mcp_print_cpu_info(p_hwfn, p_ptt);
-
- 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);
- if (!QED_MB_FLAGS_IS_SET(p_mb_params, AVOID_BLOCK))
- qed_mcp_cmd_set_blocking(p_hwfn, true);
-
- qed_hw_err_notify(p_hwfn, p_ptt,
- QED_HW_ERR_MFW_RESP_FAIL, NULL);
- return -EAGAIN;
+ if (++cnt >= QED_DRV_MB_MAX_RETRIES)
+ goto retries_exceeded_2;
}
qed_mcp_cmd_del_elem(p_hwfn, p_cmd_elem);
@@ -576,6 +557,29 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
err:
spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
return rc;
+
+retries_exceeded_1:
+ DP_NOTICE(p_hwfn,
+ "The MFW mailbox is occupied by an uncompleted command. Failed to send command 0x%08x [param 0x%08x].\n",
+ p_mb_params->cmd, p_mb_params->param);
+ return -EAGAIN;
+
+retries_exceeded_2:
+ DP_NOTICE(p_hwfn,
+ "The MFW failed to respond to command 0x%08x [param 0x%08x].\n",
+ p_mb_params->cmd, p_mb_params->param);
+ qed_mcp_print_cpu_info(p_hwfn, p_ptt);
+
+ 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);
+
+ if (!QED_MB_FLAGS_IS_SET(p_mb_params, AVOID_BLOCK))
+ qed_mcp_cmd_set_blocking(p_hwfn, true);
+
+ qed_hw_err_notify(p_hwfn, p_ptt,
+ QED_HW_ERR_MFW_RESP_FAIL, NULL);
+ return -EAGAIN;
}
static int qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
2026-02-23 21:49 ` [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up() Bart Van Assche
@ 2026-02-23 22:17 ` Michael Chan
2026-02-23 22:27 ` Bart Van Assche
0 siblings, 1 reply; 14+ messages in thread
From: Michael Chan @ 2026-02-23 22:17 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
.
On Mon, Feb 23, 2026 at 1:52 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> bnxt_dl_reload_down() calls rtnl_lock() and netdev_lock() if it returns
> 0. Hence, bnxt_dl_reload_up() should invoke the corresponding unlock
> calls. This issue has been detected by the clang thread-sanitizer.
> Compile-tested only.
>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Shantiprasad Shettar <shantiprasad.shettar@broadcom.com>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: netdev@vger.kernel.org
> Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 15de802bbac4..1e9a3454bb29 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -562,6 +562,8 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
> break;
> }
> default:
> + netdev_unlock(bp->dev);
> + rtnl_unlock();
This doesn't look correct. The 2 locks are taken in
bnxt_dl_reload_down() only for the 2 actions
DEVLINK_RELOAD_ACTION_DRIVER_REINIT and
DEVLINK_RELOAD_ACTION_FW_ACTIVATE. For the default action, no locks
are taken so I don't think we should unlock here.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze
2026-02-23 21:49 ` [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze Bart Van Assche
@ 2026-02-23 22:23 ` Jakub Kicinski
2026-02-23 22:24 ` Jakub Kicinski
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2026-02-23 22:23 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Peter Zijlstra, Shantiprasad Shettar, netdev
On Mon, 23 Feb 2026 13:49:05 -0800 Bart Van Assche wrote:
> Pass the same argument to netdev_lock() and netdev_unlock(). This patch
> prepares for enabling the Clang thread-safety analysis functionality. No
> functional change intended.
Please split out the 13(?) netdev patches to a separate series.
If you CC us on a subset of the series out CI will not test it.
Please keep in mind that we ask to wait at least 24h between
resubmissions
--
pw-bot: cr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze
2026-02-23 22:23 ` Jakub Kicinski
@ 2026-02-23 22:24 ` Jakub Kicinski
2026-02-23 22:30 ` Bart Van Assche
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2026-02-23 22:24 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Peter Zijlstra, Shantiprasad Shettar, netdev
On Mon, 23 Feb 2026 14:23:44 -0800 Jakub Kicinski wrote:
> On Mon, 23 Feb 2026 13:49:05 -0800 Bart Van Assche wrote:
> > Pass the same argument to netdev_lock() and netdev_unlock(). This patch
> > prepares for enabling the Clang thread-safety analysis functionality. No
> > functional change intended.
>
> Please split out the 13(?) netdev patches to a separate series.
> If you CC us on a subset of the series out CI will not test it.
> Please keep in mind that we ask to wait at least 24h between
> resubmissions
Oh, not 13, 5, you already spammed us 3 times with this. Sigh.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
2026-02-23 22:17 ` Michael Chan
@ 2026-02-23 22:27 ` Bart Van Assche
2026-02-23 22:37 ` Michael Chan
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 22:27 UTC (permalink / raw)
To: Michael Chan
Cc: Peter Zijlstra, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
On 2/23/26 2:17 PM, Michael Chan wrote:
> .
> On Mon, Feb 23, 2026 at 1:52 PM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> bnxt_dl_reload_down() calls rtnl_lock() and netdev_lock() if it returns
>> 0. Hence, bnxt_dl_reload_up() should invoke the corresponding unlock
>> calls. This issue has been detected by the clang thread-sanitizer.
>> Compile-tested only.
>>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Shantiprasad Shettar <shantiprasad.shettar@broadcom.com>
>> Cc: Stanislav Fomichev <sdf@fomichev.me>
>> Cc: netdev@vger.kernel.org
>> Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> index 15de802bbac4..1e9a3454bb29 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> @@ -562,6 +562,8 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
>> break;
>> }
>> default:
>> + netdev_unlock(bp->dev);
>> + rtnl_unlock();
>
> This doesn't look correct. The 2 locks are taken in
> bnxt_dl_reload_down() only for the 2 actions
> DEVLINK_RELOAD_ACTION_DRIVER_REINIT and
> DEVLINK_RELOAD_ACTION_FW_ACTIVATE. For the default action, no locks
> are taken so I don't think we should unlock here.
Hi Michael,
My understanding is that bnxt_dl_reload_up() is only called if
bnxt_dl_reload_down() returns 0 (see also devlink_reload()).
bnxt_dl_reload_up() returns an error code (-EOPNOTSUPP) for other
actions than DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
DEVLINK_RELOAD_ACTION_FW_ACTIVATE. Hence, bnxt_dl_reload_up() is only
called if "action" is either DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
DEVLINK_RELOAD_ACTION_FW_ACTIVATE. In other words, the code in the
"default" clause will never be reached. So I think the above change is
correct but also that the patch description should be modified to
reflect that this is not a bug fix. Did I perhaps misunderstand
something?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze
2026-02-23 22:24 ` Jakub Kicinski
@ 2026-02-23 22:30 ` Bart Van Assche
0 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 22:30 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Peter Zijlstra, Shantiprasad Shettar, netdev
On 2/23/26 2:24 PM, Jakub Kicinski wrote:
> Oh, not 13, 5, you already spammed us 3 times with this. Sigh.
Sorry for the spam. I have not yet found an SMTP server that can post
the entire patch series without reporting an error message halfway.
smtpauth.mailroute.net reported "quota exceeded" while
smtp.migadu.com reported "4.7.1 Error: too many recipients from
2a00:79e0:2e7c:8:b343:b0de:3e59:4e65".
Bart.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
2026-02-23 22:27 ` Bart Van Assche
@ 2026-02-23 22:37 ` Michael Chan
2026-02-23 22:56 ` Bart Van Assche
0 siblings, 1 reply; 14+ messages in thread
From: Michael Chan @ 2026-02-23 22:37 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
On Mon, Feb 23, 2026 at 2:27 PM Bart Van Assche
<bart.vanassche@gmail.com> wrote:
> My understanding is that bnxt_dl_reload_up() is only called if
> bnxt_dl_reload_down() returns 0 (see also devlink_reload()).
> bnxt_dl_reload_up() returns an error code (-EOPNOTSUPP) for other
> actions than DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
> DEVLINK_RELOAD_ACTION_FW_ACTIVATE. Hence, bnxt_dl_reload_up() is only
> called if "action" is either DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
> DEVLINK_RELOAD_ACTION_FW_ACTIVATE. In other words, the code in the
> "default" clause will never be reached. So I think the above change is
> correct but also that the patch description should be modified to
> reflect that this is not a bug fix. Did I perhaps misunderstand
> something?
>
Yes, your description is correct. So perhaps a better cleanup would
be to remove the default case in bnxt_dl_reload_up()? Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
2026-02-23 22:37 ` Michael Chan
@ 2026-02-23 22:56 ` Bart Van Assche
2026-02-23 23:42 ` Michael Chan
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2026-02-23 22:56 UTC (permalink / raw)
To: Michael Chan
Cc: Peter Zijlstra, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
On 2/23/26 2:37 PM, Michael Chan wrote:
> On Mon, Feb 23, 2026 at 2:27 PM Bart Van Assche
> <bart.vanassche@gmail.com> wrote:
>
>> My understanding is that bnxt_dl_reload_up() is only called if
>> bnxt_dl_reload_down() returns 0 (see also devlink_reload()).
>> bnxt_dl_reload_up() returns an error code (-EOPNOTSUPP) for other
>> actions than DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
>> DEVLINK_RELOAD_ACTION_FW_ACTIVATE. Hence, bnxt_dl_reload_up() is only
>> called if "action" is either DEVLINK_RELOAD_ACTION_DRIVER_REINIT or
>> DEVLINK_RELOAD_ACTION_FW_ACTIVATE. In other words, the code in the
>> "default" clause will never be reached. So I think the above change is
>> correct but also that the patch description should be modified to
>> reflect that this is not a bug fix. Did I perhaps misunderstand
>> something?
>>
> Yes, your description is correct. So perhaps a better cleanup would
> be to remove the default case in bnxt_dl_reload_up()? Thanks.
Hmm ... wouldn't that trigger a -Wswitch warning, a warning that is
enabled by default for kernel code? From the gcc documentation:
"Warn whenever a switch statement has an index of enumerated type and
lacks a case for one or more of the named codes of that enumeration.
(The presence of a default label prevents this warning.) case labels
that do not correspond to enumerators also provoke warnings when this
option is used, unless the enumeration is marked with the flag_enum
attribute. This warning is enabled by -Wall."
Changing "default: return -EOPNOTSUPP;" into "default: BUG();" keeps
the Clang thread-sanitizer happy but does not comply with the kernel
coding style. Does the patch below look better?
Thanks,
Bart.
bnxt_en: Suppress thread-safety complaints about bnxt_dl_reload_up()
bnxt_dl_reload_down() returns an error code for actions that are not
supported. Hence, bnxt_dl_reload_up() won't be called for unsupported
actions. Since the compiler doesn't know this, add unlock calls to the
default clause. This patch suppresses a Clang thread-sanitizer
complaint. Compile-tested only.
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Shantiprasad Shettar <shantiprasad.shettar@broadcom.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: netdev@vger.kernel.org
Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 15de802bbac4..aa7ebd1426ed 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -562,6 +562,13 @@ static int bnxt_dl_reload_up(struct devlink *dl,
enum devlink_reload_action acti
break;
}
default:
+ /*
+ * Other actions have already been rejected by
+ * bnxt_dl_reload_down().
+ */
+ WARN_ON_ONCE(true);
+ netdev_unlock(bp->dev);
+ rtnl_unlock();
return -EOPNOTSUPP;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()
2026-02-23 22:56 ` Bart Van Assche
@ 2026-02-23 23:42 ` Michael Chan
0 siblings, 0 replies; 14+ messages in thread
From: Michael Chan @ 2026-02-23 23:42 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Jakub Kicinski, Shantiprasad Shettar,
Stanislav Fomichev, netdev
On Mon, Feb 23, 2026 at 2:56 PM Bart Van Assche
<bart.vanassche@gmail.com> wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 15de802bbac4..aa7ebd1426ed 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -562,6 +562,13 @@ static int bnxt_dl_reload_up(struct devlink *dl,
> enum devlink_reload_action acti
> break;
> }
> default:
> + /*
> + * Other actions have already been rejected by
> + * bnxt_dl_reload_down().
> + */
> + WARN_ON_ONCE(true);
> + netdev_unlock(bp->dev);
> + rtnl_unlock();
This is better. It makes it clear that it should never get here.
> return -EOPNOTSUPP;
> }
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-02-23 23:43 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260223214950.2153735-1-bvanassche@acm.org>
2026-02-23 21:49 ` [PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze Bart Van Assche
2026-02-23 22:23 ` Jakub Kicinski
2026-02-23 22:24 ` Jakub Kicinski
2026-02-23 22:30 ` Bart Van Assche
2026-02-23 21:49 ` [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up() Bart Van Assche
2026-02-23 22:17 ` Michael Chan
2026-02-23 22:27 ` Bart Van Assche
2026-02-23 22:37 ` Michael Chan
2026-02-23 22:56 ` Bart Van Assche
2026-02-23 23:42 ` Michael Chan
2026-02-23 21:49 ` [PATCH 20/62] octeontx2-pf: Fix locking in an error path Bart Van Assche
2026-02-23 21:49 ` [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze Bart Van Assche
2026-02-23 21:49 ` [PATCH 22/62] mctp i3c: Fix locking in error paths Bart Van Assche
[not found] <20260223220102.2158611-1-bart.vanassche@linux.dev>
2026-02-23 22:00 ` [PATCH 21/62] qed: Make _qed_mcp_cmd_and_union() easier to analyze Bart Van Assche
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox