[PATCH v6.4-rc5-rt4] rt: vmd: make cfg_lock a raw spinlock

[PATCH v6.4-rc5-rt4] rt: vmd: make cfg_lock a raw spinlock

[Luis Claudio R. Goncalves]

Sebastian, Steven,

I don't know if this is the best solution for the problem, but surely the
simplest one. If this patch makes sense, it could probably go upstream as
the changes only affect RT.

-------

rt: vmd: make cfg_lock a raw spinlock

This call sequence triggers the backtrace listed below:

    pci_user_read_config_dword()  --->  vmd_pci_read()

The first function, pci_user_read_config_dword(), takes a raw_spin_lock
(a classic spin lock) and disables preemption and interrupts. Then
vmd_pci_read() takes a RT spinlock (a rtmutex), which can sleep, hence
the complaint about a sleeping function being called from atomic context.


[  168.466387] LTP: starting read_all_proc (read_all -d /proc -q -r 3)
[  168.614389] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[  168.614393] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 10268, name: read_all
[  168.614395] preempt_count: 1, expected: 0
[  168.614396] RCU nest depth: 0, expected: 0
[  168.614397] INFO: lockdep is turned off.
[  168.614398] irq event stamp: 0
[  168.614398] hardirqs last  enabled at (0): [<0000000000000000>] 0x0
[  168.614402] hardirqs last disabled at (0): [<ffffffffa4b0f3bf>] copy_process+0x80f/0x1990
[  168.614411] softirqs last  enabled at (0): [<ffffffffa4b0f3bf>] copy_process+0x80f/0x1990
[  168.614414] softirqs last disabled at (0): [<0000000000000000>] 0x0
[  168.614415] Preemption disabled at:
[  168.614416] [<0000000000000000>] 0x0
[  168.614418] CPU: 19 PID: 10268 Comm: read_all Tainted: G        W          6.4.0-rc5-rt4 #2
[  168.614421] Hardware name: Dell Inc. Precision 7820 Tower/0804P1, BIOS 2.6.3 05/04/2020
[  168.614422] Call Trace:
[  168.614424]  <TASK>
[  168.614425]  dump_stack_lvl+0x47/0x80
[  168.614433]  __might_resched+0x19b/0x250
[  168.614441]  rt_spin_lock+0x4c/0x100
[  168.614446]  ? vmd_pci_read+0x85/0xf0
[  168.614453]  vmd_pci_read+0x85/0xf0
[  168.614456]  pci_user_read_config_dword+0x78/0x100
[  168.614464]  proc_bus_pci_read+0x110/0x2e0
[  168.614471]  ? selinux_file_permission+0x117/0x150
[  168.614478]  proc_reg_read+0x59/0xa0
[  168.614483]  vfs_read+0xa7/0x2e0
[  168.614488]  ? lock_acquire+0x1b9/0x300
[  168.614492]  ? do_sys_openat2+0x85/0x160
[  168.614496]  ? kmem_cache_free+0x269/0x3c0
[  168.614502]  ksys_read+0x68/0xf0
[  168.614505]  do_syscall_64+0x59/0x90
[  168.614508]  ? do_syscall_64+0x69/0x90
[  168.614510]  ? do_syscall_64+0x69/0x90
[  168.614511]  ? trace_hardirqs_on_prepare+0x19/0x30
[  168.614515]  ? do_syscall_64+0x69/0x90
[  168.614516]  ? do_syscall_64+0x69/0x90
[  168.614517]  ? trace_irq_enable.constprop.0+0x1f/0xb0
[  168.614520]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  168.614525] RIP: 0033:0x7ff67e88eaf2
[  168.614527] Code: c0 e9 b2 fe ff ff 50 48 8d 3d ca 0c 08 00 e8 35 eb 01 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
[  168.614529] RSP: 002b:00007ffe5f3f2698 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[  168.614531] RAX: ffffffffffffffda RBX: 0000000001990b98 RCX: 00007ff67e88eaf2
[  168.614533] RDX: 00000000000003ff RSI: 00007ffe5f3f2750 RDI: 0000000000000003
[  168.614534] RBP: 00007ff67e73d028 R08: 0000000000037fde R09: 0000000000000004
[  168.614535] R10: 00007ffe5f3f9170 R11: 0000000000000246 R12: 0000000000000078
[  168.614537] R13: 0000000000000003 R14: 00007ff67e739000 R15: 000000000000281c
[  168.614541]  </TASK>


