public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
* [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 20/62] octeontx2-pf: Fix locking in an error path
       [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,
	Naveen Mamindlapalli, Jakub Kicinski, Sunil Goutham,
	Geetha sowjanya, Subbaraya Sundeep, hariprasad, Bharat Bhushan,
	netdev

From: Bart Van Assche <bvanassche@acm.org>

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

* 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 20/62] octeontx2-pf: Fix locking in an error path 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