Hi Jonathan,
On 2023/10/19 21:30, Jonathan Cameron wrote:
On Thu, 12 Oct 2023 17:47:04 +0800 Junhao He hejunhao3@huawei.com wrote:
When we to enable the SMB by perf, the perf sched will call perf_ctx_lock() to close system preempt in event_function_call(). But SMB::enable_smb() use mutex to lock the critical section, which may sleep.
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580 in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 153023, name: perf preempt_count: 2, expected: 0 RCU nest depth: 0, expected: 0 INFO: lockdep is turned off. irq event stamp: 0 hardirqs last enabled at (0): [<0000000000000000>] 0x0 hardirqs last disabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48 softirqs last enabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48 softirqs last disabled at (0): [<0000000000000000>] 0x0 CPU: 2 PID: 153023 Comm: perf Kdump: loaded Tainted: G W O 6.5.0-rc4+ #1
Call trace: ... __mutex_lock+0xbc/0xa70 mutex_lock_nested+0x34/0x48 smb_update_buffer+0x58/0x360 [ultrasoc_smb] etm_event_stop+0x204/0x2d8 [coresight] etm_event_del+0x1c/0x30 [coresight] event_sched_out+0x17c/0x3b8 group_sched_out.part.0+0x5c/0x208 __perf_event_disable+0x15c/0x210 event_function+0xe0/0x230 remote_function+0xb4/0xe8 generic_exec_single+0x160/0x268 smp_call_function_single+0x20c/0x2a0 event_function_call+0x20c/0x220 _perf_event_disable+0x5c/0x90 perf_event_for_each_child+0x58/0xc0 _perf_ioctl+0x34c/0x1250 perf_ioctl+0x64/0x98 ...
Use spinlock replace mutex to control driver data access to one at a time. But the function copy_to_user() may sleep so spinlock do not to lock it.
I'd like to see a comment on why we no longer need to lock over the copy_to_user rather than simply that we can't.
Yes, I will do that.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver") Signed-off-by: Junhao He hejunhao3@huawei.com
A follow up patch could change a lot of this to use the new cleanup.h (don't want that in the fix though as will make back porting trickier.). That should let you do guard(spin_lock)(&drvdata->spinlock); and then use direct returns instead of goto complexity.
Jonathan
Thanks for sharing. I will append up a new patch to use guards to reduce gotos.
@@ -132,10 +132,8 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, if (!len) return 0;
- mutex_lock(&drvdata->mutex);
- if (!sdb->data_size)
goto out;
return 0;to_copy = min(sdb->data_size, len); @@ -145,20 +143,18 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) { dev_dbg(dev, "Failed to copy data to user\n");
to_copy = -EFAULT;goto out;
return -EFAULT;
- spin_lock(&drvdata->spinlock); *ppos += to_copy;
Unrelated white space change that shouldn't be here.
Ok, i will drop this white space
Thanks for the comments!
Best regards, Junhao.
smb_update_read_ptr(drvdata, to_copy);
- dev_dbg(dev, "%zu bytes copied\n", to_copy);
-out: if (!sdb->data_size) smb_reset_buffer(drvdata);
- mutex_unlock(&drvdata->mutex);
- spin_unlock(&drvdata->spinlock);
- dev_dbg(dev, "%zu bytes copied\n", to_copy); return to_copy; }
.