Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb

[hejunhao]

Hi Jonathan,


On 2023/10/19 21:30, Jonathan Cameron wrote:
> On Thu, 12 Oct 2023 17:47:04 +0800
> Junhao He <hejunhao3@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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;
> >   }
>
> .