On Sun, 22 Oct 2023 20:58:06 +0200, Vegard Nossum wrote:
> This reference uses a glob pattern to match multiple files, but the
> asterisk was escaped as \* in order to not be interpreted by sphinx
> as reStructuredText markup.
>
> refcheckdocs/documentation-file-ref-check doesn't know about rST syntax
> and tries to interpret the \* literally (instead of as a glob).
>
> [...]
Applied, thanks!
[1/1] Documentation: coresight: fix `make refcheckdocs` warning
https://git.kernel.org/coresight/c/fa55e63584f2
Best regards,
--
Suzuki K Poulose <suzuki.poulose(a)arm.com>
On 10/20/2023 4:59 PM, Krzysztof Kozlowski wrote:
> On 20/10/2023 10:13, Tao Zhang wrote:
>>>> 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
> But your local DTS would not pass dtbs_check tests, so that's why I am
> saying - you never tested it on mainline kernel.
Thanks, we will add this test in the future to ensure that DTS, doc and
driver are consistent.
Best,
Tao
>
> Best regards,
> Krzysztof
>
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
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;
>> }
>
> .
>
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
>
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
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;
> .
>
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