Signed-off-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
---
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 990630ec57c6a..3253e71fa0ad9 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -125,7 +125,7 @@ struct vmd_irq_list {
 struct vmd_dev {
 	struct pci_dev		*dev;
 
-	spinlock_t		cfg_lock;
+	raw_spinlock_t		cfg_lock;
 	void __iomem		*cfgbar;
 
 	int msix_count;
@@ -402,7 +402,7 @@ static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg,
 	if (!addr)
 		return -EFAULT;
 
-	spin_lock_irqsave(&vmd->cfg_lock, flags);
+	raw_spin_lock_irqsave(&vmd->cfg_lock, flags);
 	switch (len) {
 	case 1:
 		*value = readb(addr);
@@ -417,7 +417,7 @@ static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg,
 		ret = -EINVAL;
 		break;
 	}
-	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
+	raw_spin_unlock_irqrestore(&vmd->cfg_lock, flags);
 	return ret;
 }
 
@@ -437,7 +437,7 @@ static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
 	if (!addr)
 		return -EFAULT;
 
-	spin_lock_irqsave(&vmd->cfg_lock, flags);
+	raw_spin_lock_irqsave(&vmd->cfg_lock, flags);
 	switch (len) {
 	case 1:
 		writeb(value, addr);
@@ -455,7 +455,7 @@ static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
 		ret = -EINVAL;
 		break;
 	}
-	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
+	raw_spin_unlock_irqrestore(&vmd->cfg_lock, flags);
 	return ret;
 }
 
@@ -1000,7 +1000,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (features & VMD_FEAT_OFFSET_FIRST_VECTOR)
 		vmd->first_vec = 1;
 
-	spin_lock_init(&vmd->cfg_lock);
+	raw_spin_lock_init(&vmd->cfg_lock);
 	pci_set_drvdata(dev, vmd);
 	err = vmd_enable_domain(vmd, features);
 	if (err)

Re: [PATCH v6.4-rc5-rt4] rt: vmd: make cfg_lock a raw spinlock

[Steven Rostedt]

On Thu, 8 Jun 2023 21:20:04 -0300
"Luis Claudio R. Goncalves" <lgoncalv@redhat.com> wrote:

> Sebastian, Steven,
> 
> I don't know if this is the best solution for the problem, but surely the
> simplest one. If this patch makes sense, it could probably go upstream as
> the changes only affect RT.
> 
> -------
> 
> rt: vmd: make cfg_lock a raw spinlock
> 
> This call sequence triggers the backtrace listed below:
> 
>     pci_user_read_config_dword()  --->  vmd_pci_read()

Since pci_user_read_config_dword() (and all other sizes) uses
raw_spin_lock(), I guess there's really no other choice than to have
vmd_pci_read() do the same.

-- Steve


> 
> The first function, pci_user_read_config_dword(), takes a raw_spin_lock
> (a classic spin lock) and disables preemption and interrupts. Then
> vmd_pci_read() takes a RT spinlock (a rtmutex), which can sleep, hence
> the complaint about a sleeping function being called from atomic context.
> 
> 
> [  168.466387] LTP: starting read_all_proc (read_all -d /proc -q -r 3)
> [  168.614389] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> [  168.614393] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 10268, name: read_all
> [  168.614395] preempt_count: 1, expected: 0
> [  168.614396] RCU nest depth: 0, expected: 0
> [  168.614397] INFO: lockdep is turned off.
> [  168.614398] irq event stamp: 0
> [  168.614398] hardirqs last  enabled at (0): [<0000000000000000>] 0x0
> [  168.614402] hardirqs last disabled at (0): [<ffffffffa4b0f3bf>] copy_process+0x80f/0x1990
> [  168.614411] softirqs last  enabled at (0): [<ffffffffa4b0f3bf>] copy_process+0x80f/0x1990
> [  168.614414] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [  168.614415] Preemption disabled at:
> [  168.614416] [<0000000000000000>] 0x0
> [  168.614418] CPU: 19 PID: 10268 Comm: read_all Tainted: G        W          6.4.0-rc5-rt4 #2
> [  168.614421] Hardware name: Dell Inc. Precision 7820 Tower/0804P1, BIOS 2.6.3 05/04/2020
> [  168.614422] Call Trace:
> [  168.614424]  <TASK>
> [  168.614425]  dump_stack_lvl+0x47/0x80
> [  168.614433]  __might_resched+0x19b/0x250
> [  168.614441]  rt_spin_lock+0x4c/0x100
> [  168.614446]  ? vmd_pci_read+0x85/0xf0
> [  168.614453]  vmd_pci_read+0x85/0xf0
> [  168.614456]  pci_user_read_config_dword+0x78/0x100

Re: [PATCH v6.4-rc5-rt4] rt: vmd: make cfg_lock a raw spinlock

[Sebastian Andrzej Siewior]

On 2023-06-08 21:20:04 [-0300], Luis Claudio R. Goncalves wrote:
> Sebastian, Steven,
> 
> I don't know if this is the best solution for the problem, but surely the
> simplest one. If this patch makes sense, it could probably go upstream as
> the changes only affect RT.

Lockdep should complain, too because the locking order here is
raw_spinlock_t -> spinlock_t. 

Instead of making the lock raw, I would argue to remove the lock an safe
a few cycles. The lock is only used by vmd_pci_read() and
vmd_pci_write(). Both functions are invoked by pci_bus_read_config_*()
or pci_bus_write_config_*() and both acquire pci_lock first. Therefore
the lock does not add any benefit because vmd_pci_read() vs
vmd_pci_write() is already serialized.

If you make the patch feel free to reuse the wording above.

From a quick look,
   drivers/pci/controller/pci-tegra.c
   drivers/pci/controller/pcie-rcar-host.c

could use some care ;)

Sebastian