CoreSight

coresight@lists.linaro.org

4 participants
2387 discussions

[PATCH] coresight: tmc: sg: Do not leak sg_table

by Suzuki K Poulose

Running perf with cs_etm on Juno triggers the following kmemleak warning ! :~# cat /sys/kernel/debug/kmemleak unreferenced object 0xffffff8806b6d720 (size 96): comm "perf", pid 562, jiffies 4297810960 hex dump (first 32 bytes): 38 d8 13 07 88 ff ff ff 00 d0 9e 85 c0 ff ff ff 8............... 00 10 00 88 c0 ff ff ff 00 f0 ff f7 ff 00 00 00 ................ backtrace (crc 1dbf6e00): [<ffffffc08107381c>] kmemleak_alloc+0xbc/0xd8 [<ffffffc0802f9798>] kmalloc_trace_noprof+0x220/0x2e8 [<ffffffc07bb71948>] tmc_alloc_sg_table+0x48/0x208 [coresight_tmc] [<ffffffc07bb71cbc>] tmc_etr_alloc_sg_buf+0xac/0x240 [coresight_tmc] [<ffffffc07bb72538>] tmc_alloc_etr_buf.constprop.0+0x1f0/0x260 [coresight_tmc] [<ffffffc07bb7280c>] alloc_etr_buf.constprop.0.isra.0+0x74/0xa8 [coresight_tmc] [<ffffffc07bb72950>] tmc_alloc_etr_buffer+0x110/0x260 [coresight_tmc] [<ffffffc07bb38afc>] etm_setup_aux+0x204/0x3b0 [coresight] [<ffffffc08025837c>] rb_alloc_aux+0x20c/0x318 [<ffffffc08024dd84>] perf_mmap+0x2e4/0x7a0 [<ffffffc0802cceb0>] mmap_region+0x3b0/0xa08 [<ffffffc0802cd8a8>] do_mmap+0x3a0/0x500 [<ffffffc080295328>] vm_mmap_pgoff+0x100/0x1d0 [<ffffffc0802cadf8>] ksys_mmap_pgoff+0xb8/0x110 [<ffffffc080020688>] __arm64_sys_mmap+0x38/0x58 [<ffffffc080028fc0>] invoke_syscall.constprop.0+0x58/0x100 This due to the fact that we do not free the "sg_table" itself while freeing up the SG table and data pages. Fix this by freeing the sg_table in tmc_free_sg_table(). Fixes: 99443ea19e8b ("coresight: Add generic TMC sg table framework") Cc: Mike Leach <mike.leach(a)linaro.org> Cc: James Clark <james.clark(a)arm.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> --- drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index e75428fa1592..610ad51cda65 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -261,6 +261,7 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table) { tmc_free_table_pages(sg_table); tmc_free_data_pages(sg_table); + kfree(sg_table); } EXPORT_SYMBOL_GPL(tmc_free_sg_table); @@ -342,7 +343,6 @@ struct tmc_sg_table *tmc_alloc_sg_table(struct device *dev, rc = tmc_alloc_table_pages(sg_table); if (rc) { tmc_free_sg_table(sg_table); - kfree(sg_table); return ERR_PTR(rc); } -- 2.34.1

10 months, 2 weeks

Re: [PATCH v2 RESEND] Coresight: Set correct cs_mode for TPDM to fix disable issue

by Suzuki K Poulose

On Mon, 12 Aug 2024 12:30:43 +0800, Jie Gan wrote: > The coresight_disable_source_sysfs function should verify the > mode of the coresight device before disabling the source. > > However, the mode for the TPDM device is always set to > CS_MODE_DISABLED, resulting in the check consistently failing. > As a result, TPDM cannot be properly disabled. > > [...] Applied, thanks! [1/1] Coresight: Set correct cs_mode for TPDM to fix disable issue https://git.kernel.org/coresight/c/14f5fa9b5fcbe2b3d5098893aba6ad62254d2ef6 Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

