* [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
@ 2023-10-12 9:47 ` Junhao He
0 siblings, 0 replies; 24+ messages in thread
From: Junhao He @ 2023-10-12 9:47 UTC (permalink / raw)
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3
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.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index e9a32a97fbee..b08a619d1116 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
struct smb_drv_data, miscdev);
int ret = 0;
- mutex_lock(&drvdata->mutex);
+ spin_lock(&drvdata->spinlock);
if (drvdata->reading) {
ret = -EBUSY;
@@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
drvdata->reading = true;
out:
- mutex_unlock(&drvdata->mutex);
+ spin_unlock(&drvdata->spinlock);
return ret;
}
@@ -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;
-
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;
}
@@ -167,9 +163,9 @@ static int smb_release(struct inode *inode, struct file *file)
struct smb_drv_data *drvdata = container_of(file->private_data,
struct smb_drv_data, miscdev);
- mutex_lock(&drvdata->mutex);
+ spin_lock(&drvdata->spinlock);
drvdata->reading = false;
- mutex_unlock(&drvdata->mutex);
+ spin_unlock(&drvdata->spinlock);
return 0;
}
@@ -262,7 +258,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
int ret = 0;
- mutex_lock(&drvdata->mutex);
+ spin_lock(&drvdata->spinlock);
/* Do nothing, the trace data is reading by other interface now */
if (drvdata->reading) {
@@ -294,7 +290,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
out:
- mutex_unlock(&drvdata->mutex);
+ spin_unlock(&drvdata->spinlock);
return ret;
}
@@ -304,7 +300,7 @@ static int smb_disable(struct coresight_device *csdev)
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
int ret = 0;
- mutex_lock(&drvdata->mutex);
+ spin_lock(&drvdata->spinlock);
if (drvdata->reading) {
ret = -EBUSY;
@@ -327,7 +323,7 @@ static int smb_disable(struct coresight_device *csdev)
dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
out:
- mutex_unlock(&drvdata->mutex);
+ spin_unlock(&drvdata->spinlock);
return ret;
}
@@ -408,7 +404,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
if (!buf)
return 0;
- mutex_lock(&drvdata->mutex);
+ spin_lock(&drvdata->spinlock);
/* Don't do anything if another tracer is using this sink. */
if (atomic_read(&csdev->refcnt) != 1)
@@ -432,7 +428,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
if (!buf->snapshot && lost)
perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
out:
- mutex_unlock(&drvdata->mutex);
+ spin_unlock(&drvdata->spinlock);
return data_size;
}
@@ -590,7 +586,7 @@ static int smb_probe(struct platform_device *pdev)
return ret;
}
- mutex_init(&drvdata->mutex);
+ spin_lock_init(&drvdata->spinlock);
drvdata->pid = -1;
ret = smb_register_sink(pdev, drvdata);
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
index d2e14e8d2c8a..82a44c14a882 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.h
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
@@ -8,7 +8,7 @@
#define _ULTRASOC_SMB_H
#include <linux/miscdevice.h>
-#include <linux/mutex.h>
+#include <linux/spinlock.h>
/* Offset of SMB global registers */
#define SMB_GLB_CFG_REG 0x00
@@ -105,7 +105,7 @@ struct smb_data_buffer {
* @csdev: Component vitals needed by the framework.
* @sdb: Data buffer for SMB.
* @miscdev: Specifics to handle "/dev/xyz.smb" entry.
- * @mutex: Control data access to one at a time.
+ * @spinlock: Control data access to one at a time.
* @reading: Synchronise user space access to SMB buffer.
* @pid: Process ID of the process being monitored by the
* session that is using this component.
@@ -116,7 +116,7 @@ struct smb_drv_data {
struct coresight_device *csdev;
struct smb_data_buffer sdb;
struct miscdevice miscdev;
- struct mutex mutex;
+ spinlock_t spinlock;
bool reading;
pid_t pid;
enum cs_mode mode;
--
2.33.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
2023-10-12 9:47 ` Junhao He
@ 2023-10-19 3:05 ` Yicong Yang
-1 siblings, 0 replies; 24+ messages in thread
From: Yicong Yang @ 2023-10-19 3:05 UTC (permalink / raw)
To: Junhao He
Cc: suzuki.poulose, james.clark, yangyicong, coresight,
linux-arm-kernel, linux-kernel, linuxarm, jonathan.cameron,
prime.zeng
On 2023/10/12 17:47, Junhao He 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.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..b08a619d1116 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
> struct smb_drv_data, miscdev);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
>
> drvdata->reading = true;
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -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;
> -
> 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);
Do we still need the lock here? If we get here, we should have the exclusive
access to the file, which is protected in open(). Or any other cases?
>
> + dev_dbg(dev, "%zu bytes copied\n", to_copy);
> return to_copy;
> }
>
> @@ -167,9 +163,9 @@ static int smb_release(struct inode *inode, struct file *file)
> struct smb_drv_data *drvdata = container_of(file->private_data,
> struct smb_drv_data, miscdev);
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
> drvdata->reading = false;
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return 0;
> }
> @@ -262,7 +258,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> /* Do nothing, the trace data is reading by other interface now */
> if (drvdata->reading) {
> @@ -294,7 +290,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
>
> dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -304,7 +300,7 @@ static int smb_disable(struct coresight_device *csdev)
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -327,7 +323,7 @@ static int smb_disable(struct coresight_device *csdev)
>
> dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -408,7 +404,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> if (!buf)
> return 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> /* Don't do anything if another tracer is using this sink. */
> if (atomic_read(&csdev->refcnt) != 1)
> @@ -432,7 +428,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> if (!buf->snapshot && lost)
> perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return data_size;
> }
> @@ -590,7 +586,7 @@ static int smb_probe(struct platform_device *pdev)
> return ret;
> }
>
> - mutex_init(&drvdata->mutex);
> + spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
>
> ret = smb_register_sink(pdev, drvdata);
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> index d2e14e8d2c8a..82a44c14a882 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.h
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -8,7 +8,7 @@
> #define _ULTRASOC_SMB_H
>
> #include <linux/miscdevice.h>
> -#include <linux/mutex.h>
> +#include <linux/spinlock.h>
>
> /* Offset of SMB global registers */
> #define SMB_GLB_CFG_REG 0x00
> @@ -105,7 +105,7 @@ struct smb_data_buffer {
> * @csdev: Component vitals needed by the framework.
> * @sdb: Data buffer for SMB.
> * @miscdev: Specifics to handle "/dev/xyz.smb" entry.
> - * @mutex: Control data access to one at a time.
> + * @spinlock: Control data access to one at a time.
> * @reading: Synchronise user space access to SMB buffer.
> * @pid: Process ID of the process being monitored by the
> * session that is using this component.
> @@ -116,7 +116,7 @@ struct smb_drv_data {
> struct coresight_device *csdev;
> struct smb_data_buffer sdb;
> struct miscdevice miscdev;
> - struct mutex mutex;
> + spinlock_t spinlock;
> bool reading;
> pid_t pid;
> enum cs_mode mode;
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
@ 2023-10-19 3:05 ` Yicong Yang
0 siblings, 0 replies; 24+ messages in thread
From: Yicong Yang @ 2023-10-19 3:05 UTC (permalink / raw)
To: Junhao He
Cc: suzuki.poulose, james.clark, yangyicong, coresight,
linux-arm-kernel, linux-kernel, linuxarm, jonathan.cameron,
prime.zeng
On 2023/10/12 17:47, Junhao He 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.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..b08a619d1116 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
> struct smb_drv_data, miscdev);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
>
> drvdata->reading = true;
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -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;
> -
> 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);
Do we still need the lock here? If we get here, we should have the exclusive
access to the file, which is protected in open(). Or any other cases?
>
> + dev_dbg(dev, "%zu bytes copied\n", to_copy);
> return to_copy;
> }
>
> @@ -167,9 +163,9 @@ static int smb_release(struct inode *inode, struct file *file)
> struct smb_drv_data *drvdata = container_of(file->private_data,
> struct smb_drv_data, miscdev);
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
> drvdata->reading = false;
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return 0;
> }
> @@ -262,7 +258,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> /* Do nothing, the trace data is reading by other interface now */
> if (drvdata->reading) {
> @@ -294,7 +290,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
>
> dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -304,7 +300,7 @@ static int smb_disable(struct coresight_device *csdev)
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -327,7 +323,7 @@ static int smb_disable(struct coresight_device *csdev)
>
> dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -408,7 +404,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> if (!buf)
> return 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> /* Don't do anything if another tracer is using this sink. */
> if (atomic_read(&csdev->refcnt) != 1)
> @@ -432,7 +428,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> if (!buf->snapshot && lost)
> perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return data_size;
> }
> @@ -590,7 +586,7 @@ static int smb_probe(struct platform_device *pdev)
> return ret;
> }
>
> - mutex_init(&drvdata->mutex);
> + spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
>
> ret = smb_register_sink(pdev, drvdata);
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> index d2e14e8d2c8a..82a44c14a882 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.h
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -8,7 +8,7 @@
> #define _ULTRASOC_SMB_H
>
> #include <linux/miscdevice.h>
> -#include <linux/mutex.h>
> +#include <linux/spinlock.h>
>
> /* Offset of SMB global registers */
> #define SMB_GLB_CFG_REG 0x00
> @@ -105,7 +105,7 @@ struct smb_data_buffer {
> * @csdev: Component vitals needed by the framework.
> * @sdb: Data buffer for SMB.
> * @miscdev: Specifics to handle "/dev/xyz.smb" entry.
> - * @mutex: Control data access to one at a time.
> + * @spinlock: Control data access to one at a time.
> * @reading: Synchronise user space access to SMB buffer.
> * @pid: Process ID of the process being monitored by the
> * session that is using this component.
> @@ -116,7 +116,7 @@ struct smb_drv_data {
> struct coresight_device *csdev;
> struct smb_data_buffer sdb;
> struct miscdevice miscdev;
> - struct mutex mutex;
> + spinlock_t spinlock;
> bool reading;
> pid_t pid;
> enum cs_mode mode;
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
2023-10-19 3:05 ` Yicong Yang
@ 2023-10-19 12:45 ` hejunhao
-1 siblings, 0 replies; 24+ messages in thread
From: hejunhao @ 2023-10-19 12:45 UTC (permalink / raw)
To: Yicong Yang
Cc: suzuki.poulose, james.clark, yangyicong, coresight,
linux-arm-kernel, linux-kernel, linuxarm, jonathan.cameron,
prime.zeng
Hi Yicong,
On 2023/10/19 11:05, Yicong Yang wrote:
> On 2023/10/12 17:47, Junhao He wrote:
>
>
> 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.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..b08a619d1116 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
> struct smb_drv_data, miscdev);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
>
> drvdata->reading = true;
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -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;
> -
> 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);
> Do we still need the lock here? If we get here, we should have the exclusive
> access to the file, which is protected in open(). Or any other cases?
This is something I've also considered. If someone opens an SMB device
and reads it using multithreading, The SMB device will got out of sync.
I've seen other coresight sink drivers such as etb do not use locks in
the read function.
Maybe it's not necessary here, the buffer synchronization
is guaranteed by the user.
Best regards,
Junhao.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
@ 2023-10-19 12:45 ` hejunhao
0 siblings, 0 replies; 24+ messages in thread
From: hejunhao @ 2023-10-19 12:45 UTC (permalink / raw)
To: Yicong Yang
Cc: suzuki.poulose, james.clark, yangyicong, coresight,
linux-arm-kernel, linux-kernel, linuxarm, jonathan.cameron,
prime.zeng
Hi Yicong,
On 2023/10/19 11:05, Yicong Yang wrote:
> On 2023/10/12 17:47, Junhao He wrote:
>
>
> 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.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..b08a619d1116 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
> struct smb_drv_data, miscdev);
> int ret = 0;
>
> - mutex_lock(&drvdata->mutex);
> + spin_lock(&drvdata->spinlock);
>
> if (drvdata->reading) {
> ret = -EBUSY;
> @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
>
> drvdata->reading = true;
> out:
> - mutex_unlock(&drvdata->mutex);
> + spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -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;
> -
> 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);
> Do we still need the lock here? If we get here, we should have the exclusive
> access to the file, which is protected in open(). Or any other cases?
This is something I've also considered. If someone opens an SMB device
and reads it using multithreading, The SMB device will got out of sync.
I've seen other coresight sink drivers such as etb do not use locks in
the read function.
Maybe it's not necessary here, the buffer synchronization
is guaranteed by the user.
Best regards,
Junhao.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
2023-10-12 9:47 ` Junhao He
@ 2023-10-19 13:30 ` Jonathan Cameron
-1 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-10-19 13:30 UTC (permalink / raw)
To: Junhao He, linuxarm
Cc: suzuki.poulose, james.clark, coresight, linux-arm-kernel,
linux-kernel, jonathan.cameron, yangyicong, prime.zeng
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.
>
> 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
> @@ -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.
> 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;
> }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
@ 2023-10-19 13:30 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-10-19 13:30 UTC (permalink / raw)
To: Junhao He, linuxarm
Cc: suzuki.poulose, james.clark, coresight, linux-arm-kernel,
linux-kernel, jonathan.cameron, yangyicong, prime.zeng
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.
>
> 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
> @@ -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.
> 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;
> }
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
2023-10-19 13:30 ` Jonathan Cameron
@ 2023-10-21 7:25 ` hejunhao
-1 siblings, 0 replies; 24+ messages in thread
From: hejunhao @ 2023-10-21 7:25 UTC (permalink / raw)
To: Jonathan Cameron, linuxarm
Cc: suzuki.poulose, james.clark, coresight, linux-arm-kernel,
linux-kernel, yangyicong, prime.zeng
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;
>> }
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb
@ 2023-10-21 7:25 ` hejunhao
0 siblings, 0 replies; 24+ messages in thread
From: hejunhao @ 2023-10-21 7:25 UTC (permalink / raw)
To: Jonathan Cameron, linuxarm
Cc: suzuki.poulose, james.clark, coresight, linux-arm-kernel,
linux-kernel, yangyicong, prime.zeng
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;
>> }
>
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread