[PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Emil Tantilov]

The first patch in this series improves the locking around the setting
and clearing of the free_xn_bm bitmap. Previously the lock was only
taken during init shutdown and pop, but not the push function.

Patches 2 and 3 are fixes for the async handler. Patch 2 ensures the
payload size is set before the async handler is called, and patch 3 fixes
an sleeping bug due to nesting of raw/bh spinlocks.

Emil Tantilov (3):
  idpf: improve locking around idpf_vc_xn_push_free()
  idpf: set the payload size before calling the async handler
  idpf: fix PREEMPT_RT raw/bh spinlock nesting for async VC handling

 .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 40 ++++++++++++-------
 1 file changed, 25 insertions(+), 15 deletions(-)

-- 
2.37.3

[PATCH iwl-net 1/3] idpf: improve locking around idpf_vc_xn_push_free()

[Emil Tantilov]

Refactor the VC logic dealing with transaction accounting where
both push and pop_free are using the same spinlock free_xn_bm.
This resolves potential race when setting and clearing the bits
in the free_xn_bm bitmask.

Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
Reported-by: Ray Zhang <sgzhang@google.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 113ecfc16dd7..21a6c9d22085 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -401,12 +401,18 @@ struct idpf_vc_xn *idpf_vc_xn_pop_free(struct idpf_vc_xn_manager *vcxn_mngr)
  * idpf_vc_xn_push_free - Push a free transaction to free list
  * @vcxn_mngr: transaction manager to push to
  * @xn: transaction to push
+ *
+ * Callers must ensure idpf_vc_xn_release_bufs() has been called (under
+ * idpf_vc_xn_lock) before invoking this function. This function must
+ * be called without holding idpf_vc_xn_lock to avoid nesting a sleepable
+ * spinlock inside a raw_spinlock on PREEMPT_RT kernels.
  */
 static void idpf_vc_xn_push_free(struct idpf_vc_xn_manager *vcxn_mngr,
 				 struct idpf_vc_xn *xn)
 {
-	idpf_vc_xn_release_bufs(xn);
+	spin_lock_bh(&vcxn_mngr->xn_bm_lock);
 	set_bit(xn->idx, vcxn_mngr->free_xn_bm);
+	spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
 }
 
 /**
@@ -428,6 +434,7 @@ ssize_t idpf_vc_xn_exec(struct idpf_adapter *adapter,
 			const struct idpf_vc_xn_params *params)
 {
 	const struct kvec *send_buf = &params->send_buf;
+	bool push_free = false;
 	struct idpf_vc_xn *xn;
 	ssize_t retval;
 	u16 cookie;
@@ -514,10 +521,13 @@ ssize_t idpf_vc_xn_exec(struct idpf_adapter *adapter,
 	}
 
 release_and_unlock:
-	idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
+	idpf_vc_xn_release_bufs(xn);
+	push_free = true;
 	/* If we receive a VC reply after here, it will be dropped. */
 only_unlock:
 	idpf_vc_xn_unlock(xn);
+	if (push_free)
+		idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
 
 	return retval;
 }
@@ -559,7 +569,7 @@ idpf_vc_xn_forward_async(struct idpf_adapter *adapter, struct idpf_vc_xn *xn,
 	}
 
 release_bufs:
-	idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
+	idpf_vc_xn_release_bufs(xn);
 
 	return err;
 }
@@ -619,6 +629,7 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
 	case IDPF_VC_XN_ASYNC:
 		err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
 		idpf_vc_xn_unlock(xn);
