* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-02 20:56 [PATCH net] net/mlx5: retain command mailboxes after timeout Manjunath Patil
@ 2026-10-02 20:58 ` netdev-bot+sinfo
2026-10-02 21:33 ` manjunath.b.patil
2026-10-03 20:56 ` sashiko-bot
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: netdev-bot+sinfo @ 2026-10-02 20:58 UTC (permalink / raw)
To: Manjunath Patil; +Cc: saeedm, leonro, tariqt, mbloch, netdev, linux-rdma
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-02 20:58 ` netdev-bot+sinfo
@ 2026-10-02 21:33 ` manjunath.b.patil
0 siblings, 0 replies; 8+ messages in thread
From: manjunath.b.patil @ 2026-10-02 21:33 UTC (permalink / raw)
To: netdev-bot+sinfo; +Cc: saeedm, leonro, tariqt, mbloch, netdev, linux-rdma
On 10/2/26 1:58 PM, netdev-bot+sinfo@kernel.org wrote:
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
> - How the issue was discovered, e.g. hit in production, hit during
> development, syzbot report, manual code inspection, LLM or static
> analysis tool scan.
>
> - Whether the issue was actually triggered, or is only theoretical
> (e.g. found by code inspection). If it was triggered please include
> the symptoms, like the stack trace or error messages.
>
> - What hardware the change was tested on. For driver fixes please
> mention the device (and if relevant firmware version) used for
> testing, or say that the change was not tested on real hardware.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.
Hi,
Thanks for the reminder. This was prompted by crashes on two production
systems with mlx5 command timeouts and device-health errors. One log
reported "Command completion arrived after timeout (entry idx = 2)"
shortly before an oops:
BUG: kernel NULL pointer dereference, address: 0000000000000100
RIP: dma_pool_alloc+0x3b/0x211
mlx5_alloc_cmd_msg -> cmd_exec -> mlx5e_update_stats_work
The vmcores showed the mlx5_cmd DMA pool's next_block pointer at 0x100.
Manual inspection of the command timeout path then identified the early
mailbox free addressed by this patch. The production failure is real,
but we have not proved that late firmware access caused that particular
pool corruption, nor reproduced the original panic.
I tested the fix on an OL9 x86_64 VM with two passthrough ConnectX-7
SR-IOV VFs, both reporting firmware 28.34.4000. Test-only fault
injection exercised normal, timeout-late, timeout-reset, timeout-lost,
and timeout/completion-overlap paths on both VFs. In the overlap test,
the blocking caller returned -ETIMEDOUT while completion was paused
before its final mailbox access; the mailboxes and slot remained live
until completion finished, then were released. There were no KASAN
reports or kmemleak findings. The overlap uses a synthetic sleepable
completion worker, not a physical firmware EQ interrupt.
Thanks,
Manjunath
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-02 20:56 [PATCH net] net/mlx5: retain command mailboxes after timeout Manjunath Patil
2026-10-02 20:58 ` netdev-bot+sinfo
@ 2026-10-03 20:56 ` sashiko-bot
2026-10-03 21:18 ` netdev-bot+sashiko
2026-10-06 13:01 ` Moshe Shemesh
3 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-10-03 20:56 UTC (permalink / raw)
To: Manjunath Patil; +Cc: linux-rdma
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mlx5: race condition causes uninitialized memory use on command timeout
--
commit 9bc189299b20224d8c729888220a0891ddf01904
Author: Manjunath Patil <manjunath.b.patil@oracle.com>
net/mlx5: retain command mailboxes after timeout
This commit fixes a DMA pool reallocation issue by giving the command entry
ownership of the mailboxes on timeout and freeing them when the final
reference is dropped. It also serializes completion claims and firmware
reference handoffs under alloc_lock.
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 84583dc5eb1c0..22508b26972de 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
[ ... ]
> @@ -1163,6 +1170,10 @@ static void wait_func_handle_exec_timeout(struct mlx5_core_dev *dev,
> mlx5_command_str(ent->op), ent->op);
>
> ent->ret = -ETIMEDOUT;
> + /* The real handler may have claimed the completion but still be using
> + * the mailboxes. Keep them with the entry until its last reference.
> + */
> + set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
> mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
> }
[Severity: High]
Does setting the timeout error and retain flag without locks here race with
the real completion handler?
If a blocking command times out just as the hardware completes it, this can
set ent->ret = -ETIMEDOUT while the EQ handler in mlx5_cmd_comp_handler()
is simultaneously checking or updating it.
[ ... ]
> @@ -1744,38 +1816,33 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
> struct mlx5_cmd_stats *stats;
> unsigned long flags;
> unsigned long vector;
> + bool pending;
> + bool drop_fw_ref;
>
> /* there can be at most 32 command queues */
> vector = vec & 0xffffffff;
> for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
> if (test_bit(i, &vector)) {
> ent = cmd->ent_arr[i];
> -
> - if (forced && ent->ret == -ETIMEDOUT)
> - set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> - &ent->state);
> - else if (!forced) /* real FW completion */
> - clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> - &ent->state);
> + pending = mlx5_cmd_claim_completion(dev, ent, vec, forced,
> + &drop_fw_ref);
[Severity: High]
Can this overwrite the concurrently set -ETIMEDOUT with 0 on success?
If the timeout from wait_func_handle_exec_timeout() occurs concurrently with
this completion handler, the following code later in mlx5_cmd_comp_handler()
can overwrite the error:
if (!ent->ret) {
if (!cmd->checksum_disabled)
ent->ret = verify_signature(ent);
If the timeout occurs between the if (!ent->ret) check and the
ent->ret = verify_signature(ent) assignment, it appears this would hide the
timeout from the waiting thread.
[ ... ]
> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
> }
>
> err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
> - pages_queue, token, force_polling);
> + pages_queue, token, force_polling, &retain_msgs);
> if (callback && !err)
> return 0;
> + /* The entry releases retained DMA mailboxes with its final reference. */
> + if (retain_msgs)
> + goto out_up;
>
> if (err > 0) /* Failed in FW, command didn't execute */
> err = deliv_status_to_err(err);
[Severity: High]
Does this cause uninitialized memory to be used if the race described above
occurs?
If retain_msgs is true but err was overwritten to 0 by the completion handler,
this skips copying the output buffer and returns success (0).
Would this leave the caller with uninitialized memory in the output buffer,
potentially leading to data corruption or crashes when the caller assumes it
contains valid firmware responses?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261002205649.2029588-1-manjunath.b.patil@oracle.com?part=1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-02 20:56 [PATCH net] net/mlx5: retain command mailboxes after timeout Manjunath Patil
2026-10-02 20:58 ` netdev-bot+sinfo
2026-10-03 20:56 ` sashiko-bot
@ 2026-10-03 21:18 ` netdev-bot+sashiko
2026-10-05 17:23 ` manjunath.b.patil
2026-10-06 13:01 ` Moshe Shemesh
3 siblings, 1 reply; 8+ messages in thread
From: netdev-bot+sashiko @ 2026-10-03 21:18 UTC (permalink / raw)
To: manjunath.b.patil
Cc: saeedm, leonro, tariqt, mbloch, netdev, linux-rdma, kuba
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Retained mailboxes are freed after cmd_ent_put() has already
released the slot.
- [High] A reset flush can now consume the retained FW reference while a
software EQ-recovery pass is processing the late real completion for…
- [Medium] Mailboxes now owned by the entry are never released on normal
command-interface teardown, so dma_pool_destroy() finds the pool busy…
--
net/mlx5: retain command mailboxes after timeout
After a command timeout, this patch gives the command entry ownership of
its input and output mailboxes, and cmd_free_ent() frees them on the final
reference. A new helper, mlx5_cmd_claim_completion(), takes the
PENDING_COMP claim and hands off the firmware reference under alloc_lock.
Triggered (reset) completions now drop a reference that an earlier timeout
kept.
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 84583dc5eb1c0..22508b26972de 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -142,8 +142,19 @@ cmd_alloc_ent(struct mlx5_cmd *cmd, struct mlx5_cmd_msg *in,
> return ent;
> }
>
> +static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
> +static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
> + struct mlx5_cmd_msg *msg);
> +
> static void cmd_free_ent(struct mlx5_cmd_work_ent *ent)
> {
> + if (test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state)) {
> + struct mlx5_core_dev *dev = container_of(ent->cmd,
> + struct mlx5_core_dev, cmd);
> +
> + mlx5_free_cmd_msg(dev, ent->out);
> + free_msg(dev, ent->in);
> + }
> kfree(ent);
> }
[Severity: High]
Can these retained mailboxes be freed after the command DMA pool has
already been destroyed?
cmd_ent_put() releases the slot and the semaphore under alloc_lock. It
then drops the lock and only after that calls cmd_free_ent():
cmd_ent_put() {
...
if (ent->idx >= 0) {
cmd_free_index(cmd, ent->idx);
up(ent->page_queue ? &cmd->vars.pages_sem : &cmd->vars.sem);
}
spin_unlock_irqrestore(&cmd->alloc_lock, flags);
cmd_free_ent(ent);
}
With this patch, cmd_free_ent() calls dma_pool_free() on cmd->pool
through mlx5_free_cmd_msg(). free_msg() can also list_add the input msg
back onto msg->parent->head.
For an async command, cb_timeout_handler() runs on system_wq and holds its
own reference. Its forced mlx5_cmd_comp_handler() call sets RETAIN_MSGS and
TIMEDOUT and runs the user callback, so mlx5_async_ctx teardown stops
waiting. Only after that does it call cmd_ent_put().
The firmware reference can be dropped first, either by a late real
completion or by mlx5_cmd_trigger_completions(), which drops both the
firmware reference and its own temporary one. In that case the put in
cb_timeout_handler() is the final one.
Once that put calls up(), the down_trylock() loop in mlx5_cmd_flush() can
succeed. Error recovery or unload can then continue into
mlx5_cmd_disable():
flush_workqueue(cmd->wq);
clean_debug_files(dev);
destroy_msg_cache(dev);
free_cmd_page(dev, cmd);
dma_pool_destroy(cmd->pool);
This flushes only cmd->wq. It does not flush system_wq or the
cb_timeout_work items.
Suppose the timeout worker is preempted after it drops alloc_lock and
resumes after this point. Would cmd_free_ent() then call dma_pool_free() on
a freed struct dma_pool, or add the input msg to a destroyed cache list?
Before this patch, this window only covered kfree(ent), which never
touched the pool.
[ ... ]
> @@ -1732,6 +1744,66 @@ static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg)
> }
> }
>
> +/*
[ ... ]
> + * A blocking timeout retains the mailboxes even when a real completion has
> + * claimed PENDING_COMP. That handler keeps its firmware-event reference until
> + * it finishes using the mailboxes. Keeping the reference transitions under
> + * alloc_lock prevents timeout, firmware, and reset from consuming the same
> + * reference.
> + */
> +static bool mlx5_cmd_claim_completion(struct mlx5_core_dev *dev,
> + struct mlx5_cmd_work_ent *ent, u64 vec,
> + bool forced, bool *drop_fw_ref)
> +{
[ ... ]
> + } else if (!forced) {
> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
> + *drop_fw_ref = true;
> + } else if (vec & MLX5_TRIGGERED_CMD_COMP) {
> + /* Reset cannot receive a late firmware completion. */
> + *drop_fw_ref = test_and_clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> + &ent->state);
> + }
[Severity: High]
Can a reset flush and a late real completion both drop the same retained
firmware reference here?
Start with entry A, which has already timed out (PENDING_COMP clear,
TIMEDOUT set). Only its firmware reference keeps it alive.
Thread R: another command's timeout calls mlx5_cmd_eq_recover(). That runs
mlx5_eq_async_int(ASYNC_EQ_RECOVER) in process context. cmd_comp_notifier()
passes its INTERNAL_ERROR check, and mlx5_cmd_comp_handler(dev, vec, false)
reads ent = cmd->ent_arr[i] without taking a reference.
Thread F: enter_error_state() sets INTERNAL_ERROR and calls
mlx5_cmd_flush()->mlx5_cmd_trigger_completions().
mlx5_eq_synchronize_cmd_irq() does not wait for R, because R is not running
in IRQ context.
The sequence would be:
F: cmd_ent_get(cmd->ent_arr[i]) /* temporary ref */
F: mlx5_cmd_claim_completion()
vec & MLX5_TRIGGERED_CMD_COMP
test_and_clear_bit(TIMEDOUT) -> true
cmd_ent_put(ent) /* drops fw ref */
R: mlx5_cmd_claim_completion()
!forced
*drop_fw_ref = true /* unconditional */
cmd_ent_put(ent) /* drops F's temp ref */
-> refcount 0, ent_arr[i] = NULL, cmd_free_ent()
F: cmd_ent_put(cmd->ent_arr[i]) /* NULL or a new entry */
Would the trailing put in mlx5_cmd_trigger_completions() then dereference
NULL? Or would it drop a reference on an unrelated entry that has reused
slot i?
If F finishes completely first, R's claim would touch the freed entry
instead.
The comment above says:
Keeping the reference transitions under alloc_lock prevents timeout,
firmware, and reset from consuming the same reference.
The !forced branch does not check whether TIMEDOUT is still set, so does
the helper actually enforce this?
In the baseline, the triggered path never dropped the reference of an
entry with TIMEDOUT set, so this sequence stayed balanced.
[ ... ]
> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
> }
>
> err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
> - pages_queue, token, force_polling);
> + pages_queue, token, force_polling, &retain_msgs);
> if (callback && !err)
> return 0;
> + /* The entry releases retained DMA mailboxes with its final reference. */
> + if (retain_msgs)
> + goto out_up;
[Severity: Medium]
What releases these retained mailboxes on a normal command interface
teardown?
Take a command that times out while the interface is up and its opcode is
allowed. mlx5_cmd_claim_completion() sets RETAIN_MSGS and TIMEDOUT and
keeps the firmware reference. cmd_exec() now skips freeing inb and outb.
Only two things drop that reference: a later real completion, or a
MLX5_TRIGGERED_CMD_COMP flush. The flush only runs from
enter_error_state().
mlx5_function_disable() and the err_cmd_cleanup path in
mlx5_function_enable() call mlx5_cmd_disable() without either:
err_cmd_cleanup:
mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
mlx5_cmd_disable(dev);
mlx5_cmd_disable() then runs destroy_msg_cache() and
dma_pool_destroy(cmd->pool) while the retained blocks are still allocated.
dma_pool_destroy() sees nr_active != 0, prints "dma_pool_destroy mlx5_cmd
busy", and skips dma_free_coherent() for every page.
Does this leak all of the pool's coherent pages, including the ones backing
the whole message cache?
A retained cached input msg is also not on ch->head, so destroy_msg_cache()
never frees it.
Polling mode is active during mlx5_function_enable(), and it seems to reach
this easily:
cmd_work_handler() {
...
if (cmd_mode == CMD_MODE_POLLING || poll_cmd) {
poll_timeout(ent);
/* make sure we read the descriptor after ownership is SW */
rmb();
mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, !!ent->ret);
}
}
There is no command EQ yet, so no late completion can arrive. If
MANAGE_PAGES, SET_ISSI or QUERY_ISSI times out and mlx5_function_enable()
then fails, the pool leaks.
Before this patch, cmd_exec() freed the mailboxes at timeout, so only the
ent struct and its slot leaked.
A less certain follow-on: if a triggered flush drops the retained
reference after mlx5_cmd_disable() has run, would cmd_free_ent() call
dma_pool_free() on the destroyed pool? cmd->pool is not cleared.
>
> if (err > 0) /* Failed in FW, command didn't execute */
> err = deliv_status_to_err(err);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002205649.2029588-1-manjunath.b.patil%40oracle.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-03 21:18 ` netdev-bot+sashiko
@ 2026-10-05 17:23 ` manjunath.b.patil
0 siblings, 0 replies; 8+ messages in thread
From: manjunath.b.patil @ 2026-10-05 17:23 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: saeedm, leonro, tariqt, mbloch, netdev, linux-rdma, kuba
On 10/3/26 2:18 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 1 · Low: 0
Hi,
Thank you for the review. We have examined the three reported issues and
will address the command-entry teardown ordering, the race between
software EQ recovery and reset-triggered completion, and retained
mailbox ownership during polling-mode teardown. We will validate those
changes and return with a v2 patch.
Thanks,
Manjunath
pw-bot: cr
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] Retained mailboxes are freed after cmd_ent_put() has already
> released the slot.
> - [High] A reset flush can now consume the retained FW reference while a
> software EQ-recovery pass is processing the late real completion for…
> - [Medium] Mailboxes now owned by the entry are never released on normal
> command-interface teardown, so dma_pool_destroy() finds the pool busy…
>
> --
>
> net/mlx5: retain command mailboxes after timeout
>
> After a command timeout, this patch gives the command entry ownership of
> its input and output mailboxes, and cmd_free_ent() frees them on the final
> reference. A new helper, mlx5_cmd_claim_completion(), takes the
> PENDING_COMP claim and hands off the firmware reference under alloc_lock.
> Triggered (reset) completions now drop a reference that an earlier timeout
> kept.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> index 84583dc5eb1c0..22508b26972de 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> @@ -142,8 +142,19 @@ cmd_alloc_ent(struct mlx5_cmd *cmd, struct mlx5_cmd_msg *in,
>> return ent;
>> }
>>
>> +static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
>> +static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
>> + struct mlx5_cmd_msg *msg);
>> +
>> static void cmd_free_ent(struct mlx5_cmd_work_ent *ent)
>> {
>> + if (test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state)) {
>> + struct mlx5_core_dev *dev = container_of(ent->cmd,
>> + struct mlx5_core_dev, cmd);
>> +
>> + mlx5_free_cmd_msg(dev, ent->out);
>> + free_msg(dev, ent->in);
>> + }
>> kfree(ent);
>> }
>
> [Severity: High]
> Can these retained mailboxes be freed after the command DMA pool has
> already been destroyed?
>
> cmd_ent_put() releases the slot and the semaphore under alloc_lock. It
> then drops the lock and only after that calls cmd_free_ent():
>
> cmd_ent_put() {
> ...
> if (ent->idx >= 0) {
> cmd_free_index(cmd, ent->idx);
> up(ent->page_queue ? &cmd->vars.pages_sem : &cmd->vars.sem);
> }
> spin_unlock_irqrestore(&cmd->alloc_lock, flags);
>
> cmd_free_ent(ent);
> }
>
> With this patch, cmd_free_ent() calls dma_pool_free() on cmd->pool
> through mlx5_free_cmd_msg(). free_msg() can also list_add the input msg
> back onto msg->parent->head.
>
> For an async command, cb_timeout_handler() runs on system_wq and holds its
> own reference. Its forced mlx5_cmd_comp_handler() call sets RETAIN_MSGS and
> TIMEDOUT and runs the user callback, so mlx5_async_ctx teardown stops
> waiting. Only after that does it call cmd_ent_put().
>
> The firmware reference can be dropped first, either by a late real
> completion or by mlx5_cmd_trigger_completions(), which drops both the
> firmware reference and its own temporary one. In that case the put in
> cb_timeout_handler() is the final one.
>
> Once that put calls up(), the down_trylock() loop in mlx5_cmd_flush() can
> succeed. Error recovery or unload can then continue into
> mlx5_cmd_disable():
>
> flush_workqueue(cmd->wq);
> clean_debug_files(dev);
> destroy_msg_cache(dev);
> free_cmd_page(dev, cmd);
> dma_pool_destroy(cmd->pool);
>
> This flushes only cmd->wq. It does not flush system_wq or the
> cb_timeout_work items.
>
> Suppose the timeout worker is preempted after it drops alloc_lock and
> resumes after this point. Would cmd_free_ent() then call dma_pool_free() on
> a freed struct dma_pool, or add the input msg to a destroyed cache list?
>
> Before this patch, this window only covered kfree(ent), which never
> touched the pool.
>
> [ ... ]
>
>> @@ -1732,6 +1744,66 @@ static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg)
>> }
>> }
>>
>> +/*
>
> [ ... ]
>
>> + * A blocking timeout retains the mailboxes even when a real completion has
>> + * claimed PENDING_COMP. That handler keeps its firmware-event reference until
>> + * it finishes using the mailboxes. Keeping the reference transitions under
>> + * alloc_lock prevents timeout, firmware, and reset from consuming the same
>> + * reference.
>> + */
>> +static bool mlx5_cmd_claim_completion(struct mlx5_core_dev *dev,
>> + struct mlx5_cmd_work_ent *ent, u64 vec,
>> + bool forced, bool *drop_fw_ref)
>> +{
>
> [ ... ]
>
>> + } else if (!forced) {
>> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
>> + *drop_fw_ref = true;
>> + } else if (vec & MLX5_TRIGGERED_CMD_COMP) {
>> + /* Reset cannot receive a late firmware completion. */
>> + *drop_fw_ref = test_and_clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
>> + &ent->state);
>> + }
>
> [Severity: High]
> Can a reset flush and a late real completion both drop the same retained
> firmware reference here?
>
> Start with entry A, which has already timed out (PENDING_COMP clear,
> TIMEDOUT set). Only its firmware reference keeps it alive.
>
> Thread R: another command's timeout calls mlx5_cmd_eq_recover(). That runs
> mlx5_eq_async_int(ASYNC_EQ_RECOVER) in process context. cmd_comp_notifier()
> passes its INTERNAL_ERROR check, and mlx5_cmd_comp_handler(dev, vec, false)
> reads ent = cmd->ent_arr[i] without taking a reference.
>
> Thread F: enter_error_state() sets INTERNAL_ERROR and calls
> mlx5_cmd_flush()->mlx5_cmd_trigger_completions().
> mlx5_eq_synchronize_cmd_irq() does not wait for R, because R is not running
> in IRQ context.
>
> The sequence would be:
>
> F: cmd_ent_get(cmd->ent_arr[i]) /* temporary ref */
> F: mlx5_cmd_claim_completion()
> vec & MLX5_TRIGGERED_CMD_COMP
> test_and_clear_bit(TIMEDOUT) -> true
> cmd_ent_put(ent) /* drops fw ref */
> R: mlx5_cmd_claim_completion()
> !forced
> *drop_fw_ref = true /* unconditional */
> cmd_ent_put(ent) /* drops F's temp ref */
> -> refcount 0, ent_arr[i] = NULL, cmd_free_ent()
> F: cmd_ent_put(cmd->ent_arr[i]) /* NULL or a new entry */
>
> Would the trailing put in mlx5_cmd_trigger_completions() then dereference
> NULL? Or would it drop a reference on an unrelated entry that has reused
> slot i?
>
> If F finishes completely first, R's claim would touch the freed entry
> instead.
>
> The comment above says:
>
> Keeping the reference transitions under alloc_lock prevents timeout,
> firmware, and reset from consuming the same reference.
>
> The !forced branch does not check whether TIMEDOUT is still set, so does
> the helper actually enforce this?
>
> In the baseline, the triggered path never dropped the reference of an
> entry with TIMEDOUT set, so this sequence stayed balanced.
>
> [ ... ]
>
>> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>> }
>>
>> err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
>> - pages_queue, token, force_polling);
>> + pages_queue, token, force_polling, &retain_msgs);
>> if (callback && !err)
>> return 0;
>> + /* The entry releases retained DMA mailboxes with its final reference. */
>> + if (retain_msgs)
>> + goto out_up;
>
> [Severity: Medium]
> What releases these retained mailboxes on a normal command interface
> teardown?
>
> Take a command that times out while the interface is up and its opcode is
> allowed. mlx5_cmd_claim_completion() sets RETAIN_MSGS and TIMEDOUT and
> keeps the firmware reference. cmd_exec() now skips freeing inb and outb.
>
> Only two things drop that reference: a later real completion, or a
> MLX5_TRIGGERED_CMD_COMP flush. The flush only runs from
> enter_error_state().
>
> mlx5_function_disable() and the err_cmd_cleanup path in
> mlx5_function_enable() call mlx5_cmd_disable() without either:
>
> err_cmd_cleanup:
> mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
> mlx5_cmd_disable(dev);
>
> mlx5_cmd_disable() then runs destroy_msg_cache() and
> dma_pool_destroy(cmd->pool) while the retained blocks are still allocated.
> dma_pool_destroy() sees nr_active != 0, prints "dma_pool_destroy mlx5_cmd
> busy", and skips dma_free_coherent() for every page.
>
> Does this leak all of the pool's coherent pages, including the ones backing
> the whole message cache?
>
> A retained cached input msg is also not on ch->head, so destroy_msg_cache()
> never frees it.
>
> Polling mode is active during mlx5_function_enable(), and it seems to reach
> this easily:
>
> cmd_work_handler() {
> ...
> if (cmd_mode == CMD_MODE_POLLING || poll_cmd) {
> poll_timeout(ent);
> /* make sure we read the descriptor after ownership is SW */
> rmb();
> mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, !!ent->ret);
> }
> }
>
> There is no command EQ yet, so no late completion can arrive. If
> MANAGE_PAGES, SET_ISSI or QUERY_ISSI times out and mlx5_function_enable()
> then fails, the pool leaks.
>
> Before this patch, cmd_exec() freed the mailboxes at timeout, so only the
> ent struct and its slot leaked.
>
> A less certain follow-on: if a triggered flush drops the retained
> reference after mlx5_cmd_disable() has run, would cmd_free_ent() call
> dma_pool_free() on the destroyed pool? cmd->pool is not cleared.
>
>>
>> if (err > 0) /* Failed in FW, command didn't execute */
>> err = deliv_status_to_err(err);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-02 20:56 [PATCH net] net/mlx5: retain command mailboxes after timeout Manjunath Patil
` (2 preceding siblings ...)
2026-10-03 21:18 ` netdev-bot+sashiko
@ 2026-10-06 13:01 ` Moshe Shemesh
2026-10-06 19:42 ` manjunath.b.patil
3 siblings, 1 reply; 8+ messages in thread
From: Moshe Shemesh @ 2026-10-06 13:01 UTC (permalink / raw)
To: Manjunath Patil, saeedm; +Cc: leonro, tariqt, mbloch, netdev, linux-rdma
On 10/2/2026 11:56 PM, Manjunath Patil wrote:
> Firmware can access command DMA mailboxes after the driver times out.
> The command entry already keeps a reference for a possible firmware
> completion, which keeps its slot allocated, but the blocking caller
> and async callback can still free their input and output mailboxes on
> timeout. A late firmware access can therefore hit reallocated DMA
> pool memory.
>
> Give the entry ownership of the mailboxes on timeout and free them when
> its final reference is dropped. If timeout claims PENDING_COMP first,
> retain the firmware-event reference while a real completion remains
> possible; a late completion drops it.
>
> The real EQ handler may instead claim PENDING_COMP before the blocking
> timeout handler runs and still be reading the mailboxes. Set
> RETAIN_MSGS before forcing timeout completion in the blocking path.
> The EQ handler holds its entry reference through its last mailbox
> access and done notification, so the caller can return on timeout
> without freeing memory beneath it.
>
> Serialize completion claims and firmware-reference handoffs under
> alloc_lock. The existing reset/error flush marks synthetic
> completions with MLX5_TRIGGERED_CMD_COMP and treats them as terminal
> for firmware ownership; only such a triggered completion drops a
> reference retained by an earlier timeout. Normal command EQ teardown
> and its slot-drain policy are unchanged.
>
> Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters")
> Assisted-by: Codex:gpt-5
> Signed-off-by: Manjunath Patil <manjunath.b.patil@oracle.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 127 ++++++++++++++----
> include/linux/mlx5/driver.h | 1 +
> 2 files changed, 103 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 84583dc5eb1c..22508b26972d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -142,8 +142,19 @@ cmd_alloc_ent(struct mlx5_cmd *cmd, struct mlx5_cmd_msg *in,
> return ent;
> }
>
Hi Manjunath, thanks for your patch.
Danielle Costantino has sent recently a patch for the same bug.
See v2 on
https://lore.kernel.org/netdev/20260923190542.848049-1-dcostantino@meta.com/
Please follow on that thread.
Thanks,
Moshe.
> +static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
> +static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
> + struct mlx5_cmd_msg *msg);
> +
> static void cmd_free_ent(struct mlx5_cmd_work_ent *ent)
> {
> + if (test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state)) {
> + struct mlx5_core_dev *dev = container_of(ent->cmd,
> + struct mlx5_core_dev, cmd);
> +
> + mlx5_free_cmd_msg(dev, ent->out);
> + free_msg(dev, ent->in);
> + }
> kfree(ent);
> }
>
> @@ -958,10 +969,6 @@ static void cb_timeout_handler(struct work_struct *work)
> cmd_ent_put(ent); /* for the cmd_ent_get() took on schedule delayed work */
> }
>
> -static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
> -static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
> - struct mlx5_cmd_msg *msg);
> -
> static bool opcode_allowed(struct mlx5_cmd *cmd, u16 opcode)
> {
> if (cmd->allowed_opcode == CMD_ALLOWED_OPCODE_ALL)
> @@ -1163,6 +1170,10 @@ static void wait_func_handle_exec_timeout(struct mlx5_core_dev *dev,
> mlx5_command_str(ent->op), ent->op);
>
> ent->ret = -ETIMEDOUT;
> + /* The real handler may have claimed the completion but still be using
> + * the mailboxes. Keep them with the entry until its last reference.
> + */
> + set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
> mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
> }
>
> @@ -1260,7 +1271,7 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
> struct mlx5_cmd_msg *out, void *uout, int uout_size,
> mlx5_cmd_cbk_t callback,
> void *context, int page_queue,
> - u8 token, bool force_polling)
> + u8 token, bool force_polling, bool *retain_msgs)
> {
> struct mlx5_cmd *cmd = &dev->cmd;
> struct mlx5_cmd_work_ent *ent;
> @@ -1313,6 +1324,7 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
> return 0; /* mlx5_cmd_comp_handler() will put(ent) */
>
> err = wait_func(dev, ent);
> + *retain_msgs = test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
> if (err == -ETIMEDOUT || err == -ECANCELED || err == -EBUSY)
> goto out_free;
>
> @@ -1732,6 +1744,66 @@ static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg)
> }
> }
>
> +/*
> + * cmd_work_handler() takes an entry reference for the firmware event and
> + * sets PENDING_COMP before ringing the command doorbell. Firmware can still
> + * use the input and output DMA mailboxes after the caller times out.
> + *
> + * Claim PENDING_COMP under alloc_lock so exactly one handler makes the
> + * mailbox and firmware-reference decisions:
> + *
> + * - A timeout that wins retains the mailboxes in the entry. If the command
> + * interface is still up and the opcode is allowed, firmware can still
> + * generate a real completion, so TIMEDOUT retains the firmware-event ref.
> + * - A real completion that wins consumes its firmware-event ref normally.
> + * - A real completion that loses is the late event after a timeout. It drops
> + * the ref retained by TIMEDOUT; RETAIN_MSGS stays set until entry teardown.
> + * - A reset completion cannot be followed by a real firmware event. It drops
> + * a ref only when TIMEDOUT says the timeout retained one.
> + *
> + * A blocking timeout retains the mailboxes even when a real completion has
> + * claimed PENDING_COMP. That handler keeps its firmware-event reference until
> + * it finishes using the mailboxes. Keeping the reference transitions under
> + * alloc_lock prevents timeout, firmware, and reset from consuming the same
> + * reference.
> + */
> +static bool mlx5_cmd_claim_completion(struct mlx5_core_dev *dev,
> + struct mlx5_cmd_work_ent *ent, u64 vec,
> + bool forced, bool *drop_fw_ref)
> +{
> + struct mlx5_cmd *cmd = &dev->cmd;
> + unsigned long flags;
> + bool timed_out = forced && ent->ret == -ETIMEDOUT;
> + bool pending;
> +
> + *drop_fw_ref = false;
> + spin_lock_irqsave(&cmd->alloc_lock, flags);
> + pending = test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP,
> + &ent->state);
> + if (pending) {
> + if (timed_out)
> + set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
> +
> + if (timed_out && !mlx5_cmd_is_down(dev) &&
> + opcode_allowed(cmd, ent->op)) {
> + set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
> + } else {
> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
> + *drop_fw_ref = true;
> + }
> + } else if (!forced) {
> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
> + *drop_fw_ref = true;
> + } else if (vec & MLX5_TRIGGERED_CMD_COMP) {
> + /* Reset cannot receive a late firmware completion. */
> + *drop_fw_ref = test_and_clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> + &ent->state);
> + }
> + spin_unlock_irqrestore(&cmd->alloc_lock, flags);
> +
> + return pending;
> +}
> +
> static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool forced)
> {
> struct mlx5_cmd *cmd = &dev->cmd;
> @@ -1744,38 +1816,33 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
> struct mlx5_cmd_stats *stats;
> unsigned long flags;
> unsigned long vector;
> + bool pending;
> + bool drop_fw_ref;
>
> /* there can be at most 32 command queues */
> vector = vec & 0xffffffff;
> for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
> if (test_bit(i, &vector)) {
> ent = cmd->ent_arr[i];
> -
> - if (forced && ent->ret == -ETIMEDOUT)
> - set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> - &ent->state);
> - else if (!forced) /* real FW completion */
> - clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> - &ent->state);
> + pending = mlx5_cmd_claim_completion(dev, ent, vec, forced,
> + &drop_fw_ref);
>
> /* if we already completed the command, ignore it */
> - if (!test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP,
> - &ent->state)) {
> - /* only real completion can free the cmd slot */
> + if (!pending) {
> if (!forced) {
> - mlx5_core_err(dev, "Command completion arrived after timeout (entry idx = %d).\n",
> + mlx5_core_err(dev,
> + "Command completion arrived after timeout (entry idx = %d).\n",
> ent->idx);
> - cmd_ent_put(ent);
> }
> + if (drop_fw_ref)
> + cmd_ent_put(ent);
> continue;
> }
>
> if (ent->callback && cancel_delayed_work(&ent->cb_timeout_work))
> cmd_ent_put(ent); /* timeout work was canceled */
>
> - if (!forced || /* Real FW completion */
> - mlx5_cmd_is_down(dev) || /* No real FW completion is expected */
> - !opcode_allowed(cmd, ent->op))
> + if (drop_fw_ref && ent->callback)
> cmd_ent_put(ent);
>
> ent->ts2 = ktime_get_ns();
> @@ -1816,17 +1883,23 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
> ent->out,
> ent->uout_size);
>
> - mlx5_free_cmd_msg(dev, ent->out);
> - free_msg(dev, ent->in);
> + if (!test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS,
> + &ent->state)) {
> + mlx5_free_cmd_msg(dev, ent->out);
> + free_msg(dev, ent->in);
> + }
>
> /* final consumer is done, release ent */
> cmd_ent_put(ent);
> callback(err, context);
> } else {
> - /* release wait_func() so mlx5_cmd_invoke()
> - * can make the final ent_put()
> + /* No mailbox accesses after done. If the caller timed out,
> + * the firmware reference keeps ent and its mailboxes alive
> + * until this handler has finished.
> */
> complete(&ent->done);
> + if (drop_fw_ref)
> + cmd_ent_put(ent);
> }
> }
> }
> @@ -1965,6 +2038,7 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
> gfp_t gfp;
> u8 token;
> int err;
> + bool retain_msgs = false;
>
> if (mlx5_cmd_is_down(dev) || !opcode_allowed(&dev->cmd, opcode))
> return -ENXIO;
> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
> }
>
> err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
> - pages_queue, token, force_polling);
> + pages_queue, token, force_polling, &retain_msgs);
> if (callback && !err)
> return 0;
> + /* The entry releases retained DMA mailboxes with its final reference. */
> + if (retain_msgs)
> + goto out_up;
>
> if (err > 0) /* Failed in FW, command didn't execute */
> err = deliv_status_to_err(err);
> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> index 83d0a83bbfbc..99cabe13e6c1 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -829,6 +829,7 @@ typedef void (*mlx5_cmd_cbk_t)(int status, void *context);
> enum {
> MLX5_CMD_ENT_STATE_PENDING_COMP,
> MLX5_CMD_ENT_STATE_TIMEDOUT,
> + MLX5_CMD_ENT_STATE_RETAIN_MSGS,
> };
>
> struct mlx5_cmd_work_ent {
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net/mlx5: retain command mailboxes after timeout
2026-10-06 13:01 ` Moshe Shemesh
@ 2026-10-06 19:42 ` manjunath.b.patil
0 siblings, 0 replies; 8+ messages in thread
From: manjunath.b.patil @ 2026-10-06 19:42 UTC (permalink / raw)
To: Moshe Shemesh, saeedm; +Cc: leonro, tariqt, mbloch, netdev, linux-rdma
On 10/6/26 6:01 AM, Moshe Shemesh wrote:
> On 10/2/2026 11: 56 PM, Manjunath Patil wrote: > Firmware can access
> command DMA mailboxes after the driver times out. > The command entry
> already keeps a reference for a possible firmware > completion, which
> keeps its slot allocated,
>
>
> On 10/2/2026 11:56 PM, Manjunath Patil wrote:
>> Firmware can access command DMA mailboxes after the driver times out.
>> The command entry already keeps a reference for a possible firmware
>> completion, which keeps its slot allocated, but the blocking caller
>> and async callback can still free their input and output mailboxes on
>> timeout. A late firmware access can therefore hit reallocated DMA
>> pool memory.
>>
>> Give the entry ownership of the mailboxes on timeout and free them when
>> its final reference is dropped. If timeout claims PENDING_COMP first,
>> retain the firmware-event reference while a real completion remains
>> possible; a late completion drops it.
>>
>> The real EQ handler may instead claim PENDING_COMP before the blocking
>> timeout handler runs and still be reading the mailboxes. Set
>> RETAIN_MSGS before forcing timeout completion in the blocking path.
>> The EQ handler holds its entry reference through its last mailbox
>> access and done notification, so the caller can return on timeout
>> without freeing memory beneath it.
>>
>> Serialize completion claims and firmware-reference handoffs under
>> alloc_lock. The existing reset/error flush marks synthetic
>> completions with MLX5_TRIGGERED_CMD_COMP and treats them as terminal
>> for firmware ownership; only such a triggered completion drops a
>> reference retained by an earlier timeout. Normal command EQ teardown
>> and its slot-drain policy are unchanged.
>>
>> Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters")
>> Assisted-by: Codex:gpt-5
>> Signed-off-by: Manjunath Patil <manjunath.b.patil@oracle.com>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 127 ++++++++++++++----
>> include/linux/mlx5/driver.h | 1 +
>> 2 files changed, 103 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> index 84583dc5eb1c..22508b26972d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> @@ -142,8 +142,19 @@ cmd_alloc_ent(struct mlx5_cmd *cmd, struct mlx5_cmd_msg *in,
>> return ent;
>> }
>>
>
> Hi Manjunath, thanks for your patch.
> Danielle Costantino has sent recently a patch for the same bug.
> See v2 on
> https://urldefense.com/v3/__https://lore.kernel.org/
> netdev/20260923190542.848049-1-dcostantino@meta.com/__;!!
> ACWV5N9M2RV99hQ!LQVo_l7643-
> HwAmD_d3InRHBlzl8puobw7CVp0ZFW54BELP7w6vbSl8fhcrFiNZFNC5pLUsSffv1l2oebXq9$ <https://urldefense.com/v3/__https://lore.kernel.org/netdev/20260923190542.848049-1-dcostantino@meta.com/__;!!ACWV5N9M2RV99hQ!LQVo_l7643-HwAmD_d3InRHBlzl8puobw7CVp0ZFW54BELP7w6vbSl8fhcrFiNZFNC5pLUsSffv1l2oebXq9$>
> Please follow on that thread.
Thank you for pointing me to Danielle's v2. I'll hold off on sending a
v2 of my patch and follow Danielle's series through review.
-Manjunath>
> Thanks,
> Moshe.
>
>> +static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
>> +static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
>> + struct mlx5_cmd_msg *msg);
>> +
>> static void cmd_free_ent(struct mlx5_cmd_work_ent *ent)
>> {
>> + if (test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state)) {
>> + struct mlx5_core_dev *dev = container_of(ent->cmd,
>> + struct mlx5_core_dev, cmd);
>> +
>> + mlx5_free_cmd_msg(dev, ent->out);
>> + free_msg(dev, ent->in);
>> + }
>> kfree(ent);
>> }
>>
>> @@ -958,10 +969,6 @@ static void cb_timeout_handler(struct work_struct *work)
>> cmd_ent_put(ent); /* for the cmd_ent_get() took on schedule delayed work */
>> }
>>
>> -static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
>> -static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
>> - struct mlx5_cmd_msg *msg);
>> -
>> static bool opcode_allowed(struct mlx5_cmd *cmd, u16 opcode)
>> {
>> if (cmd->allowed_opcode == CMD_ALLOWED_OPCODE_ALL)
>> @@ -1163,6 +1170,10 @@ static void wait_func_handle_exec_timeout(struct mlx5_core_dev *dev,
>> mlx5_command_str(ent->op), ent->op);
>>
>> ent->ret = -ETIMEDOUT;
>> + /* The real handler may have claimed the completion but still be using
>> + * the mailboxes. Keep them with the entry until its last reference.
>> + */
>> + set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
>> mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
>> }
>>
>> @@ -1260,7 +1271,7 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
>> struct mlx5_cmd_msg *out, void *uout, int uout_size,
>> mlx5_cmd_cbk_t callback,
>> void *context, int page_queue,
>> - u8 token, bool force_polling)
>> + u8 token, bool force_polling, bool *retain_msgs)
>> {
>> struct mlx5_cmd *cmd = &dev->cmd;
>> struct mlx5_cmd_work_ent *ent;
>> @@ -1313,6 +1324,7 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
>> return 0; /* mlx5_cmd_comp_handler() will put(ent) */
>>
>> err = wait_func(dev, ent);
>> + *retain_msgs = test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
>> if (err == -ETIMEDOUT || err == -ECANCELED || err == -EBUSY)
>> goto out_free;
>>
>> @@ -1732,6 +1744,66 @@ static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg)
>> }
>> }
>>
>> +/*
>> + * cmd_work_handler() takes an entry reference for the firmware event and
>> + * sets PENDING_COMP before ringing the command doorbell. Firmware can still
>> + * use the input and output DMA mailboxes after the caller times out.
>> + *
>> + * Claim PENDING_COMP under alloc_lock so exactly one handler makes the
>> + * mailbox and firmware-reference decisions:
>> + *
>> + * - A timeout that wins retains the mailboxes in the entry. If the command
>> + * interface is still up and the opcode is allowed, firmware can still
>> + * generate a real completion, so TIMEDOUT retains the firmware-event ref.
>> + * - A real completion that wins consumes its firmware-event ref normally.
>> + * - A real completion that loses is the late event after a timeout. It drops
>> + * the ref retained by TIMEDOUT; RETAIN_MSGS stays set until entry teardown.
>> + * - A reset completion cannot be followed by a real firmware event. It drops
>> + * a ref only when TIMEDOUT says the timeout retained one.
>> + *
>> + * A blocking timeout retains the mailboxes even when a real completion has
>> + * claimed PENDING_COMP. That handler keeps its firmware-event reference until
>> + * it finishes using the mailboxes. Keeping the reference transitions under
>> + * alloc_lock prevents timeout, firmware, and reset from consuming the same
>> + * reference.
>> + */
>> +static bool mlx5_cmd_claim_completion(struct mlx5_core_dev *dev,
>> + struct mlx5_cmd_work_ent *ent, u64 vec,
>> + bool forced, bool *drop_fw_ref)
>> +{
>> + struct mlx5_cmd *cmd = &dev->cmd;
>> + unsigned long flags;
>> + bool timed_out = forced && ent->ret == -ETIMEDOUT;
>> + bool pending;
>> +
>> + *drop_fw_ref = false;
>> + spin_lock_irqsave(&cmd->alloc_lock, flags);
>> + pending = test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP,
>> + &ent->state);
>> + if (pending) {
>> + if (timed_out)
>> + set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
>> +
>> + if (timed_out && !mlx5_cmd_is_down(dev) &&
>> + opcode_allowed(cmd, ent->op)) {
>> + set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
>> + } else {
>> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
>> + *drop_fw_ref = true;
>> + }
>> + } else if (!forced) {
>> + clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
>> + *drop_fw_ref = true;
>> + } else if (vec & MLX5_TRIGGERED_CMD_COMP) {
>> + /* Reset cannot receive a late firmware completion. */
>> + *drop_fw_ref = test_and_clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
>> + &ent->state);
>> + }
>> + spin_unlock_irqrestore(&cmd->alloc_lock, flags);
>> +
>> + return pending;
>> +}
>> +
>> static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool forced)
>> {
>> struct mlx5_cmd *cmd = &dev->cmd;
>> @@ -1744,38 +1816,33 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>> struct mlx5_cmd_stats *stats;
>> unsigned long flags;
>> unsigned long vector;
>> + bool pending;
>> + bool drop_fw_ref;
>>
>> /* there can be at most 32 command queues */
>> vector = vec & 0xffffffff;
>> for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
>> if (test_bit(i, &vector)) {
>> ent = cmd->ent_arr[i];
>> -
>> - if (forced && ent->ret == -ETIMEDOUT)
>> - set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
>> - &ent->state);
>> - else if (!forced) /* real FW completion */
>> - clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
>> - &ent->state);
>> + pending = mlx5_cmd_claim_completion(dev, ent, vec, forced,
>> + &drop_fw_ref);
>>
>> /* if we already completed the command, ignore it */
>> - if (!test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP,
>> - &ent->state)) {
>> - /* only real completion can free the cmd slot */
>> + if (!pending) {
>> if (!forced) {
>> - mlx5_core_err(dev, "Command completion arrived after timeout (entry idx = %d).\n",
>> + mlx5_core_err(dev,
>> + "Command completion arrived after timeout (entry idx = %d).\n",
>> ent->idx);
>> - cmd_ent_put(ent);
>> }
>> + if (drop_fw_ref)
>> + cmd_ent_put(ent);
>> continue;
>> }
>>
>> if (ent->callback && cancel_delayed_work(&ent->cb_timeout_work))
>> cmd_ent_put(ent); /* timeout work was canceled */
>>
>> - if (!forced || /* Real FW completion */
>> - mlx5_cmd_is_down(dev) || /* No real FW completion is expected */
>> - !opcode_allowed(cmd, ent->op))
>> + if (drop_fw_ref && ent->callback)
>> cmd_ent_put(ent);
>>
>> ent->ts2 = ktime_get_ns();
>> @@ -1816,17 +1883,23 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>> ent->out,
>> ent->uout_size);
>>
>> - mlx5_free_cmd_msg(dev, ent->out);
>> - free_msg(dev, ent->in);
>> + if (!test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS,
>> + &ent->state)) {
>> + mlx5_free_cmd_msg(dev, ent->out);
>> + free_msg(dev, ent->in);
>> + }
>>
>> /* final consumer is done, release ent */
>> cmd_ent_put(ent);
>> callback(err, context);
>> } else {
>> - /* release wait_func() so mlx5_cmd_invoke()
>> - * can make the final ent_put()
>> + /* No mailbox accesses after done. If the caller timed out,
>> + * the firmware reference keeps ent and its mailboxes alive
>> + * until this handler has finished.
>> */
>> complete(&ent->done);
>> + if (drop_fw_ref)
>> + cmd_ent_put(ent);
>> }
>> }
>> }
>> @@ -1965,6 +2038,7 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>> gfp_t gfp;
>> u8 token;
>> int err;
>> + bool retain_msgs = false;
>>
>> if (mlx5_cmd_is_down(dev) || !opcode_allowed(&dev->cmd, opcode))
>> return -ENXIO;
>> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>> }
>>
>> err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
>> - pages_queue, token, force_polling);
>> + pages_queue, token, force_polling, &retain_msgs);
>> if (callback && !err)
>> return 0;
>> + /* The entry releases retained DMA mailboxes with its final reference. */
>> + if (retain_msgs)
>> + goto out_up;
>>
>> if (err > 0) /* Failed in FW, command didn't execute */
>> err = deliv_status_to_err(err);
>> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>> index 83d0a83bbfbc..99cabe13e6c1 100644
>> --- a/include/linux/mlx5/driver.h
>> +++ b/include/linux/mlx5/driver.h
>> @@ -829,6 +829,7 @@ typedef void (*mlx5_cmd_cbk_t)(int status, void *context);
>> enum {
>> MLX5_CMD_ENT_STATE_PENDING_COMP,
>> MLX5_CMD_ENT_STATE_TIMEDOUT,
>> + MLX5_CMD_ENT_STATE_RETAIN_MSGS,
>> };
>>
>> struct mlx5_cmd_work_ent {
>
^ permalink raw reply [flat|nested] 8+ messages in thread