CoreSight

coresight@lists.linaro.org

7 participants
2448 discussions

[PATCH v2 0/4] Fixed some issues and cleanup of ultrasoc-smb

by Junhao He

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 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, 37 insertions(+), 77 deletions(-) -- 2.33.0

1 year, 10 months

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

by hejunhao

Hi Jonathan, On 2023/10/19 21:30, Jonathan Cameron wrote: > On Thu, 12 Oct 2023 17:47:04 +0800 > Junhao He <hejunhao3(a)huawei.com> wrote: > >> When we to enable the SMB by perf, the perf sched will call perf_ctx_lock() >> to close system preempt in event_function_call(). But SMB::enable_smb() use >> mutex to lock the critical section, which may sleep. >> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580 >> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 153023, name: perf >> preempt_count: 2, expected: 0 >> RCU nest depth: 0, expected: 0 >> INFO: lockdep is turned off. >> irq event stamp: 0 >> hardirqs last enabled at (0): [<0000000000000000>] 0x0 >> hardirqs last disabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48 >> softirqs last enabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48 >> softirqs last disabled at (0): [<0000000000000000>] 0x0 >> CPU: 2 PID: 153023 Comm: perf Kdump: loaded Tainted: G W O 6.5.0-rc4+ #1 >> >> Call trace: >> ... >> __mutex_lock+0xbc/0xa70 >> mutex_lock_nested+0x34/0x48 >> smb_update_buffer+0x58/0x360 [ultrasoc_smb] >> etm_event_stop+0x204/0x2d8 [coresight] >> etm_event_del+0x1c/0x30 [coresight] >> event_sched_out+0x17c/0x3b8 >> group_sched_out.part.0+0x5c/0x208 >> __perf_event_disable+0x15c/0x210 >> event_function+0xe0/0x230 >> remote_function+0xb4/0xe8 >> generic_exec_single+0x160/0x268 >> smp_call_function_single+0x20c/0x2a0 >> event_function_call+0x20c/0x220 >> _perf_event_disable+0x5c/0x90 >> perf_event_for_each_child+0x58/0xc0 >> _perf_ioctl+0x34c/0x1250 >> perf_ioctl+0x64/0x98 >> ... >> >> Use spinlock replace mutex to control driver data access to one at a >> time. But the function copy_to_user() may sleep so spinlock do not to >> lock it. > I'd like to see a comment on why we no longer need to lock over the copy_to_user > rather than simply that we can't. Yes, I will do that. >> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver") >> Signed-off-by: Junhao He <hejunhao3(a)huawei.com> > A follow up patch could change a lot of this to use the new cleanup.h (don't want that > in the fix though as will make back porting trickier.). > That should let you do > guard(spin_lock)(&drvdata->spinlock); > and then use direct returns instead of goto complexity. > > > > Jonathan Thanks for sharing. I will append up a new patch to use guards to reduce gotos. > >> @@ -132,10 +132,8 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, >> if (!len) >> return 0; >> >> - mutex_lock(&drvdata->mutex); >> - >> if (!sdb->data_size) >> - goto out; >> + return 0; >> >> to_copy = min(sdb->data_size, len); >> >> @@ -145,20 +143,18 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, >> >> if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) { >> dev_dbg(dev, "Failed to copy data to user\n"); >> - to_copy = -EFAULT; >> - goto out; >> + return -EFAULT; >> } >> >> + spin_lock(&drvdata->spinlock); >> *ppos += to_copy; >> - > Unrelated white space change that shouldn't be here. Ok, i will drop this white space Thanks for the comments! Best regards, Junhao. > >> smb_update_read_ptr(drvdata, to_copy); >> >> - dev_dbg(dev, "%zu bytes copied\n", to_copy); >> -out: >> if (!sdb->data_size) >> smb_reset_buffer(drvdata); >> - mutex_unlock(&drvdata->mutex); >> + spin_unlock(&drvdata->spinlock); >> >> + dev_dbg(dev, "%zu bytes copied\n", to_copy); >> return to_copy; >> } > > . >

1 year, 10 months

[PATCH v2] MAINTAINERS: coresight: Add missing Coresight files

by James Clark