+		idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
 		return err;
 	default:
 		dev_err_ratelimited(&adapter->pdev->dev, "Overwriting VC reply (op %d)\n",
-- 
2.37.3

Re: [PATCH iwl-net 1/3] idpf: improve locking around idpf_vc_xn_push_free()

[Li Li]

As discussed in the previous email, I had an minor & optional comment:

Personally, I think the code would read better if we just get rid of
the push_free state and use 2 separate code paths:

release_and_unlock:
idpf_vc_xn_release_bufs(xn);
idpf_vc_xn_unlock(xn);
idpf_vc_xn_push_free(&adapter->vcxn_mngr, xn);
return retval;

only_unlock:
idpf_vc_xn_unlock(xn);
return retval;

But I don't have strong opinions whether or not the suggestion above
is taken, and the patch LGTM.

Thank you Emil!

Reviewed-by: Li Li <boolli@google.com>



On Mon, Mar 16, 2026 at 4:28 PM Emil Tantilov <emil.s.tantilov@intel.com> wrote:
>
> Refactor the VC logic dealing with transaction accounting where
> both push and pop_free are using the same spinlock free_xn_bm.
> This resolves potential race when setting and clearing the bits
> in the free_xn_bm bitmask.
>
> Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
> Reported-by: Ray Zhang <sgzhang@google.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 113ecfc16dd7..21a6c9d22085 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -401,12 +401,18 @@ struct idpf_vc_xn *idpf_vc_xn_pop_free(struct idpf_vc_xn_manager *vcxn_mngr)
>   * idpf_vc_xn_push_free - Push a free transaction to free list
>   * @vcxn_mngr: transaction manager to push to
>   * @xn: transaction to push
> + *
> + * Callers must ensure idpf_vc_xn_release_bufs() has been called (under
> + * idpf_vc_xn_lock) before invoking this function. This function must
> + * be called without holding idpf_vc_xn_lock to avoid nesting a sleepable
> + * spinlock inside a raw_spinlock on PREEMPT_RT kernels.
>   */
>  static void idpf_vc_xn_push_free(struct idpf_vc_xn_manager *vcxn_mngr,
>                                  struct idpf_vc_xn *xn)
>  {
> -       idpf_vc_xn_release_bufs(xn);
> +       spin_lock_bh(&vcxn_mngr->xn_bm_lock);
>         set_bit(xn->idx, vcxn_mngr->free_xn_bm);
> +       spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
>  }
>
>  /**
> @@ -428,6 +434,7 @@ ssize_t idpf_vc_xn_exec(struct idpf_adapter *adapter,
>                         const struct idpf_vc_xn_params *params)
>  {
>         const struct kvec *send_buf = &params->send_buf;
> +       bool push_free = false;
>         struct idpf_vc_xn *xn;
>         ssize_t retval;
>         u16 cookie;
> @@ -514,10 +521,13 @@ ssize_t idpf_vc_xn_exec(struct idpf_adapter *adapter,
>         }
>
>  release_and_unlock:
> -       idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
> +       idpf_vc_xn_release_bufs(xn);
> +       push_free = true;
>         /* If we receive a VC reply after here, it will be dropped. */
>  only_unlock:
>         idpf_vc_xn_unlock(xn);
> +       if (push_free)
> +               idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
>
>         return retval;
>  }
> @@ -559,7 +569,7 @@ idpf_vc_xn_forward_async(struct idpf_adapter *adapter, struct idpf_vc_xn *xn,
>         }
>
>  release_bufs:
> -       idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
> +       idpf_vc_xn_release_bufs(xn);
>
>         return err;
>  }
> @@ -619,6 +629,7 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
>         case IDPF_VC_XN_ASYNC:
>                 err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
>                 idpf_vc_xn_unlock(xn);
> +               idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
>                 return err;
>         default:
>                 dev_err_ratelimited(&adapter->pdev->dev, "Overwriting VC reply (op %d)\n",
> --
> 2.37.3
>

[PATCH iwl-net 2/3] idpf: set the payload size before calling the async handler

[Emil Tantilov]

Set the payload size before forwarding the reply to the async handler.
Without this, xn->reply_sz will be 0 and idpf_mac_filter_async_handler()
will never get past the size check.

Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 21a6c9d22085..6b9692b30040 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -627,6 +627,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
 		err = -ENXIO;
 		goto out_unlock;
 	case IDPF_VC_XN_ASYNC:
+		/* Set reply_sz from the actual payload so that async_handler
+		 * can evaluate the response.
+		 */
+		xn->reply_sz = ctlq_msg->data_len;
 		err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
 		idpf_vc_xn_unlock(xn);
 		idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
-- 
2.37.3

Re: [PATCH iwl-net 2/3] idpf: set the payload size before calling the async handler

[Li Li]

On Mon, Mar 16, 2026 at 4:28 PM Emil Tantilov <emil.s.tantilov@intel.com> wrote:
>
> Set the payload size before forwarding the reply to the async handler.
> Without this, xn->reply_sz will be 0 and idpf_mac_filter_async_handler()
> will never get past the size check.
>
> Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 21a6c9d22085..6b9692b30040 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -627,6 +627,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
>                 err = -ENXIO;
>                 goto out_unlock;
>         case IDPF_VC_XN_ASYNC:

Optional comment: could we only set the size if ctlq_msg->data_len >
0, in case the hw returns some invalid values?

> +               /* Set reply_sz from the actual payload so that async_handler
> +                * can evaluate the response.
> +                */
> +               xn->reply_sz = ctlq_msg->data_len;
>                 err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
>                 idpf_vc_xn_unlock(xn);
>                 idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
> --
> 2.37.3
>

Reviewed-by: Li Li <boolli@google.com>

Re: [PATCH iwl-net 2/3] idpf: set the payload size before calling the async handler

[Tantilov, Emil S]

On 3/17/2026 5:11 PM, Li Li wrote:
> On Mon, Mar 16, 2026 at 4:28 PM Emil Tantilov <emil.s.tantilov@intel.com> wrote:
>>
>> Set the payload size before forwarding the reply to the async handler.
>> Without this, xn->reply_sz will be 0 and idpf_mac_filter_async_handler()
>> will never get past the size check.
>>
>> Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>> ---
>>   drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> index 21a6c9d22085..6b9692b30040 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> @@ -627,6 +627,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
>>                  err = -ENXIO;
>>                  goto out_unlock;
>>          case IDPF_VC_XN_ASYNC:
> 
> Optional comment: could we only set the size if ctlq_msg->data_len >
> 0, in case the hw returns some invalid values?

0 is a valid size, but event if it wasn't the async handler already has
a check for it, which is how this issue was caught (see description).

Thanks,
Emil

> 
>> +               /* Set reply_sz from the actual payload so that async_handler
>> +                * can evaluate the response.
>> +                */
>> +               xn->reply_sz = ctlq_msg->data_len;
>>                  err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
>>                  idpf_vc_xn_unlock(xn);
>>                  idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
>> --
>> 2.37.3
>>
> 
> Reviewed-by: Li Li <boolli@google.com>

[PATCH iwl-net 3/3] idpf: fix PREEMPT_RT raw/bh spinlock nesting for async VC handling

[Emil Tantilov]

Restructure the ASYNC case to allow calling idpf_vc_xn_forward_reply()
outside of idpf_vc_xn_lock(). This avoids invalid wait context reported by
the kernel due to the async handler taking BH spinlock:

[  805.726977] =============================
[  805.726991] [ BUG: Invalid wait context ]
[  805.727006] 7.0.0-rc2-net-devq-031026+ #28 Tainted: G S         OE
[  805.727026] -----------------------------
[  805.727038] kworker/u261:0/572 is trying to lock:
[  805.727051] ff190da6a8dbb6a0 (&vport_config->mac_filter_list_lock){+...}-{3:3}, at: idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
[  805.727099] other info that might help us debug this:
[  805.727111] context-{5:5}
[  805.727119] 3 locks held by kworker/u261:0/572:
[  805.727132]  #0: ff190da6db3e6148 ((wq_completion)idpf-0000:83:00.0-mbx){+.+.}-{0:0}, at: process_one_work+0x4b5/0x730
[  805.727163]  #1: ff3c6f0a6131fe50 ((work_completion)(&(&adapter->mbx_task)->work)){+.+.}-{0:0}, at: process_one_work+0x1e5/0x730
[  805.727191]  #2: ff190da765190020 (&x->wait#34){+.+.}-{2:2}, at: idpf_recv_mb_msg+0xc8/0x710 [idpf]
[  805.727218] stack backtrace:
...
[  805.727238] Workqueue: idpf-0000:83:00.0-mbx idpf_mbx_task [idpf]
[  805.727247] Call Trace:
[  805.727249]  <TASK>
[  805.727251]  dump_stack_lvl+0x77/0xb0
[  805.727259]  __lock_acquire+0xb3b/0x2290
[  805.727268]  ? __irq_work_queue_local+0x59/0x130
[  805.727275]  lock_acquire+0xc6/0x2f0
[  805.727277]  ? idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
[  805.727284]  ? _printk+0x5b/0x80
[  805.727290]  _raw_spin_lock_bh+0x38/0x50
[  805.727298]  ? idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
[  805.727303]  idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
[  805.727310]  idpf_recv_mb_msg+0x1c8/0x710 [idpf]
[  805.727317]  process_one_work+0x226/0x730
[  805.727322]  worker_thread+0x19e/0x340
[  805.727325]  ? __pfx_worker_thread+0x10/0x10
[  805.727328]  kthread+0xf4/0x130
[  805.727333]  ? __pfx_kthread+0x10/0x10
[  805.727336]  ret_from_fork+0x32c/0x410
[  805.727345]  ? __pfx_kthread+0x10/0x10
[  805.727347]  ret_from_fork_asm+0x1a/0x30
[  805.727354]  </TASK>

Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
Reported-by: Ray Zhang <sgzhang@google.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 6b9692b30040..8ceabd86e172 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -546,32 +546,24 @@ static int
 idpf_vc_xn_forward_async(struct idpf_adapter *adapter, struct idpf_vc_xn *xn,
 			 const struct idpf_ctlq_msg *ctlq_msg)
 {
-	int err = 0;
-
 	if (ctlq_msg->cookie.mbx.chnl_opcode != xn->vc_op) {
 		dev_err_ratelimited(&adapter->pdev->dev, "Async message opcode does not match transaction opcode (msg: %d) (xn: %d)\n",
 				    ctlq_msg->cookie.mbx.chnl_opcode, xn->vc_op);
 		xn->reply_sz = 0;
-		err = -EINVAL;
-		goto release_bufs;
+		return -EINVAL;
 	}
 
-	if (xn->async_handler) {
-		err = xn->async_handler(adapter, xn, ctlq_msg);
-		goto release_bufs;
-	}
+	if (xn->async_handler)
+		return xn->async_handler(adapter, xn, ctlq_msg);
 
 	if (ctlq_msg->cookie.mbx.chnl_retval) {
 		xn->reply_sz = 0;
 		dev_err_ratelimited(&adapter->pdev->dev, "Async message failure (op %d)\n",
 				    ctlq_msg->cookie.mbx.chnl_opcode);
-		err = -EINVAL;
+		return -EINVAL;
 	}
 
-release_bufs:
-	idpf_vc_xn_release_bufs(xn);
-
-	return err;
+	return 0;
 }
 
 /**
@@ -631,7 +623,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
 		 * can evaluate the response.
 		 */
 		xn->reply_sz = ctlq_msg->data_len;
+		idpf_vc_xn_unlock(xn);
 		err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
+		idpf_vc_xn_lock(xn);
+		idpf_vc_xn_release_bufs(xn);
 		idpf_vc_xn_unlock(xn);
 		idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
 		return err;
-- 
2.37.3

Re: [PATCH iwl-net 3/3] idpf: fix PREEMPT_RT raw/bh spinlock nesting for async VC handling

[Li Li]

Thank you!

Reviewed-by: Li Li <boolli@google.com>

On Mon, Mar 16, 2026 at 4:28 PM Emil Tantilov <emil.s.tantilov@intel.com> wrote:
>
> Restructure the ASYNC case to allow calling idpf_vc_xn_forward_reply()
> outside of idpf_vc_xn_lock(). This avoids invalid wait context reported by
> the kernel due to the async handler taking BH spinlock:
>
> [  805.726977] =============================
> [  805.726991] [ BUG: Invalid wait context ]
> [  805.727006] 7.0.0-rc2-net-devq-031026+ #28 Tainted: G S         OE
> [  805.727026] -----------------------------
> [  805.727038] kworker/u261:0/572 is trying to lock:
> [  805.727051] ff190da6a8dbb6a0 (&vport_config->mac_filter_list_lock){+...}-{3:3}, at: idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
> [  805.727099] other info that might help us debug this:
> [  805.727111] context-{5:5}
> [  805.727119] 3 locks held by kworker/u261:0/572:
> [  805.727132]  #0: ff190da6db3e6148 ((wq_completion)idpf-0000:83:00.0-mbx){+.+.}-{0:0}, at: process_one_work+0x4b5/0x730
> [  805.727163]  #1: ff3c6f0a6131fe50 ((work_completion)(&(&adapter->mbx_task)->work)){+.+.}-{0:0}, at: process_one_work+0x1e5/0x730
> [  805.727191]  #2: ff190da765190020 (&x->wait#34){+.+.}-{2:2}, at: idpf_recv_mb_msg+0xc8/0x710 [idpf]
> [  805.727218] stack backtrace:
> ...
> [  805.727238] Workqueue: idpf-0000:83:00.0-mbx idpf_mbx_task [idpf]
> [  805.727247] Call Trace:
> [  805.727249]  <TASK>
> [  805.727251]  dump_stack_lvl+0x77/0xb0
> [  805.727259]  __lock_acquire+0xb3b/0x2290
> [  805.727268]  ? __irq_work_queue_local+0x59/0x130
> [  805.727275]  lock_acquire+0xc6/0x2f0
> [  805.727277]  ? idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
> [  805.727284]  ? _printk+0x5b/0x80
> [  805.727290]  _raw_spin_lock_bh+0x38/0x50
> [  805.727298]  ? idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
> [  805.727303]  idpf_mac_filter_async_handler+0xe9/0x260 [idpf]
> [  805.727310]  idpf_recv_mb_msg+0x1c8/0x710 [idpf]
> [  805.727317]  process_one_work+0x226/0x730
> [  805.727322]  worker_thread+0x19e/0x340
> [  805.727325]  ? __pfx_worker_thread+0x10/0x10
> [  805.727328]  kthread+0xf4/0x130
> [  805.727333]  ? __pfx_kthread+0x10/0x10
> [  805.727336]  ret_from_fork+0x32c/0x410
> [  805.727345]  ? __pfx_kthread+0x10/0x10
> [  805.727347]  ret_from_fork_asm+0x1a/0x30
> [  805.727354]  </TASK>
>
> Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
> Reported-by: Ray Zhang <sgzhang@google.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 21 +++++++------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 6b9692b30040..8ceabd86e172 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -546,32 +546,24 @@ static int
>  idpf_vc_xn_forward_async(struct idpf_adapter *adapter, struct idpf_vc_xn *xn,
>                          const struct idpf_ctlq_msg *ctlq_msg)
>  {
> -       int err = 0;
> -
>         if (ctlq_msg->cookie.mbx.chnl_opcode != xn->vc_op) {
>                 dev_err_ratelimited(&adapter->pdev->dev, "Async message opcode does not match transaction opcode (msg: %d) (xn: %d)\n",
>                                     ctlq_msg->cookie.mbx.chnl_opcode, xn->vc_op);
>                 xn->reply_sz = 0;
> -               err = -EINVAL;
> -               goto release_bufs;
> +               return -EINVAL;
>         }
>
> -       if (xn->async_handler) {
> -               err = xn->async_handler(adapter, xn, ctlq_msg);
> -               goto release_bufs;
> -       }
> +       if (xn->async_handler)
> +               return xn->async_handler(adapter, xn, ctlq_msg);
>
>         if (ctlq_msg->cookie.mbx.chnl_retval) {
>                 xn->reply_sz = 0;
>                 dev_err_ratelimited(&adapter->pdev->dev, "Async message failure (op %d)\n",
>                                     ctlq_msg->cookie.mbx.chnl_opcode);
> -               err = -EINVAL;
> +               return -EINVAL;
>         }
>
> -release_bufs:
> -       idpf_vc_xn_release_bufs(xn);
> -
> -       return err;
> +       return 0;
>  }
>
>  /**
> @@ -631,7 +623,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
>                  * can evaluate the response.
>                  */
>                 xn->reply_sz = ctlq_msg->data_len;
> +               idpf_vc_xn_unlock(xn);
>                 err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
> +               idpf_vc_xn_lock(xn);
> +               idpf_vc_xn_release_bufs(xn);
>                 idpf_vc_xn_unlock(xn);
>                 idpf_vc_xn_push_free(adapter->vcxn_mngr, xn);
>                 return err;
> --
> 2.37.3
>

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Sebastian Andrzej Siewior]

