Commit 59147811 authored by Emil Tantilov's avatar Emil Tantilov Committed by Tony Nguyen
Browse files

idpf: fix PREEMPT_RT raw/bh spinlock nesting for async VC handling 



Switch from using the completion's raw spinlock to a local lock in the
idpf_vc_xn struct. The conversion is safe because complete/_all() are
called outside the lock and there is no reason to share the completion
lock in the current logic. 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: 34c21fa8 ("idpf: implement virtchnl transaction manager")
Cc: stable@vger.kernel.org
Suggested-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Reported-by: default avatarRay Zhang <sgzhang@google.com>
Signed-off-by: default avatarEmil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: default avatarAleksandr Loktionov <aleksandr.loktionov@intel.com>
Acked-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: default avatarSamuel Salin <Samuel.salin@intel.com>
Signed-off-by: default avatarTony Nguyen <anthony.l.nguyen@intel.com>
parent 1caa871b

drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

+5 −9
Original line number Diff line number Diff line
@@ -287,26 +287,21 @@ int idpf_send_mb_msg(struct idpf_adapter *adapter, struct idpf_ctlq_info *asq, 
	return err;

}



/* API for virtchnl "transaction" support ("xn" for short).

 *

 * We are reusing the completion lock to serialize the accesses to the

 * transaction state for simplicity, but it could be its own separate synchro

 * as well. For now, this API is only used from within a workqueue context;

 * raw_spin_lock() is enough.

 */

/* API for virtchnl "transaction" support ("xn" for short). */



/**

 * idpf_vc_xn_lock - Request exclusive access to vc transaction

 * @xn: struct idpf_vc_xn* to access

 */

#define idpf_vc_xn_lock(xn)			\

	raw_spin_lock(&(xn)->completed.wait.lock)

	spin_lock(&(xn)->lock)



/**

 * idpf_vc_xn_unlock - Release exclusive access to vc transaction

 * @xn: struct idpf_vc_xn* to access

 */

#define idpf_vc_xn_unlock(xn)		\

	raw_spin_unlock(&(xn)->completed.wait.lock)

	spin_unlock(&(xn)->lock)



/**

 * idpf_vc_xn_release_bufs - Release reference to reply buffer(s) and
@@ -338,6 +333,7 @@ static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr) 
		xn->state = IDPF_VC_XN_IDLE;

		xn->idx = i;

		idpf_vc_xn_release_bufs(xn);

		spin_lock_init(&xn->lock);

		init_completion(&xn->completed);

	}

drivers/net/ethernet/intel/idpf/idpf_virtchnl.h

+3 −2
Original line number Diff line number Diff line
@@ -42,8 +42,8 @@ typedef int (*async_vc_cb) (struct idpf_adapter *, struct idpf_vc_xn *, 
 * struct idpf_vc_xn - Data structure representing virtchnl transactions

 * @completed: virtchnl event loop uses that to signal when a reply is

 *	       available, uses kernel completion API

 * @state: virtchnl event loop stores the data below, protected by the

 *	   completion's lock.

 * @lock: protects the transaction state fields below

 * @state: virtchnl event loop stores the data below, protected by @lock

 * @reply_sz: Original size of reply, may be > reply_buf.iov_len; it will be

 *	      truncated on its way to the receiver thread according to

 *	      reply_buf.iov_len.
@@ -58,6 +58,7 @@ typedef int (*async_vc_cb) (struct idpf_adapter *, struct idpf_vc_xn *, 
 */

struct idpf_vc_xn {

	struct completion completed;

	spinlock_t lock;

	enum idpf_vc_xn_state state;

	size_t reply_sz;

	struct kvec reply;