* [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
Fix the Three issues listed below and use guards to cleanup
a) Fixed the BUG of atomic-sleep
b) Fixed uninitialized before use buf_hw_base
c) Fixed use unreset SMB buffer
Changes since V2:
* Ignore the return value of smb_config_inport()
Link to V2: https://lore.kernel.org/lkml/20231021083822.18239-1-hejunhao3@huawei.com/
Changes since V1:
* Add comment for remove lock from smb_read()
* Move reset buffer to before register sink
* Remove patch "simplify the code for check to_copy valid"
* Add two new patches
Link to V1: https://lore.kernel.org/lkml/20231012094706.21565-1-hejunhao3@huawei.com/
Junhao He (4):
coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
coresight: ultrasoc-smb: Config SMB buffer before register sink
coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
coresight: ultrasoc-smb: Use guards to cleanup
drivers/hwtracing/coresight/ultrasoc-smb.c | 108 +++++++--------------
drivers/hwtracing/coresight/ultrasoc-smb.h | 6 +-
2 files changed, 38 insertions(+), 76 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
Fix the Three issues listed below and use guards to cleanup
a) Fixed the BUG of atomic-sleep
b) Fixed uninitialized before use buf_hw_base
c) Fixed use unreset SMB buffer
Changes since V2:
* Ignore the return value of smb_config_inport()
Link to V2: https://lore.kernel.org/lkml/20231021083822.18239-1-hejunhao3@huawei.com/
Changes since V1:
* Add comment for remove lock from smb_read()
* Move reset buffer to before register sink
* Remove patch "simplify the code for check to_copy valid"
* Add two new patches
Link to V1: https://lore.kernel.org/lkml/20231012094706.21565-1-hejunhao3@huawei.com/
Junhao He (4):
coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
coresight: ultrasoc-smb: Config SMB buffer before register sink
coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
coresight: ultrasoc-smb: Use guards to cleanup
drivers/hwtracing/coresight/ultrasoc-smb.c | 108 +++++++--------------
drivers/hwtracing/coresight/ultrasoc-smb.h | 6 +-
2 files changed, 38 insertions(+), 76 deletions(-)
--
2.33.0
_______________________________________________
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] 20+ messages in thread
* [PATCH v3 1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
2023-11-14 13:33 ` Junhao He
@ 2023-11-14 13:33 ` Junhao He
-1 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
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 to replace mutex to control driver data access to one at a
time. The function copy_to_user() may sleep, it cannot be in a spinlock
context, so we can't simply replace it in smb_read(). But we can ensure
that only one user gets the SMB device fd by smb_open(), so remove the
locks from smb_read() and buffer synchronization is guaranteed by the user.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 35 +++++++++-------------
drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index e9a32a97fbee..0a0fe9fcc57f 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,15 @@ 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;
}
*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);
+ dev_dbg(dev, "%zu bytes copied\n", to_copy);
return to_copy;
}
@@ -167,9 +160,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 +255,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 +287,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 +297,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 +320,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 +401,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 +425,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 +583,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] 20+ messages in thread
* [PATCH v3 1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
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 to replace mutex to control driver data access to one at a
time. The function copy_to_user() may sleep, it cannot be in a spinlock
context, so we can't simply replace it in smb_read(). But we can ensure
that only one user gets the SMB device fd by smb_open(), so remove the
locks from smb_read() and buffer synchronization is guaranteed by the user.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 35 +++++++++-------------
drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index e9a32a97fbee..0a0fe9fcc57f 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,15 @@ 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;
}
*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);
+ dev_dbg(dev, "%zu bytes copied\n", to_copy);
return to_copy;
}
@@ -167,9 +160,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 +255,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 +287,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 +297,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 +320,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 +401,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 +425,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 +583,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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
2023-11-14 13:33 ` Junhao He
@ 2023-11-14 13:33 ` Junhao He
-1 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
The SMB dirver register the enable/disable sysfs interface in function
smb_register_sink(), however the buffer depends on the following
configuration to work well. So it'll be possible for user to access an
unreset one.
Move the config buffer operation to before register_sink().
Ignore the return value, if smb_config_inport() fails. That will
cause the hardwares disable trace path to fail, should not affect
SMB driver remove. So we make smb_remove() return success,
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 0a0fe9fcc57f..2f2aba90a514 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
return ret;
}
+ ret = smb_config_inport(dev, true);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, drvdata);
spin_lock_init(&drvdata->spinlock);
drvdata->pid = -1;
ret = smb_register_sink(pdev, drvdata);
if (ret) {
+ smb_config_inport(&pdev->dev, false);
dev_err(dev, "Failed to register SMB sink\n");
return ret;
}
- ret = smb_config_inport(dev, true);
- if (ret) {
- smb_unregister_sink(drvdata);
- return ret;
- }
-
- platform_set_drvdata(pdev, drvdata);
-
return 0;
}
static int smb_remove(struct platform_device *pdev)
{
struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
- int ret;
-
- ret = smb_config_inport(&pdev->dev, false);
- if (ret)
- return ret;
smb_unregister_sink(drvdata);
+ smb_config_inport(&pdev->dev, false);
+
return 0;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
The SMB dirver register the enable/disable sysfs interface in function
smb_register_sink(), however the buffer depends on the following
configuration to work well. So it'll be possible for user to access an
unreset one.
Move the config buffer operation to before register_sink().
Ignore the return value, if smb_config_inport() fails. That will
cause the hardwares disable trace path to fail, should not affect
SMB driver remove. So we make smb_remove() return success,
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 0a0fe9fcc57f..2f2aba90a514 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
return ret;
}
+ ret = smb_config_inport(dev, true);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, drvdata);
spin_lock_init(&drvdata->spinlock);
drvdata->pid = -1;
ret = smb_register_sink(pdev, drvdata);
if (ret) {
+ smb_config_inport(&pdev->dev, false);
dev_err(dev, "Failed to register SMB sink\n");
return ret;
}
- ret = smb_config_inport(dev, true);
- if (ret) {
- smb_unregister_sink(drvdata);
- return ret;
- }
-
- platform_set_drvdata(pdev, drvdata);
-
return 0;
}
static int smb_remove(struct platform_device *pdev)
{
struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
- int ret;
-
- ret = smb_config_inport(&pdev->dev, false);
- if (ret)
- return ret;
smb_unregister_sink(drvdata);
+ smb_config_inport(&pdev->dev, false);
+
return 0;
}
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
2023-11-14 13:33 ` Junhao He
@ 2023-11-14 13:33 ` Junhao He
-1 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
In smb_reset_buffer, the sdb->buf_hw_base variable is uninitialized
before use, which initializes it in smb_init_data_buffer. And the SMB
regiester are set in smb_config_inport.
So move the call after smb_config_inport.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 2f2aba90a514..6e32d31a95fe 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -477,7 +477,6 @@ static int smb_init_data_buffer(struct platform_device *pdev,
static void smb_init_hw(struct smb_drv_data *drvdata)
{
smb_disable_hw(drvdata);
- smb_reset_buffer(drvdata);
writel(SMB_LB_CFG_LO_DEFAULT, drvdata->base + SMB_LB_CFG_LO_REG);
writel(SMB_LB_CFG_HI_DEFAULT, drvdata->base + SMB_LB_CFG_HI_REG);
@@ -587,6 +586,7 @@ static int smb_probe(struct platform_device *pdev)
if (ret)
return ret;
+ smb_reset_buffer(drvdata);
platform_set_drvdata(pdev, drvdata);
spin_lock_init(&drvdata->spinlock);
drvdata->pid = -1;
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
In smb_reset_buffer, the sdb->buf_hw_base variable is uninitialized
before use, which initializes it in smb_init_data_buffer. And the SMB
regiester are set in smb_config_inport.
So move the call after smb_config_inport.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 2f2aba90a514..6e32d31a95fe 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -477,7 +477,6 @@ static int smb_init_data_buffer(struct platform_device *pdev,
static void smb_init_hw(struct smb_drv_data *drvdata)
{
smb_disable_hw(drvdata);
- smb_reset_buffer(drvdata);
writel(SMB_LB_CFG_LO_DEFAULT, drvdata->base + SMB_LB_CFG_LO_REG);
writel(SMB_LB_CFG_HI_DEFAULT, drvdata->base + SMB_LB_CFG_HI_REG);
@@ -587,6 +586,7 @@ static int smb_probe(struct platform_device *pdev)
if (ret)
return ret;
+ smb_reset_buffer(drvdata);
platform_set_drvdata(pdev, drvdata);
spin_lock_init(&drvdata->spinlock);
drvdata->pid = -1;
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 4/4] coresight: ultrasoc-smb: Use guards to cleanup
2023-11-14 13:33 ` Junhao He
@ 2023-11-14 13:33 ` Junhao He
-1 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
Use guards to reduce gotos and simplify control flow.
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 70 +++++++---------------
1 file changed, 22 insertions(+), 48 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 6e32d31a95fe..cd14b2eded4e 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -97,27 +97,19 @@ static int smb_open(struct inode *inode, struct file *file)
{
struct smb_drv_data *drvdata = container_of(file->private_data,
struct smb_drv_data, miscdev);
- int ret = 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
- if (atomic_read(&drvdata->csdev->refcnt)) {
- ret = -EBUSY;
- goto out;
- }
+ if (atomic_read(&drvdata->csdev->refcnt))
+ return -EBUSY;
smb_update_data_size(drvdata);
-
drvdata->reading = true;
-out:
- spin_unlock(&drvdata->spinlock);
- return ret;
+ return 0;
}
static ssize_t smb_read(struct file *file, char __user *data, size_t len,
@@ -160,9 +152,8 @@ 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);
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
drvdata->reading = false;
- spin_unlock(&drvdata->spinlock);
return 0;
}
@@ -255,19 +246,15 @@ 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;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
/* Do nothing, the trace data is reading by other interface now */
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
/* Do nothing, the SMB is already enabled as other mode */
- if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
+ return -EBUSY;
switch (mode) {
case CS_MODE_SYSFS:
@@ -281,13 +268,10 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
}
if (ret)
- goto out;
+ return ret;
atomic_inc(&csdev->refcnt);
-
dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
-out:
- spin_unlock(&drvdata->spinlock);
return ret;
}
@@ -295,19 +279,14 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
static int smb_disable(struct coresight_device *csdev)
{
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
- int ret = 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
- if (atomic_dec_return(&csdev->refcnt)) {
- ret = -EBUSY;
- goto out;
- }
+ if (atomic_dec_return(&csdev->refcnt))
+ return -EBUSY;
/* Complain if we (somehow) got out of sync */
WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
@@ -317,12 +296,9 @@ static int smb_disable(struct coresight_device *csdev)
/* Dissociate from the target process. */
drvdata->pid = -1;
drvdata->mode = CS_MODE_DISABLED;
-
dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
-out:
- spin_unlock(&drvdata->spinlock);
- return ret;
+ return 0;
}
static void *smb_alloc_buffer(struct coresight_device *csdev,
@@ -395,17 +371,17 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
struct smb_data_buffer *sdb = &drvdata->sdb;
struct cs_buffers *buf = sink_config;
- unsigned long data_size = 0;
+ unsigned long data_size;
bool lost = false;
if (!buf)
return 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
/* Don't do anything if another tracer is using this sink. */
if (atomic_read(&csdev->refcnt) != 1)
- goto out;
+ return 0;
smb_disable_hw(drvdata);
smb_update_data_size(drvdata);
@@ -424,8 +400,6 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
smb_sync_perf_buffer(drvdata, buf, handle->head);
if (!buf->snapshot && lost)
perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
-out:
- spin_unlock(&drvdata->spinlock);
return data_size;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 4/4] coresight: ultrasoc-smb: Use guards to cleanup
@ 2023-11-14 13:33 ` Junhao He
0 siblings, 0 replies; 20+ messages in thread
From: Junhao He @ 2023-11-14 13:33 UTC (permalink / raw
To: suzuki.poulose, james.clark
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, hejunhao3,
u.kleine-koenig
Use guards to reduce gotos and simplify control flow.
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
drivers/hwtracing/coresight/ultrasoc-smb.c | 70 +++++++---------------
1 file changed, 22 insertions(+), 48 deletions(-)
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 6e32d31a95fe..cd14b2eded4e 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -97,27 +97,19 @@ static int smb_open(struct inode *inode, struct file *file)
{
struct smb_drv_data *drvdata = container_of(file->private_data,
struct smb_drv_data, miscdev);
- int ret = 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
- if (atomic_read(&drvdata->csdev->refcnt)) {
- ret = -EBUSY;
- goto out;
- }
+ if (atomic_read(&drvdata->csdev->refcnt))
+ return -EBUSY;
smb_update_data_size(drvdata);
-
drvdata->reading = true;
-out:
- spin_unlock(&drvdata->spinlock);
- return ret;
+ return 0;
}
static ssize_t smb_read(struct file *file, char __user *data, size_t len,
@@ -160,9 +152,8 @@ 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);
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
drvdata->reading = false;
- spin_unlock(&drvdata->spinlock);
return 0;
}
@@ -255,19 +246,15 @@ 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;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
/* Do nothing, the trace data is reading by other interface now */
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
/* Do nothing, the SMB is already enabled as other mode */
- if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
+ return -EBUSY;
switch (mode) {
case CS_MODE_SYSFS:
@@ -281,13 +268,10 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
}
if (ret)
- goto out;
+ return ret;
atomic_inc(&csdev->refcnt);
-
dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
-out:
- spin_unlock(&drvdata->spinlock);
return ret;
}
@@ -295,19 +279,14 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
static int smb_disable(struct coresight_device *csdev)
{
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
- int ret = 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
- if (drvdata->reading) {
- ret = -EBUSY;
- goto out;
- }
+ if (drvdata->reading)
+ return -EBUSY;
- if (atomic_dec_return(&csdev->refcnt)) {
- ret = -EBUSY;
- goto out;
- }
+ if (atomic_dec_return(&csdev->refcnt))
+ return -EBUSY;
/* Complain if we (somehow) got out of sync */
WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
@@ -317,12 +296,9 @@ static int smb_disable(struct coresight_device *csdev)
/* Dissociate from the target process. */
drvdata->pid = -1;
drvdata->mode = CS_MODE_DISABLED;
-
dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
-out:
- spin_unlock(&drvdata->spinlock);
- return ret;
+ return 0;
}
static void *smb_alloc_buffer(struct coresight_device *csdev,
@@ -395,17 +371,17 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
struct smb_data_buffer *sdb = &drvdata->sdb;
struct cs_buffers *buf = sink_config;
- unsigned long data_size = 0;
+ unsigned long data_size;
bool lost = false;
if (!buf)
return 0;
- spin_lock(&drvdata->spinlock);
+ guard(spinlock)(&drvdata->spinlock);
/* Don't do anything if another tracer is using this sink. */
if (atomic_read(&csdev->refcnt) != 1)
- goto out;
+ return 0;
smb_disable_hw(drvdata);
smb_update_data_size(drvdata);
@@ -424,8 +400,6 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
smb_sync_perf_buffer(drvdata, buf, handle->head);
if (!buf->snapshot && lost)
perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
-out:
- spin_unlock(&drvdata->spinlock);
return data_size;
}
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
2023-11-14 13:33 ` Junhao He
@ 2023-11-15 16:39 ` James Clark
-1 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:39 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, 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 to replace mutex to control driver data access to one at a
> time. The function copy_to_user() may sleep, it cannot be in a spinlock
> context, so we can't simply replace it in smb_read(). But we can ensure
> that only one user gets the SMB device fd by smb_open(), so remove the
> locks from smb_read() and buffer synchronization is guaranteed by the user.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
Reviewed-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 35 +++++++++-------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..0a0fe9fcc57f 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,15 @@ 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;
> }
>
> *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);
>
> + dev_dbg(dev, "%zu bytes copied\n", to_copy);
> return to_copy;
> }
>
> @@ -167,9 +160,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 +255,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 +287,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 +297,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 +320,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 +401,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 +425,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 +583,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] 20+ messages in thread
* Re: [PATCH v3 1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
@ 2023-11-15 16:39 ` James Clark
0 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:39 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, 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 to replace mutex to control driver data access to one at a
> time. The function copy_to_user() may sleep, it cannot be in a spinlock
> context, so we can't simply replace it in smb_read(). But we can ensure
> that only one user gets the SMB device fd by smb_open(), so remove the
> locks from smb_read() and buffer synchronization is guaranteed by the user.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
Reviewed-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 35 +++++++++-------------
> drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++--
> 2 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..0a0fe9fcc57f 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,15 @@ 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;
> }
>
> *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);
>
> + dev_dbg(dev, "%zu bytes copied\n", to_copy);
> return to_copy;
> }
>
> @@ -167,9 +160,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 +255,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 +287,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 +297,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 +320,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 +401,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 +425,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 +583,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] 20+ messages in thread
* Re: [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
2023-11-14 13:33 ` Junhao He
@ 2023-11-15 16:43 ` James Clark
-1 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:43 UTC (permalink / raw
To: Junhao He, suzuki.poulose, Uwe Kleine-König
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng
On 14/11/2023 13:33, Junhao He wrote:
> The SMB dirver register the enable/disable sysfs interface in function
> smb_register_sink(), however the buffer depends on the following
> configuration to work well. So it'll be possible for user to access an
> unreset one.
>
> Move the config buffer operation to before register_sink().
> Ignore the return value, if smb_config_inport() fails. That will
> cause the hardwares disable trace path to fail, should not affect
> SMB driver remove. So we make smb_remove() return success,
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 0a0fe9fcc57f..2f2aba90a514 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ret = smb_config_inport(dev, true);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, drvdata);
> spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
>
> ret = smb_register_sink(pdev, drvdata);
> if (ret) {
> + smb_config_inport(&pdev->dev, false);
> dev_err(dev, "Failed to register SMB sink\n");
> return ret;
> }
>
> - ret = smb_config_inport(dev, true);
> - if (ret) {
> - smb_unregister_sink(drvdata);
> - return ret;
> - }
> -
> - platform_set_drvdata(pdev, drvdata);
> -
> return 0;
> }
>
> static int smb_remove(struct platform_device *pdev)
> {
> struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
> - int ret;
> -
> - ret = smb_config_inport(&pdev->dev, false);
> - if (ret)
> - return ret;
>
> smb_unregister_sink(drvdata);
>
> + smb_config_inport(&pdev->dev, false);
> +
> return 0;
> }
>
I'm not sure if it makes sense to implement Uwe's change to use
.remove_new instead here? And then drop the change on the other
patchset. Otherwise Suzuki will have to resolve the conflict. Maybe it's
quite simple so it's not an issue.
Either way:
Reviewed-by: James Clark <james.clark@arm.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
@ 2023-11-15 16:43 ` James Clark
0 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:43 UTC (permalink / raw
To: Junhao He, suzuki.poulose, Uwe Kleine-König
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng
On 14/11/2023 13:33, Junhao He wrote:
> The SMB dirver register the enable/disable sysfs interface in function
> smb_register_sink(), however the buffer depends on the following
> configuration to work well. So it'll be possible for user to access an
> unreset one.
>
> Move the config buffer operation to before register_sink().
> Ignore the return value, if smb_config_inport() fails. That will
> cause the hardwares disable trace path to fail, should not affect
> SMB driver remove. So we make smb_remove() return success,
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 0a0fe9fcc57f..2f2aba90a514 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ret = smb_config_inport(dev, true);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, drvdata);
> spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
>
> ret = smb_register_sink(pdev, drvdata);
> if (ret) {
> + smb_config_inport(&pdev->dev, false);
> dev_err(dev, "Failed to register SMB sink\n");
> return ret;
> }
>
> - ret = smb_config_inport(dev, true);
> - if (ret) {
> - smb_unregister_sink(drvdata);
> - return ret;
> - }
> -
> - platform_set_drvdata(pdev, drvdata);
> -
> return 0;
> }
>
> static int smb_remove(struct platform_device *pdev)
> {
> struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
> - int ret;
> -
> - ret = smb_config_inport(&pdev->dev, false);
> - if (ret)
> - return ret;
>
> smb_unregister_sink(drvdata);
>
> + smb_config_inport(&pdev->dev, false);
> +
> return 0;
> }
>
I'm not sure if it makes sense to implement Uwe's change to use
.remove_new instead here? And then drop the change on the other
patchset. Otherwise Suzuki will have to resolve the conflict. Maybe it's
quite simple so it's not an issue.
Either way:
Reviewed-by: James Clark <james.clark@arm.com>
_______________________________________________
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] 20+ messages in thread
* Re: [PATCH v3 3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
2023-11-14 13:33 ` Junhao He
@ 2023-11-15 16:47 ` James Clark
-1 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:47 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, Junhao He wrote:
> In smb_reset_buffer, the sdb->buf_hw_base variable is uninitialized
> before use, which initializes it in smb_init_data_buffer. And the SMB
> regiester are set in smb_config_inport.
> So move the call after smb_config_inport.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 2f2aba90a514..6e32d31a95fe 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -477,7 +477,6 @@ static int smb_init_data_buffer(struct platform_device *pdev,
> static void smb_init_hw(struct smb_drv_data *drvdata)
> {
> smb_disable_hw(drvdata);
> - smb_reset_buffer(drvdata);
>
> writel(SMB_LB_CFG_LO_DEFAULT, drvdata->base + SMB_LB_CFG_LO_REG);
> writel(SMB_LB_CFG_HI_DEFAULT, drvdata->base + SMB_LB_CFG_HI_REG);
> @@ -587,6 +586,7 @@ static int smb_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + smb_reset_buffer(drvdata);
> platform_set_drvdata(pdev, drvdata);
> spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
Reviewed-by: James Clark <james.clark@arm.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
@ 2023-11-15 16:47 ` James Clark
0 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:47 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, Junhao He wrote:
> In smb_reset_buffer, the sdb->buf_hw_base variable is uninitialized
> before use, which initializes it in smb_init_data_buffer. And the SMB
> regiester are set in smb_config_inport.
> So move the call after smb_config_inport.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 2f2aba90a514..6e32d31a95fe 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -477,7 +477,6 @@ static int smb_init_data_buffer(struct platform_device *pdev,
> static void smb_init_hw(struct smb_drv_data *drvdata)
> {
> smb_disable_hw(drvdata);
> - smb_reset_buffer(drvdata);
>
> writel(SMB_LB_CFG_LO_DEFAULT, drvdata->base + SMB_LB_CFG_LO_REG);
> writel(SMB_LB_CFG_HI_DEFAULT, drvdata->base + SMB_LB_CFG_HI_REG);
> @@ -587,6 +586,7 @@ static int smb_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + smb_reset_buffer(drvdata);
> platform_set_drvdata(pdev, drvdata);
> spin_lock_init(&drvdata->spinlock);
> drvdata->pid = -1;
Reviewed-by: James Clark <james.clark@arm.com>
_______________________________________________
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] 20+ messages in thread
* Re: [PATCH v3 4/4] coresight: ultrasoc-smb: Use guards to cleanup
2023-11-14 13:33 ` Junhao He
@ 2023-11-15 16:51 ` James Clark
-1 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:51 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, Junhao He wrote:
> Use guards to reduce gotos and simplify control flow.
>
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
Nice cleanup!
Reviewed-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 70 +++++++---------------
> 1 file changed, 22 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 6e32d31a95fe..cd14b2eded4e 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -97,27 +97,19 @@ static int smb_open(struct inode *inode, struct file *file)
> {
> struct smb_drv_data *drvdata = container_of(file->private_data,
> struct smb_drv_data, miscdev);
> - int ret = 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> - if (atomic_read(&drvdata->csdev->refcnt)) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (atomic_read(&drvdata->csdev->refcnt))
> + return -EBUSY;
>
> smb_update_data_size(drvdata);
> -
> drvdata->reading = true;
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> - return ret;
> + return 0;
> }
>
> static ssize_t smb_read(struct file *file, char __user *data, size_t len,
> @@ -160,9 +152,8 @@ 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);
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
> drvdata->reading = false;
> - spin_unlock(&drvdata->spinlock);
>
> return 0;
> }
> @@ -255,19 +246,15 @@ 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;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> /* Do nothing, the trace data is reading by other interface now */
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> /* Do nothing, the SMB is already enabled as other mode */
> - if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
> + return -EBUSY;
>
> switch (mode) {
> case CS_MODE_SYSFS:
> @@ -281,13 +268,10 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> }
>
> if (ret)
> - goto out;
> + return ret;
>
> atomic_inc(&csdev->refcnt);
> -
> dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -295,19 +279,14 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> static int smb_disable(struct coresight_device *csdev)
> {
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> - int ret = 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> - if (atomic_dec_return(&csdev->refcnt)) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (atomic_dec_return(&csdev->refcnt))
> + return -EBUSY;
>
> /* Complain if we (somehow) got out of sync */
> WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> @@ -317,12 +296,9 @@ static int smb_disable(struct coresight_device *csdev)
> /* Dissociate from the target process. */
> drvdata->pid = -1;
> drvdata->mode = CS_MODE_DISABLED;
> -
> dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> - return ret;
> + return 0;
> }
>
> static void *smb_alloc_buffer(struct coresight_device *csdev,
> @@ -395,17 +371,17 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> struct smb_data_buffer *sdb = &drvdata->sdb;
> struct cs_buffers *buf = sink_config;
> - unsigned long data_size = 0;
> + unsigned long data_size;
> bool lost = false;
>
> if (!buf)
> return 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> /* Don't do anything if another tracer is using this sink. */
> if (atomic_read(&csdev->refcnt) != 1)
> - goto out;
> + return 0;
>
> smb_disable_hw(drvdata);
> smb_update_data_size(drvdata);
> @@ -424,8 +400,6 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> smb_sync_perf_buffer(drvdata, buf, handle->head);
> if (!buf->snapshot && lost)
> perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> return data_size;
> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 4/4] coresight: ultrasoc-smb: Use guards to cleanup
@ 2023-11-15 16:51 ` James Clark
0 siblings, 0 replies; 20+ messages in thread
From: James Clark @ 2023-11-15 16:51 UTC (permalink / raw
To: Junhao He, suzuki.poulose
Cc: coresight, linux-arm-kernel, linux-kernel, linuxarm,
jonathan.cameron, yangyicong, prime.zeng, u.kleine-koenig
On 14/11/2023 13:33, Junhao He wrote:
> Use guards to reduce gotos and simplify control flow.
>
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
Nice cleanup!
Reviewed-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/ultrasoc-smb.c | 70 +++++++---------------
> 1 file changed, 22 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 6e32d31a95fe..cd14b2eded4e 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -97,27 +97,19 @@ static int smb_open(struct inode *inode, struct file *file)
> {
> struct smb_drv_data *drvdata = container_of(file->private_data,
> struct smb_drv_data, miscdev);
> - int ret = 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> - if (atomic_read(&drvdata->csdev->refcnt)) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (atomic_read(&drvdata->csdev->refcnt))
> + return -EBUSY;
>
> smb_update_data_size(drvdata);
> -
> drvdata->reading = true;
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> - return ret;
> + return 0;
> }
>
> static ssize_t smb_read(struct file *file, char __user *data, size_t len,
> @@ -160,9 +152,8 @@ 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);
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
> drvdata->reading = false;
> - spin_unlock(&drvdata->spinlock);
>
> return 0;
> }
> @@ -255,19 +246,15 @@ 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;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> /* Do nothing, the trace data is reading by other interface now */
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> /* Do nothing, the SMB is already enabled as other mode */
> - if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
> + return -EBUSY;
>
> switch (mode) {
> case CS_MODE_SYSFS:
> @@ -281,13 +268,10 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> }
>
> if (ret)
> - goto out;
> + return ret;
>
> atomic_inc(&csdev->refcnt);
> -
> dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> return ret;
> }
> @@ -295,19 +279,14 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> static int smb_disable(struct coresight_device *csdev)
> {
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> - int ret = 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> - if (drvdata->reading) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (drvdata->reading)
> + return -EBUSY;
>
> - if (atomic_dec_return(&csdev->refcnt)) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (atomic_dec_return(&csdev->refcnt))
> + return -EBUSY;
>
> /* Complain if we (somehow) got out of sync */
> WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> @@ -317,12 +296,9 @@ static int smb_disable(struct coresight_device *csdev)
> /* Dissociate from the target process. */
> drvdata->pid = -1;
> drvdata->mode = CS_MODE_DISABLED;
> -
> dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> - return ret;
> + return 0;
> }
>
> static void *smb_alloc_buffer(struct coresight_device *csdev,
> @@ -395,17 +371,17 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> struct smb_data_buffer *sdb = &drvdata->sdb;
> struct cs_buffers *buf = sink_config;
> - unsigned long data_size = 0;
> + unsigned long data_size;
> bool lost = false;
>
> if (!buf)
> return 0;
>
> - spin_lock(&drvdata->spinlock);
> + guard(spinlock)(&drvdata->spinlock);
>
> /* Don't do anything if another tracer is using this sink. */
> if (atomic_read(&csdev->refcnt) != 1)
> - goto out;
> + return 0;
>
> smb_disable_hw(drvdata);
> smb_update_data_size(drvdata);
> @@ -424,8 +400,6 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> smb_sync_perf_buffer(drvdata, buf, handle->head);
> if (!buf->snapshot && lost)
> perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> -out:
> - spin_unlock(&drvdata->spinlock);
>
> return data_size;
> }
_______________________________________________
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] 20+ messages in thread
* Re: [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb
2023-11-14 13:33 ` Junhao He
@ 2023-11-21 11:34 ` Suzuki K Poulose
-1 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2023-11-21 11:34 UTC (permalink / raw
To: Junhao He, james.clark
Cc: Suzuki K Poulose, coresight, prime.zeng, jonathan.cameron,
u.kleine-koenig, yangyicong, linux-arm-kernel, linux-kernel,
linuxarm
On Tue, 14 Nov 2023 21:33:42 +0800, Junhao He wrote:
> Fix the Three issues listed below and use guards to cleanup
> a) Fixed the BUG of atomic-sleep
> b) Fixed uninitialized before use buf_hw_base
> c) Fixed use unreset SMB buffer
>
> Changes since V2:
> * Ignore the return value of smb_config_inport()
>
> [...]
Applied, first 3 patches to fixes
[1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
https://git.kernel.org/coresight/c/b8411287aef4
[2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
https://git.kernel.org/coresight/c/830a7f54db10
[3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
https://git.kernel.org/coresight/c/862c135bde8b
And the last one to next branch
[4/4] coresight: ultrasoc-smb: Use guards to cleanup
https://git.kernel.org/coresight/c/60e5f23dc5d6
Best regards,
--
Suzuki K Poulose <suzuki.poulose@arm.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb
@ 2023-11-21 11:34 ` Suzuki K Poulose
0 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2023-11-21 11:34 UTC (permalink / raw
To: Junhao He, james.clark
Cc: Suzuki K Poulose, coresight, prime.zeng, jonathan.cameron,
u.kleine-koenig, yangyicong, linux-arm-kernel, linux-kernel,
linuxarm
On Tue, 14 Nov 2023 21:33:42 +0800, Junhao He wrote:
> Fix the Three issues listed below and use guards to cleanup
> a) Fixed the BUG of atomic-sleep
> b) Fixed uninitialized before use buf_hw_base
> c) Fixed use unreset SMB buffer
>
> Changes since V2:
> * Ignore the return value of smb_config_inport()
>
> [...]
Applied, first 3 patches to fixes
[1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb
https://git.kernel.org/coresight/c/b8411287aef4
[2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink
https://git.kernel.org/coresight/c/830a7f54db10
[3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base
https://git.kernel.org/coresight/c/862c135bde8b
And the last one to next branch
[4/4] coresight: ultrasoc-smb: Use guards to cleanup
https://git.kernel.org/coresight/c/60e5f23dc5d6
Best regards,
--
Suzuki K Poulose <suzuki.poulose@arm.com>
_______________________________________________
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] 20+ messages in thread
end of thread, other threads:[~2023-11-21 11:35 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-14 13:33 [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb Junhao He
2023-11-14 13:33 ` Junhao He
2023-11-14 13:33 ` [PATCH v3 1/4] coresight: ultrasoc-smb: Fix sleep while close preempt in enable_smb Junhao He
2023-11-14 13:33 ` Junhao He
2023-11-15 16:39 ` James Clark
2023-11-15 16:39 ` James Clark
2023-11-14 13:33 ` [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink Junhao He
2023-11-14 13:33 ` Junhao He
2023-11-15 16:43 ` James Clark
2023-11-15 16:43 ` James Clark
2023-11-14 13:33 ` [PATCH v3 3/4] coresight: ultrasoc-smb: Fix uninitialized before use buf_hw_base Junhao He
2023-11-14 13:33 ` Junhao He
2023-11-15 16:47 ` James Clark
2023-11-15 16:47 ` James Clark
2023-11-14 13:33 ` [PATCH v3 4/4] coresight: ultrasoc-smb: Use guards to cleanup Junhao He
2023-11-14 13:33 ` Junhao He
2023-11-15 16:51 ` James Clark
2023-11-15 16:51 ` James Clark
2023-11-21 11:34 ` [PATCH v3 0/4] Fixed some issues and cleanup of ultrasoc-smb Suzuki K Poulose
2023-11-21 11:34 ` Suzuki K Poulose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.