[PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Bart Van Assche]

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;
 	}
 

[PATCH 17/62] bnxt_en: Make bnxt_resume() easier to analyze

[Bart Van Assche]

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);

[PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Bart Van Assche]

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;
 	}
 

[PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Bart Van Assche]

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

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;
 	}
 

Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Michael Chan]

.
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.

Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Bart Van Assche]

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.

Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Michael Chan]

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.

Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Bart Van Assche]

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;
  	}

Re: [PATCH 18/62] bnxt_en: Fix bnxt_dl_reload_up()

[Michael Chan]

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;
>         }
>
>