Hi Krzysztof,
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org>
> Sent: Saturday, September 30, 2023 8:59 PM
> To: Linu Cherian <lcherian(a)marvell.com>; suzuki.poulose(a)arm.com;
> mike.leach(a)linaro.org; james.clark(a)arm.com; leo.yan(a)linaro.org
> Cc: linux-arm-kernel(a)lists.infradead.org; coresight(a)lists.linaro.org; linux-
> kernel(a)vger.kernel.org; robh+dt(a)kernel.org;
> krzysztof.kozlowski+dt(a)linaro.org; conor+dt(a)kernel.org;
> devicetree(a)vger.kernel.org; Sunil Kovvuri Goutham
> <sgoutham(a)marvell.com>; George Cherian <gcherian(a)marvell.com>
> Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add
> "memory-region" property
>
> External Email
>
> ----------------------------------------------------------------------
> On 29/09/2023 15:37, Linu Cherian wrote:
> > memory-region 0: Reserved trace buffer memory
> >
> > TMC ETR: When available, use this reserved memory region for
> > trace data capture. Same region is used for trace data
> > retention after a panic or watchdog reset.
> >
> > TMC ETF: When available, use this reserved memory region for
> > trace data retention synced from internal SRAM after a panic or
> > watchdog reset.
> >
> > memory-region 1: Reserved meta data memory
> >
> > TMC ETR, ETF: When available, use this memory for register
> > snapshot retention synced from hardware registers after a panic
> > or watchdog reset.
> >
> > Signed-off-by: Linu Cherian <lcherian(a)marvell.com>
> > ---
>
> Where is the changelog? This is supposed to be v4 or something later.
> Please, keep proper versioning and changelog.
Sure, will add the changelog from next version onwards.
Yeah, the last version was RFC v3 and the RFC tag has been dropped from this version onwards.
Hence started this version with V1.
>
> > .../bindings/arm/arm,coresight-tmc.yaml | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-
> tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-
> tmc.yaml
> > index cb8dceaca70e..45ca4d02d73e 100644
> > --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml
> > +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml
> > @@ -101,6 +101,22 @@ properties:
> > and ETF configurations.
> > $ref: /schemas/graph.yaml#/properties/port
> >
> > + memory-region:
> > + items:
> > + - description: Reserved trace buffer memory for ETR and ETF sinks.
> > + For ETR, this reserved memory region is used for trace data capture.
> > + Same region is used for trace data retention as well after a panic
> > + or watchdog reset.
> > + For ETF, this reserved memory region is used for retention of trace
> > + data synced from internal SRAM after a panic or watchdog reset.
> > +
> > + - description: Reserved meta data memory. Used for ETR and ETF sinks.
> > +
> > + memory-region-names:
> > + items:
> > + - const: trace-mem
> > + - const: metadata-mem
>
> Drop the 'mem' suffixes.
>
Ack. Will remove it in next version.
The commmit [1] replace "source_ops(csdev)::enable()" with
coresight_enable_source(). After this patch, when enable ETM through
perf mode, the csdev::enable will be set to true. Then if we to disable
the ETM by sysfs mode at the time, the resource will be released
incorrectly, kernel will report the following call trace.
perf record -e cs_etm/@ultra_smb0/ -C 2 sleep 30 &
echo 1 > /sys/bus/coresight/devices/ultra_smb0/enable_sink
echo 1 > /sys/bus/coresight/devices/etm0/enable_source
echo 0 > /sys/bus/coresight/devices/etm0/enable_source
Unable to handle kernel NULL pointer dereference at virtual
address 00000000000001d0
Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
...
Call trace:
etm4_disable+0x54/0x150 [coresight_etm4x]
coresight_disable_source+0x6c/0x98 [coresight]
coresight_disable+0x74/0x1c0 [coresight]
enable_source_store+0x88/0xa0 [coresight]
dev_attr_store+0x20/0x40
sysfs_kf_write+0x4c/0x68
kernfs_fop_write_iter+0x120/0x1b8
vfs_write+0x2dc/0x3b0
ksys_write+0x70/0x108
__arm64_sys_write+0x24/0x38
invoke_syscall+0x50/0x128
el0_svc_common.constprop.0+0x104/0x130
do_el0_svc+0x40/0xb8
el0_svc+0x2c/0xb8
el0t_64_sync_handler+0xc0/0xc8
el0t_64_sync+0x1a4/0x1a8
Code: d53cd042 91002000 b9402a81 b8626800 (f940ead5)
---[ end trace 0000000000000000 ]---
The csdev::enable specify the device is currently part of an active
path, when enabling source via perf mode, csdev::enable also should
be set to true. We can add mode check in etm4_disable() to fix that,
if it's done, the sysfs cannot report busy when sysfs to enable ETM
that has been enabled by perf.
Struct coresight_device add member of mode to check device busy in
coresight_(enable/disable)_source function to fixes handle kernel
NULL pointer, and sysfs report busy if perf session is already using
the ETM.
By the way, inperhaps another cleanup patch may be upload to remove
the etmv4_drvdata::mode tmc_drvdata::mode or others.
Test:
perf record -e cs_etm/@ultra_smb0/ -C 2 sleep 30 &
cat /sys/bus/coresight/devices/etm2/enable_source
1
echo 1 > /sys/bus/coresight/devices/etm2/enable_source
-bash: echo: write error: Device or resource busy
echo 0 > /sys/bus/coresight/devices/etm2/enable_source
coresight etm2: Someone is already using the tracer
cat /sys/bus/coresight/devices/etm2/enable_source
1
[1] "coresight: Enable and disable helper devices adjacent to the path"
Fixes: 6148652807ba ("coresight: Enable and disable helper devices adjacent to the path")
Signed-off-by: Junhao He <hejunhao3(a)huawei.com>
---
drivers/hwtracing/coresight/coresight-core.c | 18 ++++++++++++++++--
.../hwtracing/coresight/coresight-etm-perf.c | 6 +++++-
drivers/hwtracing/coresight/coresight-priv.h | 3 ++-
include/linux/coresight.h | 14 ++++++++------
4 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 206f17826399..7b485fc2ed19 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -388,6 +388,7 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
return ret;
}
csdev->enable = true;
+ csdev->mode = mode;
}
atomic_inc(&csdev->refcnt);
@@ -446,18 +447,26 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
* the device if there are no users left.
*
* @csdev: The coresight device to disable
+ * @mode: How the coresight device is being used, perf mode or sysfs mode.
* @data: Opaque data to pass on to the disable function of the source device.
* For example in perf mode this is a pointer to the struct perf_event.
*
* Returns true if the device has been disabled.
*/
-bool coresight_disable_source(struct coresight_device *csdev, void *data)
+bool coresight_disable_source(struct coresight_device *csdev, enum cs_mode mode,
+ void *data)
{
+ if (csdev->mode && csdev->mode != mode) {
+ dev_err(&csdev->dev, "Someone is already using the tracer\n");
+ return false;
+ }
+
if (atomic_dec_return(&csdev->refcnt) == 0) {
if (source_ops(csdev)->disable)
source_ops(csdev)->disable(csdev, data);
coresight_disable_helpers(csdev);
csdev->enable = false;
+ csdev->mode = CS_MODE_DISABLED;
}
return !csdev->enable;
}
@@ -1117,6 +1126,11 @@ int coresight_enable(struct coresight_device *csdev)
if (ret)
goto out;
+ if (csdev->mode == CS_MODE_PERF) {
+ ret = -EBUSY;
+ goto out;
+ }
+
if (csdev->enable) {
/*
* There could be multiple applications driving the software
@@ -1202,7 +1216,7 @@ void coresight_disable(struct coresight_device *csdev)
if (ret)
goto out;
- if (!csdev->enable || !coresight_disable_source(csdev, NULL))
+ if (!csdev->enable || !coresight_disable_source(csdev, CS_MODE_SYSFS, NULL))
goto out;
switch (csdev->subtype.source_subtype) {
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 5ca6278baff4..015aea9f2b22 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -459,6 +459,10 @@ static void etm_event_start(struct perf_event *event, int flags)
if (WARN_ON(ctxt->event_data))
goto fail;
+ /* Sysfs is already using the tracer */
+ if (csdev->mode == CS_MODE_SYSFS)
+ goto fail;
+
/*
* Deal with the ring buffer API and get a handle on the
* session's information.
@@ -587,7 +591,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
return;
/* stop tracer */
- coresight_disable_source(csdev, event);
+ coresight_disable_source(csdev, CS_MODE_PERF, event);
/* tell the core */
event->hw.state = PERF_HES_STOPPED;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 767076e07970..1f8b512ba5ac 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -233,6 +233,7 @@ void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
struct coresight_device *coresight_get_percpu_sink(int cpu);
int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
void *data);
-bool coresight_disable_source(struct coresight_device *csdev, void *data);
+bool coresight_disable_source(struct coresight_device *csdev, enum cs_mode mode,
+ void *data);
#endif
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index a269fffaf991..acbbedb9abf3 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -216,6 +216,12 @@ struct coresight_sysfs_link {
const char *target_name;
};
+enum cs_mode {
+ CS_MODE_DISABLED,
+ CS_MODE_SYSFS,
+ CS_MODE_PERF,
+};
+
/**
* struct coresight_device - representation of a device as used by the framework
* @pdata: Platform data with device connections associated to this device.
@@ -228,6 +234,7 @@ struct coresight_sysfs_link {
* @refcnt: keep track of what is in use.
* @orphan: true if the component has connections that haven't been linked.
* @enable: 'true' if component is currently part of an active path.
+ * @mode: This tracer's mode, i.e sysFS, Perf or disabled.
* @activated: 'true' only if a _sink_ has been activated. A sink can be
* activated but not yet enabled. Enabling for a _sink_
* happens when a source has been selected and a path is enabled
@@ -252,6 +259,7 @@ struct coresight_device {
atomic_t refcnt;
bool orphan;
bool enable; /* true only if configured as part of a path */
+ enum cs_mode mode;
/* sink specific fields */
bool activated; /* true only if a sink is part of a path */
struct dev_ext_attribute *ea;
@@ -290,12 +298,6 @@ static struct coresight_dev_list (var) = { \
#define to_coresight_device(d) container_of(d, struct coresight_device, dev)
-enum cs_mode {
- CS_MODE_DISABLED,
- CS_MODE_SYSFS,
- CS_MODE_PERF,
-};
-
#define source_ops(csdev) csdev->ops->source_ops
#define sink_ops(csdev) csdev->ops->sink_ops
#define link_ops(csdev) csdev->ops->link_ops
--
2.30.0
Hi Greg
Please find a couple of fixes for coresight subsystem targetting v6.6
Kindly pull
Suzuki
The following changes since commit 0bb80ecc33a8fb5a682236443c1e740d5c917d1d:
Linux 6.6-rc1 (2023-09-10 16:28:41 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-fixes-v6.6-1
for you to fetch changes up to e5028011885a85032aa3c1b7e3e493bcdacb4a0a:
coresight: tmc-etr: Disable warnings for allocation failures (2023-09-20 10:46:30 +0100)
----------------------------------------------------------------
coresight: Fixes for v6.6
Couple of fixes for the CoreSight self-hosted tracing
targeting v6.6. Includes :
- Fix runtime warnings while reusing the TMC-ETR buffer
- Fix (disable) warning when a large buffer is allocated in
the flat mode.
----------------------------------------------------------------
Linu Cherian (1):
coresight: Fix run time warnings while reusing ETR buffer
Suzuki K Poulose (1):
coresight: tmc-etr: Disable warnings for allocation failures
drivers/hwtracing/coresight/coresight-tmc-etr.c | 27 ++++++++++++++-----------
1 file changed, 15 insertions(+), 12 deletions(-)