CoreSight October 2023

coresight@lists.linaro.org

11 participants
45 discussions

Re: [PATCH 3/3] coresight: ultrasoc-smb: fix uninitialized before use buf_hw_base

by hejunhao

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.

6 months, 3 weeks

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

by hejunhao

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.

6 months, 3 weeks

[PATCH v1 0/2] perf cs-etm: Add support for itrace option 'T'

by Leo Yan

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

6 months, 3 weeks

Re: [PATCH v1 1/2] perf auxtrace: Add 'T' itrace option for timestamp trace

by James Clark

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" >> > >

6 months, 3 weeks

[PATCH] coresight: etm4x: Enable ETE device accessed via MMIO

by Ruidong Tian

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

6 months, 3 weeks

RE: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

by Linu Cherian

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.

6 months, 3 weeks

Re: [PATCH] Documentation: ABI: coresight-tpdm: Fix Bit[3] description indentation

by Suzuki K Poulose

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

6 months, 3 weeks

Re: [PATCH] coresight: etm4x: Remove bogous __exit annotation for some functions

by Suzuki K Poulose

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

6 months, 3 weeks

[PATCH] coresight: Fix crash when Perf and sysfs modes are used concurrently

by James Clark

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

6 months, 3 weeks

[PATCH V7 0/3] coresight: etm: Make cycle count threshold user configurable

by Anshuman Khandual

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

6 months, 3 weeks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight October 2023