On 2023/10/19 10:34, Yicong Yang wrote:
> On 2023/10/12 17:47, 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(a)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 e78edc3480ce..21ba473786e5 100644
>> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> @@ -475,7 +475,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);
>> @@ -597,6 +596,7 @@ static int smb_probe(struct platform_device *pdev)
>> }
>>
>> platform_set_drvdata(pdev, drvdata);
>> + smb_reset_buffer(drvdata);
> Shouldn't we reset the buffer before registering the sink? Otherwise it'll
> be possible for user to access an unreset one.
>
Hi Yicong,
Thanks for the comments!
The code after the smb_register_sink() function also needs to moved.
I will fix all in next version.
Best regards,
Junhao.
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(a)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.
This patch set is to introduce a new itrace option 'T' for forcily use
timestamp trace for kernel time and support this option in cs-etm.
Some Arm platforms (either Arm CPUs prior to Armv8 or miss the FEAT_TRF
feature, currently the ETM driver cannot decide if the timestamp trace
is same with kernel time. This is why we introduce the itrace option
'T', we leave decision to users so users can specify this option to
forcily use the timestamp trace as the kernel time.
This patch set is tested on Arm Juno board.
Leo Yan (2):
perf auxtrace: Add 'T' itrace option for timestamp trace
perf cs-etm: Enable itrace option 'T'
tools/perf/Documentation/itrace.txt | 1 +
tools/perf/util/auxtrace.c | 3 +++
tools/perf/util/auxtrace.h | 3 +++
tools/perf/util/cs-etm.c | 21 ++++++++++++++++++---
4 files changed, 25 insertions(+), 3 deletions(-)
--
2.34.1
On 19/10/2023 11:47, Adrian Hunter wrote:
> On 14/10/23 10:45, Leo Yan wrote:
>> An AUX trace can contain timestamp, but in some situations, the hardware
>> trace module (e.g. Arm CoreSight) cannot decide the traced timestamp is
>> the same source with CPU's time, thus the decoder can not use the
>> timestamp trace for samples.
>>
>> This patch introduces 'T' itrace option. If users know the platforms
>
> "If users know" <- how would users know? Could the kernel
> or tools also figure it out?
>
The problem is this was only made into a discoverable feature with
Feat_TRF in Armv8.4. So this workaround is to support devices that
already had the right kind of timestamps before that feature.
It might be possible to list every device in the driver, but maybe there
could even be some firmware that disconnects the clock source on a
device that we thought would have it.
IMO adding this option is the best way to workaround it.
>> they are working on have the same time counter with CPUs, users can
>> use this new option to tell a decoder for using timestamp trace as
>> kernel time.
>>
>> Signed-off-by: Leo Yan <leo.yan(a)linaro.org>
>> ---
>> tools/perf/Documentation/itrace.txt | 1 +
>> tools/perf/util/auxtrace.c | 3 +++
>> tools/perf/util/auxtrace.h | 3 +++
>> 3 files changed, 7 insertions(+)
>>
>> diff --git a/tools/perf/Documentation/itrace.txt b/tools/perf/Documentation/itrace.txt
>> index a97f95825b14..19cc179be9a7 100644
>> --- a/tools/perf/Documentation/itrace.txt
>> +++ b/tools/perf/Documentation/itrace.txt
>> @@ -25,6 +25,7 @@
>> q quicker (less detailed) decoding
>> A approximate IPC
>> Z prefer to ignore timestamps (so-called "timeless" decoding)
>> + T use the timestamp trace as kernel time
>>
>> The default is all events i.e. the same as --itrace=iybxwpe,
>> except for perf script where it is --itrace=ce
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index a0368202a746..f528c4364d23 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -1638,6 +1638,9 @@ int itrace_do_parse_synth_opts(struct itrace_synth_opts *synth_opts,
>> case 'Z':
>> synth_opts->timeless_decoding = true;
>> break;
>> + case 'T':
>> + synth_opts->use_timestamp = true;
>> + break;
>> case ' ':
>> case ',':
>> break;
>> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
>> index 29eb82dff574..55702215a82d 100644
>> --- a/tools/perf/util/auxtrace.h
>> +++ b/tools/perf/util/auxtrace.h
>> @@ -99,6 +99,7 @@ enum itrace_period_type {
>> * @remote_access: whether to synthesize remote access events
>> * @mem: whether to synthesize memory events
>> * @timeless_decoding: prefer "timeless" decoding i.e. ignore timestamps
>> + * @use_timestamp: use the timestamp trace as kernel time
>> * @vm_time_correlation: perform VM Time Correlation
>> * @vm_tm_corr_dry_run: VM Time Correlation dry-run
>> * @vm_tm_corr_args: VM Time Correlation implementation-specific arguments
>> @@ -146,6 +147,7 @@ struct itrace_synth_opts {
>> bool remote_access;
>> bool mem;
>> bool timeless_decoding;
>> + bool use_timestamp;
>> bool vm_time_correlation;
>> bool vm_tm_corr_dry_run;
>> char *vm_tm_corr_args;
>> @@ -678,6 +680,7 @@ bool auxtrace__evsel_is_auxtrace(struct perf_session *session,
>> " q: quicker (less detailed) decoding\n" \
>> " A: approximate IPC\n" \
>> " Z: prefer to ignore timestamps (so-called \"timeless\" decoding)\n" \
>> +" T: use the timestamp trace as kernel time\n" \
>> " PERIOD[ns|us|ms|i|t]: specify period to sample stream\n" \
>> " concatenate multiple options. Default is iybxwpe or cewp\n"
>>
>
>
The ETM4X driver now assume that all ETE as CPU system instructions
accessed device, in fact the ETE device on some machines also accessed
via MMIO.
Signed-off-by: Ruidong Tian <tianruidong(a)linux.alibaba.com>
---
drivers/hwtracing/coresight/coresight-etm4x-core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 285539104bcc..ad298c9cc87e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1103,8 +1103,9 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
* with MMIO. But we cannot touch the OSLK until we are
* sure this is an ETM. So rely only on the TRCDEVARCH.
*/
- if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) {
- pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n");
+ if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH &&
+ (devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETE_ARCH) {
+ pr_warn_once("TRCDEVARCH doesn't match ETMv4/ETE architecture\n");
return false;
}
--
2.33.1
Hi Conor,
> -----Original Message-----
> From: Conor Dooley <conor(a)kernel.org>
> Sent: Saturday, October 14, 2023 6:56 PM
> To: Linu Cherian <lcherian(a)marvell.com>
> Cc: Mike Leach <mike.leach(a)linaro.org>; suzuki.poulose(a)arm.com;
> james.clark(a)arm.com; leo.yan(a)linaro.org; 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: Re: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add
> "memory-region" property
>
> On Sat, Oct 14, 2023 at 11:36:37AM +0000, Linu Cherian wrote:
> > > > + - description: Reserved meta data memory. Used for ETR and ETF
> sinks.
> > > > +
> > > > + memory-region-names:
> > > > + items:
> > > > + - const: trace-mem
> > > > + - const: metadata-mem
> > > > +
> > >
> > > Is there a constraint required here? If we are using the memory area
> > > for trace in a panic situation, then we must have the meta data
> > > memory area defined?
> > > Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" ,
> > > "panic-metadata-mem", were the first is for general ETR trace in
> > > non-panic situation and then constrain the "panic-" areas to appear
> together.
> > > The "etr-trace-mem", "panic-trace-mem" could easily point to the
> > > same area.
> > >
> > As noted above, we do not have other generic use case for these reserved
> regions now.
> > Hence two regions/names, panic-trace-mem and panic-metadata-mem
> with
> > constraints kept as
> > minItems: 2 and maxItems: 2 would suffice ?
>
> Whatever you do, please delete the -mem suffix.
Ack.
On 17/10/2023 10:56, Bagas Sanjaya wrote:
> Stephen Rothwell reported htmldocs warnings when merging coresight tree:
>
> Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm:48: ERROR: Unexpected indentation.
> Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm:48: WARNING: Block quote ends without a blank line; unexpected unindent.
>
> Fix indentation alignment for Bit[3] list entry in dsb_mode description to
> silence above warnings.
>
> Fixes: 535d80d3c10fff ("coresight-tpdm: Add node to set dsb programming mode")
> Reported-by: Stephen Rothwell <sfr(a)canb.auug.org.au>
> Closes: https://lore.kernel.org/linux-next/20231017143324.75387a21@canb.auug.org.au/
> Signed-off-by: Bagas Sanjaya <bagasdotme(a)gmail.com>
> ---
> Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index f07218e788439d..4dd49b159543b6 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -54,8 +54,8 @@ Description:
> Accepts the value needs to be greater than 0. What data
> bits do is listed below.
> Bit[0:1] : Test mode control bit for choosing the inputs.
> - Bit[3] : Set to 0 for low performance mode.
> - Set to 1 for high performance mode.
> + Bit[3] : Set to 0 for low performance mode. Set to 1 for high
> + performance mode.
> Bit[4:8] : Select byte lane for high performance mode.
>
> What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge/ctrl_idx
>
> base-commit: 90a7371cb08d7e542fa4f283c881973bba09f23b
Thanks, confirmed the patch to fix the issue. I have queued this to next.
https://git.kernel.org/coresight/c/845333e5f0f3
Thanks
Suzuki
On 29/09/2023 09:16, Uwe Kleine-König wrote:
> etm4_platform_driver (which lives in ".data" contains a reference to
> etm4_remove_platform_dev(). So the latter must not be marked with __exit
> which results in the function being discarded for a build with
> CONFIG_CORESIGHT_SOURCE_ETM4X=y which in turn makes the remove pointer
> contain invalid data.
>
> etm4x_amba_driver referencing etm4_remove_amba() has the same issue.
>
> Drop the __exit annotations for the two affected functions and a third
> one that is called by the other two.
>
> For reasons I don't understand this isn't catched by building with
> CONFIG_DEBUG_SECTION_MISMATCH=y.
>
> Fixes: c23bc382ef0e ("coresight: etm4x: Refactor probing routine")
> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig(a)pengutronix.de>
Thanks for the fix, I can queue them via the coresight tree.
Suzuki
Partially revert the change in commit 6148652807ba ("coresight: Enable
and disable helper devices adjacent to the path") which changed the bare
call from source_ops(csdev)->enable() to coresight_enable_source() for
Perf sessions. It was missed that coresight_enable_source() is
specifically for the sysfs interface, rather than being a generic call.
This interferes with the sysfs reference counting to cause the following
crash:
$ perf record -e cs_etm/@tmc_etr0/ -C 0 &
$ echo 1 > /sys/bus/coresight/devices/tmc_etr0/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 ]---
This commit linked below also fixes the issue, but has unlocked updates
to the mode which could potentially race. So until we come up with a
more complete solution that takes all locking and interaction between
both modes into account, just revert back to the old behavior for Perf.
Reported-by: Junhao He <hejunhao3(a)huawei.com>
Closes: https://lore.kernel.org/linux-arm-kernel/20230921132904.60996-1-hejunhao3@h…
Fixes: 6148652807ba ("coresight: Enable and disable helper devices adjacent to the path")
Signed-off-by: James Clark <james.clark(a)arm.com>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 5ca6278baff4..89e8ed214ea4 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -493,7 +493,7 @@ static void etm_event_start(struct perf_event *event, int flags)
goto fail_end_stop;
/* Finally enable the tracer */
- if (coresight_enable_source(csdev, CS_MODE_PERF, event))
+ if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
goto fail_disable_path;
/*
@@ -587,7 +587,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
return;
/* stop tracer */
- coresight_disable_source(csdev, event);
+ source_ops(csdev)->disable(csdev, event);
/* tell the core */
event->hw.state = PERF_HES_STOPPED;
--
2.34.1
This series makes ETM TRCCCCTRL based 'cc_threshold' user configurable via
the perf event attribute. But first, this implements an errata work around
affecting ETM TRCIDR3.CCITMIN value on certain cpus, overriding the field.
This series applies on coresight/for-next/queue.
Cc: Catalin Marinas <catalin.marinas(a)arm.com>
Cc: Will Deacon <will(a)kernel.org>
Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com>
Cc: Mike Leach <mike.leach(a)linaro.org>
Cc: James Clark <james.clark(a)arm.com>
Cc: Leo Yan <leo.yan(a)linaro.org>
Cc: Jonathan Corbet <corbet(a)lwn.net>
Cc: linux-doc(a)vger.kernel.org
Cc: coresight(a)lists.linaro.org
Cc: linux-arm-kernel(a)lists.infradead.org
Cc: linux-kernel(a)vger.kernel.org
Changes in V7:
- Changed commit message for the second patch adding cc_threshold
Changes in V6:
https://lore.kernel.org/all/20230920095443.1126617-1-anshuman.khandual@arm.…
- Renamed etm4_core_reads_wrong_ccitmin() as etm4_fixup_wrong_ccitmin()
- Moved drvdata->ccitmin fixup inside etm4_fixup_wrong_ccitmin()
Changes in V5:
https://lore.kernel.org/all/20230821045216.641499-1-anshuman.khandual@arm.c…https://lore.kernel.org/all/20230915093649.435163-1-anshuman.khandual@arm.c…
- Replaced 'where as' with single word 'whereas'
- Reworked 'cc_threshold' fallback to ETM_CYC_THRESHOLD_DEFAULT
Changes in V4:
https://lore.kernel.org/all/20230818112051.594986-1-anshuman.khandual@arm.c…
- Fixed a typo s/rangess/ranges,
- Renamed etm4_work_around_wrong_ccitmin() as etm4_core_reads_wrong_ccitmin()
- Moved drvdata->ccitmin value check for 256 inside etm4_core_reads_wrong_ccitmin()
- Moved the comment inside etm4_core_reads_wrong_ccitmin()
Changes in V3:
https://lore.kernel.org/all/20230811034600.944386-1-anshuman.khandual@arm.c…
- Added errata work around affecting TRCIDR3.CCITMIN
- Split the document update into a separate patch
Changes in V2:
https://lore.kernel.org/all/20230808074533.380537-1-anshuman.khandual@arm.c…
- s/treshhold/threshold
Changes in V1:
https://lore.kernel.org/all/20230804044720.1478900-1-anshuman.khandual@arm.…
Anshuman Khandual (3):
coresight: etm: Override TRCIDR3.CCITMIN on errata affected cpus
coresight: etm: Make cycle count threshold user configurable
Documentation: coresight: Add cc_threshold tunable
Documentation/arch/arm64/silicon-errata.rst | 10 ++++
Documentation/trace/coresight/coresight.rst | 4 ++
.../hwtracing/coresight/coresight-etm-perf.c | 2 +
.../coresight/coresight-etm4x-core.c | 46 ++++++++++++++++++-
4 files changed, 60 insertions(+), 2 deletions(-)
--
2.25.1