On 2026-03-16 16:28:16 [-0700], Emil Tantilov wrote:
> The first patch in this series improves the locking around the setting
> and clearing of the free_xn_bm bitmap. Previously the lock was only
> taken during init shutdown and pop, but not the push function.
> 
> Patches 2 and 3 are fixes for the async handler. Patch 2 ensures the
> payload size is set before the async handler is called, and patch 3 fixes
> an sleeping bug due to nesting of raw/bh spinlocks.

Why is there a raw_spinlock_t? From a quick look a spinlock_t would do
just fine with not runtime change for !PREEMPT_RT.

Sebastian

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Tantilov, Emil S]

On 3/17/2026 2:00 AM, Sebastian Andrzej Siewior wrote:
> On 2026-03-16 16:28:16 [-0700], Emil Tantilov wrote:
>> The first patch in this series improves the locking around the setting
>> and clearing of the free_xn_bm bitmap. Previously the lock was only
>> taken during init shutdown and pop, but not the push function.
>>
>> Patches 2 and 3 are fixes for the async handler. Patch 2 ensures the
>> payload size is set before the async handler is called, and patch 3 fixes
>> an sleeping bug due to nesting of raw/bh spinlocks.
> 
> Why is there a raw_spinlock_t? From a quick look a spinlock_t would do
> just fine with not runtime change for !PREEMPT_RT.