There are a few files missing from the list like test_arm_coresight.sh and arm-coresight.txt so add the missing entries. Signed-off-by: James Clark <james.clark(a)arm.com> --- Changes since V1: * Don't wildcard everything so that habanalabs/gaudi/gaudi_coresight* isn't included. Link to V1: https://lore.kernel.org/lkml/20231005084001.34741-1-james.clark@arm.com/ MAINTAINERS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7a7bd8bd80e9..6b0e21ab7d00 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2068,12 +2068,16 @@ F: Documentation/trace/coresight/* F: drivers/hwtracing/coresight/* F: include/dt-bindings/arm/coresight-cti-dt.h F: include/linux/coresight* +F: include/uapi/linux/coresight* F: samples/coresight/* +F: tools/perf/Documentation/arm-coresight.txt F: tools/perf/arch/arm/util/auxtrace.c F: tools/perf/arch/arm/util/cs-etm.c F: tools/perf/arch/arm/util/cs-etm.h F: tools/perf/arch/arm/util/pmu.c +F: tools/perf/tests/shell/*coresight* F: tools/perf/tests/shell/coresight/* +F: tools/perf/tests/shell/lib/*coresight* F: tools/perf/util/cs-etm-decoder/* F: tools/perf/util/cs-etm.* -- 2.34.1

1 year, 10 months

Re: [PATCH] coresight-tpdm: Correct the property name of MSR number

by Tao Zhang

On 10/20/2023 2:55 PM, Krzysztof Kozlowski wrote: > On 20/10/2023 04:51, Tao Zhang wrote: >> Correct the property name of the DSB MSR number that needs to be >> read in TPDM driver. The right property name is >> "qcom,dsb-msrs-num". > Missing Fixes tag. I will add it in the next patch. > >> This patch depends on patch series "Add support to configure TPDM DSB >> subunit" >> https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=788353 > This is not suitable for commit msg. Dependencies are noted under ---. > > And how is this depending on that patch? Your buggy code was applied > long time ago! Yes, no need to depend on the patch series and it has been applied. I will remove this comments in the next patch. >> Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c >> index b25284e..97654aa 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -892,7 +892,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) >> >> if (drvdata && tpdm_has_dsb_dataset(drvdata)) >> of_property_read_u32(drvdata->dev->of_node, >> - "qcom,dsb_msr_num", &drvdata->dsb_msr_num); >> + "qcom,dsb-msrs-num", &drvdata->dsb_msr_num); > So you never tested your DTS... We can keep asking about this but still > testing does not happen :/ Since this new property has not been applied on the exist upstream DTS, I tested this driver with the local DTS. Unfortunately, the property name in the local DTS is not updated, this is why it is not found in the tests. Best, Tao > > Best regards, > Krzysztof >

1 year, 10 months

[PATCH] coresight-tpdm: Correct the property name of MSR number

by Tao Zhang

Correct the property name of the DSB MSR number that needs to be read in TPDM driver. The right property name is "qcom,dsb-msrs-num". This patch depends on patch series "Add support to configure TPDM DSB subunit" https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=788353 Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> --- drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index b25284e..97654aa 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -892,7 +892,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) if (drvdata && tpdm_has_dsb_dataset(drvdata)) of_property_read_u32(drvdata->dev->of_node, - "qcom,dsb_msr_num", &drvdata->dsb_msr_num); + "qcom,dsb-msrs-num", &drvdata->dsb_msr_num); /* Set up coresight component description */ desc.name = coresight_alloc_device_name(&tpdm_devs, dev); -- 2.7.4

1 year, 10 months

Re: [PATCH 2/3] coresight: ultrasoc-smb: simplify the code for check to_copy valid

by hejunhao

On 2023/10/19 21:35, Jonathan Cameron wrote: > On Thu, 12 Oct 2023 17:47:05 +0800 > Junhao He <hejunhao3(a)huawei.com> wrote: > >> We only need to check once when before using the to_copy variable >> to simplify the code. >> >> Signed-off-by: Junhao He <hejunhao3(a)huawei.com> > I'm not convinced this one is an improvement. Sometimes it's easier to just > see the individual conditions checked even if we could combine them. > It's easy to understand we don't copy data if: > a) We ask for 0 data. > b) There is 0 data > > Less easy to establish that with the extra wrap around code in there > (even though that has no impact on to_copy if it is 0) > > Jonathan > Thanks, I will drop this patch. Best regards, Junhao. >> --- >> drivers/hwtracing/coresight/ultrasoc-smb.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c >> index b08a619d1116..e78edc3480ce 100644 >> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c >> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c >> @@ -127,20 +127,15 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, >> struct smb_drv_data, miscdev); >> struct smb_data_buffer *sdb = &drvdata->sdb; >> struct device *dev = &drvdata->csdev->dev; >> - ssize_t to_copy = 0; >> - >> - if (!len) >> - return 0; >> - >> - if (!sdb->data_size) >> - return 0; >> - >> - to_copy = min(sdb->data_size, len); >> + ssize_t to_copy = min(sdb->data_size, len); >> >> /* Copy parts of trace data when read pointer wrap around SMB buffer */ >> if (sdb->buf_rdptr + to_copy > sdb->buf_size) >> to_copy = sdb->buf_size - sdb->buf_rdptr; >> >> + if (!to_copy) >> + return 0; >> + >> if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) { >> dev_dbg(dev, "Failed to copy data to user\n"); >> return -EFAULT; > . >

1 year, 10 months

[PATCH v2 0/6] kvm/coresight: Support exclude guest and exclude host

by James Clark

This is a combination of the RFC for nVHE here [1] and v3 of VHE version here [2]. After a few of the review comments it seemed much simpler for both versions to use the same interface and be in the same patchset. FEAT_TRF is a Coresight feature that allows trace capture to be completely filtered at different exception levels, unlike the existing TRCVICTLR controls which may still emit target addresses of branches, even if the following trace is filtered. Without FEAT_TRF, it was possible to start a trace session on a host and also collect trace from the guest as TRCVICTLR was never programmed to exclude guests (and it could still emit target addresses even if it was). With FEAT_TRF, the current behavior of trace in guests exists depends on whether nVHE or VHE are being used. Both of the examples below are from the host's point of view, as Coresight isn't accessible from guests. This patchset is only relevant to when FEAT_TRF exists, otherwise there is no change. nVHE: Because the host and the guest are both using TRFCR_EL1, trace will be generated in guests depending on the same filter rules the host is using. For example if the host is tracing userspace only, then guest userspace trace will also be collected. (This is further limited by whether TRBE is used because an issue with TRBE means that it's completely disabled in nVHE guests, but it's possible to have other tracing components.) VHE: With VHE, the host filters will be in TRFCR_EL2, but the filters in TRFCR_EL1 will be active when the guest is running. Because we don't write to TRFCR_EL1, guest trace will be completely disabled. With this change, the guest filtering rules from the Perf session are honored for both nVHE and VHE modes. This is done by either writing to TRFCR_EL12 at the start of the Perf session and doing nothing else further, or caching the guest value and writing it at guest switch for nVHE. The first commit moves the register to sysreg because I add the EL12 version in a later commit. --- Changes since V1: * Squashed all the arm64/tools/sysreg changes into the first commit * Add a new commit to move SPE and TRBE regs into the kvm sysreg array * Add a comment above the TRFCR global that it's per host CPU rather than vcpu Changes since nVHE RFC [1]: * Re-write just in terms of the register value to be written for the host and the guest. This removes some logic from the hyp code and a value of kvm_vcpu_arch:trfcr_el1 = 0 no longer means "don't restore". * Remove all the conditional compilation and new files. * Change the kvm_etm_update_vcpu_events macro to a function. * Re-use DEBUG_STATE_SAVE_TRFCR so iflags don't need to be expanded anymore. * Expand the cover letter. Changes since VHE v3 [2]: * Use the same interface as nVHE mode so TRFCR_EL12 is now written by kvm. [1]: https://lore.kernel.org/kvmarm/20230804101317.460697-1-james.clark@arm.com/ [2]: https://lore.kernel.org/kvmarm/20230905102117.2011094-1-james.clark@arm.com/ James Clark (6): arm64/sysreg: Move TRFCR definitions to sysreg arm64: KVM: Rename DEBUG_STATE_SAVE_TRBE to DEBUG_STATE_SAVE_TRFCR arm64: KVM: Move SPE and trace registers to the sysreg array arm64: KVM: Add interface to set guest value for TRFCR register arm64: KVM: Write TRFCR value on guest switch with nVHE coresight: Pass guest TRFCR value to KVM arch/arm64/include/asm/kvm_host.h | 13 +-- arch/arm64/include/asm/kvm_hyp.h | 6 +- arch/arm64/include/asm/sysreg.h | 12 --- arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/debug.c | 43 ++++++++- arch/arm64/kvm/hyp/nvhe/debug-sr.c | 87 +++++++++++-------- arch/arm64/kvm/hyp/nvhe/switch.c | 4 +- arch/arm64/tools/sysreg | 41 +++++++++ .../coresight/coresight-etm4x-core.c | 42 +++++++-- drivers/hwtracing/coresight/coresight-etm4x.h | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 3 + 11 files changed, 186 insertions(+), 68 deletions(-) -- 2.34.1

1 year, 10 months

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.

1 year, 10 months

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.

1 year, 10 months

[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

1 year, 10 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight