On Mon, Oct 26, 2026 at 03:19:07PM -0700, Emil Tantilov wrote:
> libie_ctlq_xn_shutdown() waits on xnm->can_destroy for the outstanding
> transactions to be returned. libie_ctlq_xn_push_free() signals it with
> complete() when the last entry goes back to the free bitmap. Once the
> manager is shut down, libie_ctlq_xn_pop_free() hands out no more entries,
> so that point is reached exactly once and only one waiter is woken.
>
> Nothing restricts libie_ctlq_xn_shutdown() to a single caller, though.
> If two threads call it while a transaction is still outstanding, both see
> the entry in use and both wait, and the one that is not woken blocks
> forever, as wait_for_completion() has no timeout.
>
> A drained manager stays drained until libie_ctlq_xn_deinit() frees it, so
> signal it with complete_all(). All callers waiting at that point are
> released, and later ones find the bitmap full and return at once.
>
Yeah, I should have used complete_all().
> Signal it before free_xns_bm_lock is dropped. A caller that finds the
> bitmap full then knows the completion has already been signalled. That
> leaves libie_ctlq_xn_push_free() holding the lock after it has woken the
> waiters, so have a woken waiter take and release free_xns_bm_lock before
> it returns, the same lock/unlock pair completion_done() uses to wait for
> complete(), see commit dec13c42d21a ("completion: Replace
> spin_unlock_wait() with lock/unlock pair"). Its unlock is then the last
> access to the manager by the time any caller returns.
The way you phrase this, the locking change seems redundant, as complete()
provides the same end synchronization. I can't see a scenario, where just
complete()->complete_all() change wouldn't be enough by itself, so please
describe the problematic sequence.
>
> The manager must still outlive every caller, since a woken waiter has to
> retake the completion and bitmap locks before it returns. Spell that out
> in the kernel-doc: libie_ctlq_xn_deinit() must not be called until every
> libie_ctlq_xn_shutdown() caller has returned.
>
> The next patch makes idpf call libie_ctlq_xn_shutdown() from its service
> task, which can run concurrently with the reset and remove paths.
>
> Fixes: b4ff4e626a7c ("libie: add bookkeeping support for control queue
> messages")
> Assisted-by: LLM
> Signed-off-by: Emil Tantilov <[email protected]>
> ---
> drivers/net/ethernet/intel/libie/controlq.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/libie/controlq.c
> b/drivers/net/ethernet/intel/libie/controlq.c
> index 6214fc036ce5f..9bcb022ed9782 100644
> --- a/drivers/net/ethernet/intel/libie/controlq.c
> +++ b/drivers/net/ethernet/intel/libie/controlq.c
> @@ -735,13 +735,10 @@ static bool __libie_ctlq_xn_push_free(struct
> libie_ctlq_xn_manager *xnm,
> static void libie_ctlq_xn_push_free(struct libie_ctlq_xn_manager *xnm,
> struct libie_ctlq_xn *xn)
> {
> - bool can_destroy;
> -
> - scoped_guard(spinlock, &xnm->free_xns_bm_lock)
> - can_destroy = __libie_ctlq_xn_push_free(xnm, xn);
> + guard(spinlock)(&xnm->free_xns_bm_lock);
>
> - if (can_destroy)
> - complete(&xnm->can_destroy);
> + if (__libie_ctlq_xn_push_free(xnm, xn))
> + complete_all(&xnm->can_destroy);
> }
>
> /**
> @@ -1171,7 +1168,9 @@ EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_send_clean,
> "LIBIE_CP");
> * @xnm: pointer to the transaction manager
> *
> * Synchronously terminate existing transactions and stop accepting new ones.
> - * Async transactions are discarded without invoking resp_cb.
> + * Async transactions are discarded without invoking resp_cb. May be called
> + * concurrently, libie_ctlq_xn_deinit() must not be called until every caller
> + * has returned.
> */
> void libie_ctlq_xn_shutdown(struct libie_ctlq_xn_manager *xnm)
> {
> @@ -1218,8 +1217,12 @@ void libie_ctlq_xn_shutdown(struct
> libie_ctlq_xn_manager *xnm)
>
> spin_unlock(&xnm->free_xns_bm_lock);
>
> - if (must_wait)
> + if (must_wait) {
> wait_for_completion(&xnm->can_destroy);
> + /* Synchronize with libie_ctlq_xn_push_free() unlocking */
> + spin_lock(&xnm->free_xns_bm_lock);
> + spin_unlock(&xnm->free_xns_bm_lock);
> + }
> }
> EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_shutdown, "LIBIE_CP");
>
> --
> 2.37.3
>
