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

[Bart Van Assche <bart.vanassche@gmail.com>]

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.