Received-SPF: pass (google.com: domain of linux-kernel+bounces-76441-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 22 Feb 2024 11:46:59 +0000
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: "Wu, Wentong" <wentong.wu@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>,
	"Winkler, Tomas" <tomas.winkler@intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ
 handler
Message-ID: <Zdc0M9Uf2zn63P0e@kekkonen.localdomain>
References: <20240219195807.517742-1-sakari.ailus@linux.intel.com>
 <20240219195807.517742-2-sakari.ailus@linux.intel.com>
 <MW5PR11MB5787130A75404600F47A7D9C8D562@MW5PR11MB5787.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MW5PR11MB5787130A75404600F47A7D9C8D562@MW5PR11MB5787.namprd11.prod.outlook.com>

Hi Wentong,

On Thu, Feb 22, 2024 at 03:26:18AM +0000, Wu, Wentong wrote:
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
> 
> Does this mean we can't use wake_up() in isr? 

Good question. A lot of callers currently do.

In this case, handle_edge_irq() takes the raw spinlock and acquiring the
wake queue spinlock in wake_up() leads to sleeping IRQs disabled (see
below).

I don't think there's any harm in moving the wake_up() to the threaded
handler.

-------8<---------------------------
[   54.216989] =============================
[   54.218458] [ BUG: Invalid wait context ]
[   54.219913] 6.8.0-rc2+ #1972 Not tainted
[   54.221493] -----------------------------
[   54.223165] swapper/4/0 is trying to lock:
[   54.224756] ffff888112d04688 (&tp->xfer_wait){....}-{3:3}, at: __wake_up_common_lock+0x25/0x60
[   54.226426] other info that might help us debug this:
[   54.228189] context-{2:2}
[   54.229817] no locks held by swapper/4/0.
[   54.231453] stack backtrace:
[   54.233078] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 6.8.0-rc2+ #1972
[   54.234720] Hardware name: Dell Inc. XPS 9315/0WWXF6, BIOS 1.3.0 08/16/2022
[   54.236361] Call Trace:
[   54.237983]  <IRQ>
[   54.239594]  dump_stack_lvl+0x6a/0x9f
[   54.241147]  __lock_acquire+0x43e/0x11fb
[   54.242704]  ? mark_lock+0x96/0x34d
[   54.244212]  ? check_chain_key+0xe5/0x132
[   54.245759]  ? __wake_up_common_lock+0x25/0x60
[   54.247312]  lock_acquire+0x128/0x27c
[   54.248848]  ? __wake_up_common_lock+0x25/0x60
[   54.250384]  _raw_spin_lock_irqsave+0x3e/0x51
[   54.251922]  ? __wake_up_common_lock+0x25/0x60
[   54.253524]  __wake_up_common_lock+0x25/0x60
[   54.255049]  vsc_tp_isr+0x1e/0x28 [mei_vsc_hw]
[   54.256569]  __handle_irq_event_percpu+0xb4/0x1aa
[   54.258054]  handle_irq_event_percpu+0xf/0x32
[   54.259550]  handle_irq_event+0x34/0x53
[   54.260982]  handle_edge_irq+0xb0/0xcf
[   54.262440]  handle_irq_desc+0x39/0x43
[   54.263898]  intel_gpio_irq+0x105/0x15a
[   54.265348]  __handle_irq_event_percpu+0xb4/0x1aa
[   54.266792]  handle_irq_event_percpu+0xf/0x32
[   54.268244]  handle_irq_event+0x34/0x53
[   54.269634]  handle_fasteoi_irq+0xa5/0x131
[   54.271018]  __common_interrupt+0xdc/0xeb
[   54.272392]  common_interrupt+0x96/0xc1
[   54.273754]  </IRQ>
[   54.275109]  <TASK>
[   54.276391]  asm_common_interrupt+0x22/0x40
[   54.277733] RIP: 0010:cpuidle_enter_state+0x171/0x253
[   54.279071] Code: 8b 73 04 83 cf ff 49 89 c6 e8 62 fe df ff 31 ff e8 65 29 79 ff 45 84 ff 74 07 31 ff e8 0d c3 7f ff e8 fc 91 85 ff fb 45 85 e4 <0f> 88 ba 00 00 00 48 8b 04 24 49 63 cc 48 6b d1 68 49 29 c6 48 89
[   54.280496] RSP: 0018:ffffc900001a7e88 EFLAGS: 00000202
[   54.281935] RAX: 0000000000000004 RBX: ffffe8ffffa310c0 RCX: 000000000000001f
[   54.283382] RDX: 0000000c9f7cf88f RSI: ffffffff82103e63 RDI: ffffffff8209b592
[   54.284828] RBP: 0000000000000004 R08: 0000000000000000 R09: 0000000000000001
[   54.286274] R10: 0000000001151268 R11: 0000000000000020 R12: 0000000000000004
[   54.287699] R13: ffffffff8266fec0 R14: 0000000c9f7cf88f R15: 0000000000000000
[   54.289127]  ? cpuidle_enter_state+0x16d/0x253
[   54.290564]  cpuidle_enter+0x2a/0x38
[   54.291987]  do_idle+0x190/0x204
[   54.293394]  cpu_startup_entry+0x2a/0x2c
[   54.294797]  start_secondary+0x12d/0x12d
[   54.296198]  secondary_startup_64_no_verify+0x15e/0x16b
[   54.297595]  </TASK>
-------8<---------------------------

> 
> BR,
> Wentong
> 
> > This leads to sleeping in atomic context.
> > 
> > Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> > where it can be safely called.
> > 
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/misc/mei/vsc-tp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> > 6f4a4be6ccb5..2b69ada9349e 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> > 
> >  	atomic_inc(&tp->assert_cnt);
> > 
> > -	wake_up(&tp->xfer_wait);
> > -
> >  	return IRQ_WAKE_THREAD;
> >  }
> > 
> > @@ -425,6 +423,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> > *data)  {
> >  	struct vsc_tp *tp = data;
> > 
> > +	wake_up(&tp->xfer_wait);
> > +
> >  	if (tp->event_notify)
> >  		tp->event_notify(tp->event_notify_context);
> > 

-- 
Regards,

Sakari Ailus