10 months, 2 weeks

Re: [PATCH v1 RESEND] Coresight: Set correct cs_mode for dummy source to fix disable issue

by Suzuki K Poulose

On Mon, 12 Aug 2024 12:28:44 +0800, Jie Gan wrote: > The coresight_disable_source_sysfs function should verify the > mode of the coresight device before disabling the source. > However, the mode for the dummy source device is always set to > CS_MODE_DISABLED, resulting in the check consistently failing. > As a result, dummy source cannot be properly disabled. > > Configure CS_MODE_SYSFS/CS_MODE_PERF during the enablement. > Configure CS_MODE_DISABLED during the disablement. > > [...] Applied, thanks! [1/1] Coresight: Set correct cs_mode for dummy source to fix disable issue https://git.kernel.org/coresight/c/e6b64cda393efd84709ab3df2e42d36d36d7553e Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

10 months, 2 weeks

Re: [PATCH v2 0/3] use device_for_each_child_node_scoped to access device child nodes

by Suzuki K Poulose

On Thu, 08 Aug 2024 17:12:36 +0200, Javier Carrasco wrote: > This series removes accesses to the device `fwnode` to iterate over its > own child nodes. Using the `device_for_each_child_node` macro provides > direct access to the device child nodes, and given that in all cases > they are only required within the loop, the scoped variant of the macro > can be used. > > It has been stated in previous discussions [1] that `device_for_each_*` > should be used to access device child nodes, removing the need to access > its internal fwnode, and restricting `fwnode_for_each_*` to traversing > subnodes when required. > > [...] Applied, to coresight next tree. Thanks! [1/3] coresight: cti: use device_* to iterate over device child nodes https://git.kernel.org/coresight/c/daca644d0c9e0 Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

10 months, 2 weeks

OpenCSD new branch

by Mike Leach

A new branch of OpenCSD is now available:- ocsd-consistency-checks-1.5.4-rc1 This adds in a number of consistency tests to try to detect when clients provide incorrect program images used for decode. Correct decode is dependent on getting the correct program image to walk looking for waypoints (branch instructions etc) to associate with the E/N atoms in the trace. Checks are enabled using operation flags in decoder creation:- OCSD_C_API ocsd_err_t ocsd_dt_create_decoder(const dcd_tree_handle_t handle, const char *decoder_name, const int create_flags, const void *decoder_cfg, unsigned char *pCSID ); Added flags for the create_flags parameter: OCSD_OPFLG_N_UNCOND_DIR_BR_CHK Check for N atom on unconditional direct branches - unconditional branches should always have associated E atoms. This option will generate an error and reset and resync the decoder if the target of the direct branch is not the next instruction. Some ETM IP will incorrectly trace taken branches to the next instruction as not taken branches - so this allows for these cases OCSD_OPFLG_STRICT_N_UNCOND_BR_CHK Check for N atom on all unconditional branches. This includes any direct and indirect unconditional branches. This check may be used if it is known the ETM IP does not have the issue mentioned above. OCSD_OPFLG_CHK_RANGE_CONTINUE Inconsistent range continuity on not taken branches. If a range has been output the ends on a non taken branch, it is expected that the next range will start at the end address of the previous range. If any of these checks fail then a OCSD_GEN_TRC_ELEM_NO_SYNC packet with a reason of UNSYNC_BAD_IMAGE will be output. Clients can choose which flags to use. By default all checks are off - to minimise possible performance hit. Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

10 months, 2 weeks

Re: [PATCH v2 1/3] coresight: cti: use device_* to iterate over device child nodes

by Mike Leach