The handling of the virtchannel messages is done via the completion API
and the transactions are using the raw spinlock from struct
swait_queue_head:

https://elixir.bootlin.com/linux/v6.19.8/source/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c#L298

https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/tree/include/linux/swait.h?h=dev-queue#n44

Thanks,
Emil

> 
> Sebastian

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Sebastian Andrzej Siewior]

On 2026-03-17 07:20:18 [-0700], Tantilov, Emil S wrote:
> > Why is there a raw_spinlock_t? From a quick look a spinlock_t would do
> > just fine with not runtime change for !PREEMPT_RT.
> 
> The handling of the virtchannel messages is done via the completion API
> and the transactions are using the raw spinlock from struct
> swait_queue_head:
> 
> https://elixir.bootlin.com/linux/v6.19.8/source/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c#L298
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/tree/include/linux/swait.h?h=dev-queue#n44

I am aware that completions use a raw_spinlock_t. I don't see the link.
What would break if you make that lock a spinlock_t?

> Thanks,
> Emil

Sebastian

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Tantilov, Emil S]

On 3/17/2026 7:38 AM, Sebastian Andrzej Siewior wrote:
> On 2026-03-17 07:20:18 [-0700], Tantilov, Emil S wrote:
>>> Why is there a raw_spinlock_t? From a quick look a spinlock_t would do
>>> just fine with not runtime change for !PREEMPT_RT.
>>
>> The handling of the virtchannel messages is done via the completion API
>> and the transactions are using the raw spinlock from struct
>> swait_queue_head:
>>
>> https://elixir.bootlin.com/linux/v6.19.8/source/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c#L298
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/tree/include/linux/swait.h?h=dev-queue#n44
> 
> I am aware that completions use a raw_spinlock_t. I don't see the link.
> What would break if you make that lock a spinlock_t?

Right. Scope and risk - these fixes are specifically for the async
handler and I did not want to touch the global locking that will
impact the entire VC handling. We do have series in flight for -next
that refactor that code, while moving it to libie:
https://lore.kernel.org/netdev/20251117134912.18566-10-larysa.zaremba@intel.com/

... that also remove the raw spinlock. With that being said, I can look
into converting the lock to spinlock_t if that is the preferred approach.

Thanks,
Emil

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Sebastian Andrzej Siewior]

On 2026-03-17 12:30:39 [-0700], Tantilov, Emil S wrote:
> > What would break if you make that lock a spinlock_t?
> 
> Right. Scope and risk - these fixes are specifically for the async
> handler and I did not want to touch the global locking that will
> impact the entire VC handling. We do have series in flight for -next
> that refactor that code, while moving it to libie:
> https://lore.kernel.org/netdev/20251117134912.18566-10-larysa.zaremba@intel.com/

Now I understood. You fiddle with the completion's lock. That is
something that should not have been done.

> ... that also remove the raw spinlock. With that being said, I can look
> into converting the lock to spinlock_t if that is the preferred approach.

The preferred approach is that, if you pick raw_spinlock_t for locking,
you are aware of all the consequences and you have a solid reason for
it. The comment in the file says
| For now, this API is only used from within a workqueue context;
| - * raw_spin_lock() is enough.

that is not it.

> Thanks,
> Emil

Sebastian

Re: [PATCH iwl-net 0/3] idpf: virtchnl locking and async fixes

[Tantilov, Emil S]

On 3/18/2026 12:24 AM, Sebastian Andrzej Siewior wrote:
> On 2026-03-17 12:30:39 [-0700], Tantilov, Emil S wrote:
>>> What would break if you make that lock a spinlock_t?
>>
>> Right. Scope and risk - these fixes are specifically for the async
>> handler and I did not want to touch the global locking that will
>> impact the entire VC handling. We do have series in flight for -next
>> that refactor that code, while moving it to libie:
>> https://lore.kernel.org/netdev/20251117134912.18566-10-larysa.zaremba@intel.com/
> 
> Now I understood. You fiddle with the completion's lock. That is
> something that should not have been done.
> 
>> ... that also remove the raw spinlock. With that being said, I can look
>> into converting the lock to spinlock_t if that is the preferred approach.
> 
> The preferred approach is that, if you pick raw_spinlock_t for locking,
> you are aware of all the consequences and you have a solid reason for
> it. The comment in the file says
> | For now, this API is only used from within a workqueue context;
> | - * raw_spin_lock() is enough.
> 
> that is not it.

Understood. I will convert it in v2.

Thanks for the feedback!
Emil