On Thu, 8 Aug 2024 at 16:12, Javier Carrasco <javier.carrasco.cruz(a)gmail.com> wrote: > > Drop the manual access to the fwnode of the device to iterate over its > child nodes. `device_for_each_child_node` macro provides direct access > to the child nodes, and given that they are only required within the > loop, the scoped variant of the macro can be used. > > Use the `device_for_each_child_node_scoped` macro to iterate over the > direct child nodes of the device. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz(a)gmail.com> > --- > drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c > index ccef04f27f12..d0ae10bf6128 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-platform.c > +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c > @@ -416,20 +416,16 @@ static int cti_plat_create_impdef_connections(struct device *dev, > struct cti_drvdata *drvdata) > { > int rc = 0; > - struct fwnode_handle *fwnode = dev_fwnode(dev); > - struct fwnode_handle *child = NULL; > > - if (IS_ERR_OR_NULL(fwnode)) > + if (IS_ERR_OR_NULL(dev_fwnode(dev))) > return -EINVAL; > > - fwnode_for_each_child_node(fwnode, child) { > + device_for_each_child_node_scoped(dev, child) { > if (cti_plat_node_name_eq(child, CTI_DT_CONNS)) > - rc = cti_plat_create_connection(dev, drvdata, > - child); > + rc = cti_plat_create_connection(dev, drvdata, child); > if (rc != 0) > break; > } > - fwnode_handle_put(child); > > return rc; > } > > -- > 2.43.0 > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

10 months, 2 weeks

[PATCH v2 0/2] coresight: Add remote etm support

by Mao Jinlong

The system on chip (SoC) consists of main APSS(Applications processor subsytem) and additional processors like modem, lpass. There is coresight-etm driver for etm trace of APSS. Coresight remote etm driver is for enabling and disabling the etm trace of remote processors. It uses QMI interface to communicate with remote processors' software and uses coresight framework to configure the connection from remote etm source to TMC sinks. Example to capture the remote etm trace: Enable source: echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink echo 1 > /sys/bus/coresight/devices/remote_etm0/enable_source Capture the trace: cat /dev/tmc_etf0 > /data/remote_etm.bin Disable source: echo 0 > /sys/bus/coresight/devices/remote_etm0/enable_source Changes since V1: 1. Remove unused content 2. Use CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS as remote etm source type. 3. Use enabled instead of enable in driver data. 4. Validate instance id value where it's read from the DT. Mao Jinlong (2): dt-bindings: arm: Add qcom,inst-id for remote etm coresight: Add remote etm support .../arm/qcom,coresight-remote-etm.yaml | 10 + drivers/hwtracing/coresight/Kconfig | 13 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-qmi.h | 89 +++++ .../coresight/coresight-remote-etm.c | 308 ++++++++++++++++++ 5 files changed, 421 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c -- 2.41.0

10 months, 3 weeks

[PATCH v2 0/3] Add source filtering for multi-port output

by Tao Zhang

In our hardware design, by combining a funnel and a replicator, it implement a hardware device with one-to-one correspondence between output ports and input ports. The programming usage on this device is the same as funnel. The software uses a funnel and a static replicator to implement the driver of this device. Since original funnels only support a single output connection and original replicator only support a single input connection, the code needs to be modified to support this new feature. The following is a typical topology diagram of multi-port output mechanism. |----------| |---------| |----------| |---------| | TPDM 0 | | Source0 | | Source 1 | | TPDM 1 | |----------| |---------| |----------| |---------| | | | | | | | | | --------- | | | | | | | | | | | | | | | \-------------/ ---------------------- | \ Funnel 0 / | | ----------- | ------------------------------ | | | | | | \------------------/ \ Funnel 1 / ----| \--------------/ | | |----> Combine a funnel and a | | static replicator /-----------------\ | / replicator 0 \ ----| /---------------------\ | | | | | |-----------| | |---------| | | |TPDM0 |TPDM1 | \-----------------/ | \ TPDA 0 / | \-------------/ | | | | |Source0/1 | \-------------------------------/ \ Funnel 2 / \---------------------------/ Changes in V2: 1. Change the reference for endpoint property in dt-binding. -- Krzysztof Kozlowski 2. Change the property name "filter_src" to "filter-src". -- Krzysztof Kozlowski 3. Fix the errors in running 'make dt_binding_check'. -- Rob Herring 4. Pass in the source parameter instead of path. -- Suzuki K Poulose 5. Reset the "filter_src_dev" if the "src" csdev is being removed. -- Suzuki K Poulose 6. Add a warning if the "filter_src_dev" is of not the type DEV_TYPE_SOURCE. -- Suzuki K Poulose 7. Optimize the procedure for handling all possible cases. -- Suzuki K Poulose Changes in V1: 1. Add a static replicator connect to a funnel to implement the correspondence between the output ports and the input ports on funnels. -- Suzuki K Poulose 2. Add filter_src_dev and filter_src_dev phandle to "coresight_connection" struct, and populate them if there is one. -- Suzuki K Poulose 3. To look at the phandle and then fixup/remove the filter_src device in fixup/remove connections. -- Suzuki K Poulose 4. When TPDA reads DSB/CMB element size, it is implemented by looking up filter src device in the connections. -- Suzuki K Poulose Tao Zhang (3): dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering coresight: Add source filtering for multi-port output coresight-tpda: Optimize the function of reading element size .../arm/arm,coresight-static-replicator.yaml | 19 ++- drivers/hwtracing/coresight/coresight-core.c | 125 ++++++++++++++++-- .../hwtracing/coresight/coresight-platform.c | 18 +++ drivers/hwtracing/coresight/coresight-tpda.c | 3 +- include/linux/coresight.h | 5 + 5 files changed, 155 insertions(+), 15 deletions(-) -- 2.17.1

11 months

Re: [PATCH v1 3/9] perf auxtrace: Refactor auxtrace__evsel_is_auxtrace()

by James Clark

On 08/08/2024 1:58 pm, Adrian Hunter wrote: > On 6/08/24 23:41, Leo Yan wrote: >> The auxtrace__evsel_is_auxtrace() function invokes the callback >> .evsel_is_auxtrace() to check if an event is an AUX trace. In the >> low-level code, every AUX trace module provides its callback to >> compare the PMU type. >> >> This commit refactors auxtrace__evsel_is_auxtrace() by simply >> calling evsel__is_aux_event() rather than using the callback function. >> As a result, the callback .evsel_is_auxtrace() is no longer needed, so >> the definition and implementations are removed. > > evsel__is_aux_event() assumes it is on the target machine e.g. > being called from perf record. It indirectly reads from sysfs > to find PMUs, which will not necessarily be the same a different > machine. > > For example, what happens if a perf data file from one arch is > being processed on a machine from another arch. > I think this does go a bit wrong. If I open an SPE file on x86 it finds the intel_pt PMU which both have the same type number. But because that's also an auxtrace one it appears to work.

11 months

Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken

by Mike Leach

On Thu, 8 Aug 2024 at 12:15, Ganapatrao Kulkarni <gankulkarni(a)os.amperecomputing.com> wrote: > > > > On 08-08-2024 04:21 pm, James Clark wrote: > > > > > > On 08/08/2024 10:21 am, James Clark wrote: > >> > >> > >> On 08/08/2024 8:42 am, Leo Yan wrote: > >>> On 8/8/2024 5:36 AM, Ganapatrao Kulkarni wrote: > >>>> > >>>> On 08-08-2024 12:50 am, Leo Yan wrote: > >>>>> On 8/7/2024 5:18 PM, Ganapatrao Kulkarni wrote: > >>>>> > >>>>>> Is below diff with force option looks good? > >>>>>> > >>>>>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py > >>>>>> b/tools/perf/scripts/python/arm-cs-trace-disasm.py > >>>>>> index d973c2baed1c..efe34f308beb 100755 > >>>>>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py > >>>>>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py > >>>>>> @@ -36,7 +36,10 @@ option_list = [ > >>>>>> help="Set path to objdump executable file"), > >>>>>> make_option("-v", "--verbose", dest="verbose", > >>>>>> action="store_true", default=False, > >>>>>> - help="Enable debugging log") > >>>>>> + help="Enable debugging log"), > >>>>>> + make_option("-f", "--force", dest="force", > >>>>>> + action="store_true", default=False, > >>>>>> + help="Force decoder to continue") > >>>>>> ] > >>>>>> > >>>>>> parser = OptionParser(option_list=option_list) > >>>>>> @@ -257,6 +260,12 @@ def process_event(param_dict): > >>>>>> print("Stop address 0x%x is out of range [ 0x%x > >>>>>> .. 0x%x > >>>>>> ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso)) > >>>>>> return > >>>>>> > >>>>>> + if (stop_addr < start_addr): > >>>>>> + if (options.verbose == True or options.force): > >>>>>> + print("Packet Discontinuity detected > >>>>>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, > >>>>>> start_addr, dso)) > >> > >> The options.force for the print should be "options.verbose or not > >> options.force" I think? You want to print the error until the user > >> adds -f, then hide it. Unless verbose is on. > >> > >>>>>> + if (options.force): > >>>>>> + return > >> > >> Oops I had this one the wrong way around in my example. This way is > >> correct. > >> > >>>>> > >>>>> I struggled a bit for the code - it is confused that force mode > >>>>> bails out > >>>>> and the non-force mode continues to run. I prefer to always bail > >>>>> out for > >>>>> the discontinuity case, as it is pointless to continue in this case. > >>>> > >>>> Kept bail out with force option since I though it is not good to hide > >>>> the error in normal use, otherwise we never able to notice this > >>>> error in > >>>> the future and it becomes default hidden. Eventually this error should > >>>> be fixed. > >>> > >>> As James said, the issue should be fixed in OpenCSD or Perf decoding > >>> flow. > >>> > >>> Thus, perf tool should be tolerant errors - report warning and drop > >>> discontinuous samples. This would be easier for developers later if face > >>> the same issue, they don't need to spend time to locate issue and > >>> struggle > >>> for overriding the error. > >>> > >>> If you prefer to use force option, it might be better to give > >>> reasoning and > >>> *suggestion* in one go, something like: > >>> > >>> if (stop_addr < start_addr): > >>> print("Packet Discontinuity detected [stop_add:0x%x > >>> start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, dso)) > >>> print("Use option '-f' following the script for force mode" > >>> if (options.force) > >>> return > >>> > >>> Either way is fine for me. Thanks a lot for taking time on the issue. > >>> > >>> Leo > >> > >> But your diff looks good Ganapat, I think send a patch with Leo's > >> extra help message added and the first force flipped. > > > > One other small detail about Leo's suggestion print out. Can you add an > > instruction of how to keep the warnings as well: > > > > print("Use option '-f' following the script for force mode. Add '-v' \ > > to continue printing decode warnings.") > > > > Thanks James and Leo for your comments. > I will send the V2 with the changes as discussed. > > Thanks. > Ganapat > Certainly any ARM trace decode is dependent on accurate program images being input to provide correct trace decode at the output. So if an OpenCSD client does not provide accurate information then it should not really expect to get accurate trace as an output! That said there are certainly a couple of changes that can be made:- 1) Clearly outputting a trace range with a finish address lower than the start address is incorrect. This can unconditionally output a hard error. 2) Detection of non-cond not taken should be added as a configurable option. - either all, or direct only. This can be achieved by adding flags to the library configuration API. For a client like perf - these could be controlled by the verbose level - which I believe is an int in the range 0-10 or something? However - when we do detect these errors, it is essential that the entire decoder is reset and tracing not restarted till the next sync point in the trace data. i.e. assuming that the next range that happens to be consecutive after a break, given a prior input of incorrect address data is simply invalid. There is no way of knowing if the branch taken / not taken sequence matches the addresses in the program image any more. The solution that James proposes above, needs to actually generate an error which will then automatically reset the decoder to an unsynced state. Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

11 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight