CoreSight December 2021

coresight@lists.linaro.org

8 participants
27 discussions

Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver

by Mike Leach

Hi, I have a couple of comments relating to Coresight trace IDs On Wed, 15 Dec 2021 at 16:10, Jinlong Mao <quic_jinlmao(a)quicinc.com> wrote: > > Hi Mathieu, > > On 12/15/2021 2:57 AM, Mathieu Poirier wrote: > > On Thu, Dec 09, 2021 at 10:15:36PM +0800, Mao Jinlong wrote: > >> Add driver to support Coresight device TPDM (Trace, Profiling and > >> Diagnostics Monitor). TPDM is a monitor to collect data from > >> different datasets. This change is to add probe/enable/disable > >> functions for tpdm source. > >> > >> Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > >> Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com> > >> --- > >> MAINTAINERS | 8 + > >> drivers/hwtracing/coresight/Kconfig | 13 ++ > >> drivers/hwtracing/coresight/Makefile | 1 + > >> drivers/hwtracing/coresight/coresight-core.c | 3 +- > >> drivers/hwtracing/coresight/coresight-tpdm.c | 152 +++++++++++++++++++ > >> drivers/hwtracing/coresight/coresight-tpdm.h | 31 ++++ > >> include/linux/coresight.h | 1 + > >> 7 files changed, 208 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c > >> create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.h > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index 7a2345ce8521..59f39b3194f6 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -15560,6 +15560,14 @@ L: netdev(a)vger.kernel.org > >> S: Supported > >> F: drivers/net/ipa/ > >> > >> +QCOM CORESIGHT COMPONENTS DRIVER > >> +M: Tao Zhang <quic_taozha(a)quicinc.com> > >> +M: Jinlong Mao <quic_jinlmao(a)quicinc.com> > >> +M: Mathieu Poirier <mathieu.poirier(a)linaro.org> > >> +M: Suzuki K Poulose <suzuki.poulose(a)arm.com> > >> +S: Maintained > >> +F: drivers/hwtracing/coresight/coresight-tpdm.c > >> + > > There is no need for an extra entry in the MAINTAINERS file. The checkpatch.pl > > script is smart enough to know when to CC you and Tao every time the TPDM/TPDA > > drivers are modified. Suzuki and I will simply wait for you guys to add your RB > > tags before reviewing the patches. I have explained this in the previous revision. > > Hi Mathieu, > > I will remove this entry. > > >> QEMU MACHINE EMULATOR AND VIRTUALIZER SUPPORT > >> M: Gabriel Somlo <somlo(a)cmu.edu> > >> M: "Michael S. Tsirkin" <mst(a)redhat.com> > >> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > >> index 514a9b8086e3..5c506a1cd08f 100644 > >> --- a/drivers/hwtracing/coresight/Kconfig > >> +++ b/drivers/hwtracing/coresight/Kconfig > >> @@ -201,4 +201,17 @@ config CORESIGHT_TRBE > >> > >> To compile this driver as a module, choose M here: the module will be > >> called coresight-trbe. > >> + > >> +config CORESIGHT_TPDM > >> + tristate "CoreSight Trace, Profiling & Diagnostics Monitor driver" > >> + select CORESIGHT_LINKS_AND_SINKS > > Is this available on 32bit HW as well? If not please make it dependent on ARM64 > > as it for ETMv4x devices. > This is available on 32bit HW as well. > >> + help > >> + This driver provides support for configuring monitor. Monitors are > >> + primarily responsible for data set collection and support the > >> + ability to collect any permutation of data set types. Monitors are > >> + also responsible for interaction with system cross triggering. > >> + > >> + To compile this driver as a module, choose M here: the module will be > >> + called coresight-tpdm. > >> + > >> endif > >> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > >> index b6c4a48140ec..e7392a0dddeb 100644 > >> --- a/drivers/hwtracing/coresight/Makefile > >> +++ b/drivers/hwtracing/coresight/Makefile > >> @@ -25,5 +25,6 @@ obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o > >> obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o > >> obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o > >> obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o > >> +obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o > >> coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ > >> coresight-cti-sysfs.o > >> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > >> index cc6b6cabf85f..a7f1a6f09cfb 100644 > >> --- a/drivers/hwtracing/coresight/coresight-core.c > >> +++ b/drivers/hwtracing/coresight/coresight-core.c > >> @@ -1071,7 +1071,8 @@ static int coresight_validate_source(struct coresight_device *csdev, > >> } > >> > >> if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && > >> - subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) { > >> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && > >> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SYS) { > >> dev_err(&csdev->dev, "wrong device subtype in %s\n", function); > >> return -EINVAL; > >> } > >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > >> new file mode 100644 > >> index 000000000000..f494cef4fb24 > >> --- /dev/null > >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > >> @@ -0,0 +1,152 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > >> + */ > >> + > >> +#include <linux/amba/bus.h> > >> +#include <linux/bitmap.h> > >> +#include <linux/coresight.h> > >> +#include <linux/device.h> > >> +#include <linux/err.h> > >> +#include <linux/fs.h> > >> +#include <linux/io.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > >> +#include <linux/regulator/consumer.h> > >> + > >> +#include "coresight-priv.h" > >> +#include "coresight-tpdm.h" > >> + > >> +DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); > >> + > >> +static int tpdm_enable(struct coresight_device *csdev, > >> + struct perf_event *event, u32 mode) > >> +{ > >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > >> + > >> + mutex_lock(&drvdata->lock); > >> + if (drvdata->enable) { > >> + mutex_unlock(&drvdata->lock); > >> + return -EBUSY; > >> + } > >> + > >> + drvdata->enable = true; > >> + mutex_unlock(&drvdata->lock); > >> + > >> + dev_info(drvdata->dev, "TPDM tracing enabled\n"); > >> + return 0; > >> +} > >> + > >> +static void tpdm_disable(struct coresight_device *csdev, > >> + struct perf_event *event) > >> +{ > >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > >> + > >> + mutex_lock(&drvdata->lock); > >> + if (!drvdata->enable) { > >> + mutex_unlock(&drvdata->lock); > >> + return; > >> + } > >> + > >> + drvdata->enable = false; > >> + mutex_unlock(&drvdata->lock); > >> + > >> + dev_info(drvdata->dev, "TPDM tracing disabled\n"); > >> +} > >> + > >> +static int tpdm_trace_id(struct coresight_device *csdev) > >> +{ > >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > >> + > >> + return drvdata->traceid; > >> +} > >> + > >> +static const struct coresight_ops_source tpdm_source_ops = { > >> + .trace_id = tpdm_trace_id, > >> + .enable = tpdm_enable, > >> + .disable = tpdm_disable, > >> +}; > >> + > >> +static const struct coresight_ops tpdm_cs_ops = { > >> + .source_ops = &tpdm_source_ops, > >> +}; > >> + > >> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) > >> +{ > >> + static int traceid = TPDM_TRACE_ID_START; > >> + > >> + drvdata->traceid = traceid++; > >> +} > > I have been specific on how to properly do this in the last revision. Given the > > above about the MAINTAINERS file, I am not sure that I will continue reviewing this set. > > > > There is also no need to rush another revision as I won't have the bandwidth to > > process it before the holidays. > > > > Thanks, > > Mathieu > > Hi Mathieu, > > Sorry, not addressed your previous comments here. > > For the trace id, each coresight component has 7 bits to store the trace > id. So the trace id should be from 1 to 127 as 0 is invalid. IDs 0x70 - 0x7F (`112 - 127 ) are reserved - see the ARM Coresight Architecture specification v3.0 > > Apart from TPDMs/STM/ETMs, we also have other coresight components in > our internal device. About 80 ids are already used. > > Some components have fixed trace id in HW. If we use functions below to > count the trace id, there will be conflict to other components. > > Can we use 1-15 for etm trace ids and 16 - 127 for other coresight > components ? And handle trace ids in its' own driver ? > This will limit systems to 15 cores - some have more! > > static inline int coresight_get_system_trace_id(int id) > { > /* Start system IDs above the highest per CPU trace ID. */ > return coresigth_get_trace_id(cpumask_last(cpu_possible_mask) + 1); > } > > static inline int coresight_get_trace_id(int cpu) > { > /* > * A trace ID of value 0 is invalid, so let's start at some > * random value that fits in 7 bits and go from there. Since > * the common convention is to have data trace IDs be I(N) + 1, > * set instruction trace IDs as a function of the CPU number. > */ > return (CORESIGHT_ETM_PMU_SEED + (cpu * 2)); > } > This fixed relationship between cpu and trace ID is used in the perf tooling to populate the elements in the perf.data file to correctly allow association between CPU and trace data, and thus allow correct trace decode. It should be possible to create another more dynamic mapping scheme - but this must include a way to support the perf requirements too. Regards Mike > Thanks > > Jinlong Mao > > >> + > >> +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > >> +{ > >> + struct device *dev = &adev->dev; > >> + struct coresight_platform_data *pdata; > >> + struct tpdm_drvdata *drvdata; > >> + struct coresight_desc desc = { 0 }; > >> + > >> + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); > >> + if (!desc.name) > >> + return -ENOMEM; > >> + pdata = coresight_get_platform_data(dev); > >> + if (IS_ERR(pdata)) > >> + return PTR_ERR(pdata); > >> + adev->dev.platform_data = pdata; > >> + > >> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > >> + if (!drvdata) > >> + return -ENOMEM; > >> + drvdata->dev = &adev->dev; > >> + dev_set_drvdata(dev, drvdata); > >> + > >> + drvdata->base = devm_ioremap_resource(dev, &adev->res); > >> + if (!drvdata->base) > >> + return -ENOMEM; > >> + > >> + mutex_init(&drvdata->lock); > >> + > >> + desc.type = CORESIGHT_DEV_TYPE_SOURCE; > >> + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SYS; > >> + desc.ops = &tpdm_cs_ops; > >> + desc.pdata = adev->dev.platform_data; > >> + desc.dev = &adev->dev; > >> + drvdata->csdev = coresight_register(&desc); > >> + if (IS_ERR(drvdata->csdev)) > >> + return PTR_ERR(drvdata->csdev); > >> + > >> + tpdm_init_default_data(drvdata); > >> + pm_runtime_put(&adev->dev); > >> + > >> + return 0; > >> +} > >> + > >> +static void __exit tpdm_remove(struct amba_device *adev) > >> +{ > >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); > >> + > >> + coresight_unregister(drvdata->csdev); > >> +} > >> + > >> +static struct amba_id tpdm_ids[] = { > >> + { > >> + .id = 0x000f0e00, > >> + .mask = 0x000fff00, > >> + }, > >> + { 0, 0}, > >> +}; > >> + > >> +static struct amba_driver tpdm_driver = { > >> + .drv = { > >> + .name = "coresight-tpdm", > >> + .owner = THIS_MODULE, > >> + .suppress_bind_attrs = true, > >> + }, > >> + .probe = tpdm_probe, > >> + .id_table = tpdm_ids, > >> +}; > >> + > >> +module_amba_driver(tpdm_driver); > >> + > >> +MODULE_LICENSE("GPL v2"); > >> +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); > >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > >> new file mode 100644 > >> index 000000000000..980ae90ff1c8 > >> --- /dev/null > >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > >> @@ -0,0 +1,31 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > >> + */ > >> + > >> +#ifndef _CORESIGHT_CORESIGHT_TPDM_H > >> +#define _CORESIGHT_CORESIGHT_TPDM_H > >> + > >> +/* Default value of the traceid */ > >> +#define TPDM_TRACE_ID_START 128 > >> + > >> +/** > >> + * struct tpdm_drvdata - specifics associated to an TPDM component > >> + * @base: memory mapped base address for this component. > >> + * @dev: The device entity associated to this component. > >> + * @csdev: component vitals needed by the framework. > >> + * @lock: lock for the enable value. > >> + * @enable: enable status of the component. > >> + * @traceid: value of the current ID for this component. > >> + */ > >> + > >> +struct tpdm_drvdata { > >> + void __iomem *base; > >> + struct device *dev; > >> + struct coresight_device *csdev; > >> + struct mutex lock; > >> + bool enable; > >> + int traceid; > >> +}; > >> + > >> +#endif /* _CORESIGHT_CORESIGHT_TPDM_H */ > >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h > >> index 93a2922b7653..e48d463be63b 100644 > >> --- a/include/linux/coresight.h > >> +++ b/include/linux/coresight.h > >> @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source { > >> CORESIGHT_DEV_SUBTYPE_SOURCE_PROC, > >> CORESIGHT_DEV_SUBTYPE_SOURCE_BUS, > >> CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE, > >> + CORESIGHT_DEV_SUBTYPE_SOURCE_SYS, > >> }; > >> > >> enum coresight_dev_subtype_helper { > >> -- > >> 2.17.1 > >> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

3 years

Re: [PATCH v2 6/7] audit: Use task_is_in_init_pid_ns()

by Leo Yan

On Wed, Dec 15, 2021 at 02:09:12PM -0500, Richard Guy Briggs wrote: > On 2021-12-14 17:35, Paul Moore wrote: > > On Wed, Dec 8, 2021 at 3:33 AM Leo Yan <leo.yan(a)linaro.org> wrote: > > > > > > Replace open code with task_is_in_init_pid_ns() for checking root PID > > > namespace. > > > > > > Signed-off-by: Leo Yan <leo.yan(a)linaro.org> > > > --- > > > kernel/audit.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > I'm not sure how necessary this is, but it looks correct to me. > > I had the same thought. I looks correct to me. I could see the value > if it permitted init_pid_ns to not be global. Just for a background info, we need to check root PID namespace in some drivers [1], to avoid introducing more open codes, we decided to refactor with helper task_is_in_init_pid_ns(). [1] https://lore.kernel.org/lkml/20211213121323.1887180-1-leo.yan@linaro.org/ > > Acked-by: Paul Moore <paul(a)paul-moore.com> > > Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com> Thanks for review, Paul and Richard. Leo

3 years

[PATCH 01/12] perf test: Shell - Limit to only run executable scripts in tests

by carsten.haitzler＠foss.arm.com

From: Carsten Haitzler <carsten.haitzler(a)arm.com> Perf test's shell runner will just run everything in the tests directory (as long as it's not another directory or does not begin with a dot), but sometimes you find files in there that are not shell scripts - perf.data output for example if you do some testing and then the next time you run perf test it tries to run these. Check the files are executable so they are actually intended to be test scripts and not just some "random junk" files there. Signed-off-by: Carsten Haitzler <carsten.haitzler(a)arm.com> --- tools/perf/tests/builtin-test.c | 2 +- tools/perf/util/path.c | 12 ++++++++++++ tools/perf/util/path.h | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c index c4b888f18e9c..1a7e21e5acf1 100644 --- a/tools/perf/tests/builtin-test.c +++ b/tools/perf/tests/builtin-test.c @@ -512,7 +512,7 @@ static const char *shell_test__description(char *description, size_t size, #define for_each_shell_test(dir, base, ent) \ while ((ent = readdir(dir)) != NULL) \ - if (!is_directory(base, ent) && ent->d_name[0] != '.') + if (!is_directory(base, ent) && is_executable_file(base, ent) && ent->d_name[0] != '.') static const char *shell_tests__dir(char *path, size_t size) { diff --git a/tools/perf/util/path.c b/tools/perf/util/path.c index caed0336429f..7dde8c230ae8 100644 --- a/tools/perf/util/path.c +++ b/tools/perf/util/path.c @@ -92,3 +92,15 @@ bool is_directory(const char *base_path, const struct dirent *dent) return S_ISDIR(st.st_mode); } + +bool is_executable_file(const char *base_path, const struct dirent *dent) +{ + char path[PATH_MAX]; + struct stat st; + + sprintf(path, "%s/%s", base_path, dent->d_name); + if (stat(path, &st)) + return false; + + return !S_ISDIR(st.st_mode) && (st.st_mode & S_IXUSR); +} diff --git a/tools/perf/util/path.h b/tools/perf/util/path.h index 083429b7efa3..d94902c22222 100644 --- a/tools/perf/util/path.h +++ b/tools/perf/util/path.h @@ -12,5 +12,6 @@ int path__join3(char *bf, size_t size, const char *path1, const char *path2, con bool is_regular_file(const char *file); bool is_directory(const char *base_path, const struct dirent *dent); +bool is_executable_file(const char *base_path, const struct dirent *dent); #endif /* _PERF_PATH_H */ -- 2.32.0

3 years

Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver

by Mathieu Poirier

On Thu, Dec 09, 2021 at 10:15:36PM +0800, Mao Jinlong wrote: > Add driver to support Coresight device TPDM (Trace, Profiling and > Diagnostics Monitor). TPDM is a monitor to collect data from > different datasets. This change is to add probe/enable/disable > functions for tpdm source. > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com> > --- > MAINTAINERS | 8 + > drivers/hwtracing/coresight/Kconfig | 13 ++ > drivers/hwtracing/coresight/Makefile | 1 + > drivers/hwtracing/coresight/coresight-core.c | 3 +- > drivers/hwtracing/coresight/coresight-tpdm.c | 152 +++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tpdm.h | 31 ++++ > include/linux/coresight.h | 1 + > 7 files changed, 208 insertions(+), 1 deletion(-) > create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c > create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 7a2345ce8521..59f39b3194f6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15560,6 +15560,14 @@ L: netdev(a)vger.kernel.org > S: Supported > F: drivers/net/ipa/ > > +QCOM CORESIGHT COMPONENTS DRIVER > +M: Tao Zhang <quic_taozha(a)quicinc.com> > +M: Jinlong Mao <quic_jinlmao(a)quicinc.com> > +M: Mathieu Poirier <mathieu.poirier(a)linaro.org> > +M: Suzuki K Poulose <suzuki.poulose(a)arm.com> > +S: Maintained > +F: drivers/hwtracing/coresight/coresight-tpdm.c > + There is no need for an extra entry in the MAINTAINERS file. The checkpatch.pl script is smart enough to know when to CC you and Tao every time the TPDM/TPDA drivers are modified. Suzuki and I will simply wait for you guys to add your RB tags before reviewing the patches. I have explained this in the previous revision. > QEMU MACHINE EMULATOR AND VIRTUALIZER SUPPORT > M: Gabriel Somlo <somlo(a)cmu.edu> > M: "Michael S. Tsirkin" <mst(a)redhat.com> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > index 514a9b8086e3..5c506a1cd08f 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -201,4 +201,17 @@ config CORESIGHT_TRBE > > To compile this driver as a module, choose M here: the module will be > called coresight-trbe. > + > +config CORESIGHT_TPDM > + tristate "CoreSight Trace, Profiling & Diagnostics Monitor driver" > + select CORESIGHT_LINKS_AND_SINKS Is this available on 32bit HW as well? If not please make it dependent on ARM64 as it for ETMv4x devices. > + help > + This driver provides support for configuring monitor. Monitors are > + primarily responsible for data set collection and support the > + ability to collect any permutation of data set types. Monitors are > + also responsible for interaction with system cross triggering. > + > + To compile this driver as a module, choose M here: the module will be > + called coresight-tpdm. > + > endif > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > index b6c4a48140ec..e7392a0dddeb 100644 > --- a/drivers/hwtracing/coresight/Makefile > +++ b/drivers/hwtracing/coresight/Makefile > @@ -25,5 +25,6 @@ obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o > obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o > obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o > obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o > +obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o > coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ > coresight-cti-sysfs.o > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index cc6b6cabf85f..a7f1a6f09cfb 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1071,7 +1071,8 @@ static int coresight_validate_source(struct coresight_device *csdev, > } > > if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && > - subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) { > + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && > + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SYS) { > dev_err(&csdev->dev, "wrong device subtype in %s\n", function); > return -EINVAL; > } > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > new file mode 100644 > index 000000000000..f494cef4fb24 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -0,0 +1,152 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/amba/bus.h> > +#include <linux/bitmap.h> > +#include <linux/coresight.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/fs.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regulator/consumer.h> > + > +#include "coresight-priv.h" > +#include "coresight-tpdm.h" > + > +DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); > + > +static int tpdm_enable(struct coresight_device *csdev, > + struct perf_event *event, u32 mode) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + mutex_lock(&drvdata->lock); > + if (drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EBUSY; > + } > + > + drvdata->enable = true; > + mutex_unlock(&drvdata->lock); > + > + dev_info(drvdata->dev, "TPDM tracing enabled\n"); > + return 0; > +} > + > +static void tpdm_disable(struct coresight_device *csdev, > + struct perf_event *event) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return; > + } > + > + drvdata->enable = false; > + mutex_unlock(&drvdata->lock); > + > + dev_info(drvdata->dev, "TPDM tracing disabled\n"); > +} > + > +static int tpdm_trace_id(struct coresight_device *csdev) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + return drvdata->traceid; > +} > + > +static const struct coresight_ops_source tpdm_source_ops = { > + .trace_id = tpdm_trace_id, > + .enable = tpdm_enable, > + .disable = tpdm_disable, > +}; > + > +static const struct coresight_ops tpdm_cs_ops = { > + .source_ops = &tpdm_source_ops, > +}; > + > +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) > +{ > + static int traceid = TPDM_TRACE_ID_START; > + > + drvdata->traceid = traceid++; > +} I have been specific on how to properly do this in the last revision. Given the above about the MAINTAINERS file, I am not sure that I will continue reviewing this set. There is also no need to rush another revision as I won't have the bandwidth to process it before the holidays. Thanks, Mathieu > + > +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > +{ > + struct device *dev = &adev->dev; > + struct coresight_platform_data *pdata; > + struct tpdm_drvdata *drvdata; > + struct coresight_desc desc = { 0 }; > + > + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + pdata = coresight_get_platform_data(dev); > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + adev->dev.platform_data = pdata; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + drvdata->dev = &adev->dev; > + dev_set_drvdata(dev, drvdata); > + > + drvdata->base = devm_ioremap_resource(dev, &adev->res); > + if (!drvdata->base) > + return -ENOMEM; > + > + mutex_init(&drvdata->lock); > + > + desc.type = CORESIGHT_DEV_TYPE_SOURCE; > + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SYS; > + desc.ops = &tpdm_cs_ops; > + desc.pdata = adev->dev.platform_data; > + desc.dev = &adev->dev; > + drvdata->csdev = coresight_register(&desc); > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > + > + tpdm_init_default_data(drvdata); > + pm_runtime_put(&adev->dev); > + > + return 0; > +} > + > +static void __exit tpdm_remove(struct amba_device *adev) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); > + > + coresight_unregister(drvdata->csdev); > +} > + > +static struct amba_id tpdm_ids[] = { > + { > + .id = 0x000f0e00, > + .mask = 0x000fff00, > + }, > + { 0, 0}, > +}; > + > +static struct amba_driver tpdm_driver = { > + .drv = { > + .name = "coresight-tpdm", > + .owner = THIS_MODULE, > + .suppress_bind_attrs = true, > + }, > + .probe = tpdm_probe, > + .id_table = tpdm_ids, > +}; > + > +module_amba_driver(tpdm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > new file mode 100644 > index 000000000000..980ae90ff1c8 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _CORESIGHT_CORESIGHT_TPDM_H > +#define _CORESIGHT_CORESIGHT_TPDM_H > + > +/* Default value of the traceid */ > +#define TPDM_TRACE_ID_START 128 > + > +/** > + * struct tpdm_drvdata - specifics associated to an TPDM component > + * @base: memory mapped base address for this component. > + * @dev: The device entity associated to this component. > + * @csdev: component vitals needed by the framework. > + * @lock: lock for the enable value. > + * @enable: enable status of the component. > + * @traceid: value of the current ID for this component. > + */ > + > +struct tpdm_drvdata { > + void __iomem *base; > + struct device *dev; > + struct coresight_device *csdev; > + struct mutex lock; > + bool enable; > + int traceid; > +}; > + > +#endif /* _CORESIGHT_CORESIGHT_TPDM_H */ > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 93a2922b7653..e48d463be63b 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source { > CORESIGHT_DEV_SUBTYPE_SOURCE_PROC, > CORESIGHT_DEV_SUBTYPE_SOURCE_BUS, > CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE, > + CORESIGHT_DEV_SUBTYPE_SOURCE_SYS, > }; > > enum coresight_dev_subtype_helper { > -- > 2.17.1 >

3 years

Re: [PATCH v2 1/9] Use IDR to maintain all the enabled sources' paths.

by Mathieu Poirier

Hi Mao, On Thu, Dec 09, 2021 at 10:15:35PM +0800, Mao Jinlong wrote: > Use hash length of the source's device name to map to the pointer > of the enabled path. Using IDR will be more efficient than using > the list. And there could be other sources except STM and CPU etms > in the new HWs. It is better to maintain all the paths together. > > Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 76 +++++++------------- > 1 file changed, 26 insertions(+), 50 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..cc6b6cabf85f 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/types.h> > #include <linux/device.h> > +#include <linux/idr.h> > #include <linux/io.h> > #include <linux/err.h> > #include <linux/export.h> > @@ -26,6 +27,12 @@ > static DEFINE_MUTEX(coresight_mutex); > static DEFINE_PER_CPU(struct coresight_device *, csdev_sink); > > +/* > + * Use IDR to map the hash length of the source's device name > + * to the pointer of path for the source > + */ > +static DEFINE_IDR(path_idr); > + > /** > * struct coresight_node - elements of a path, from source to sink > * @csdev: Address of an element. > @@ -36,20 +43,6 @@ struct coresight_node { > struct list_head link; > }; > > -/* > - * When operating Coresight drivers from the sysFS interface, only a single > - * path can exist from a tracer (associated to a CPU) to a sink. > - */ > -static DEFINE_PER_CPU(struct list_head *, tracer_path); > - > -/* > - * As of this writing only a single STM can be found in CS topologies. Since > - * there is no way to know if we'll ever see more and what kind of > - * configuration they will enact, for the time being only define a single path > - * for STM. > - */ > -static struct list_head *stm_path; > - > /* > * When losing synchronisation a new barrier packet needs to be inserted at the > * beginning of the data collected in a buffer. That way the decoder knows that > @@ -1088,10 +1081,11 @@ static int coresight_validate_source(struct coresight_device *csdev, > > int coresight_enable(struct coresight_device *csdev) > { > - int cpu, ret = 0; > + int ret = 0; > struct coresight_device *sink; > struct list_head *path; > enum coresight_dev_subtype_source subtype; > + u32 hash; > > subtype = csdev->subtype.source_subtype; > > @@ -1133,26 +1127,14 @@ int coresight_enable(struct coresight_device *csdev) > if (ret) > goto err_source; > > - switch (subtype) { > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > - /* > - * When working from sysFS it is important to keep track > - * of the paths that were created so that they can be > - * undone in 'coresight_disable()'. Since there can only > - * be a single session per tracer (when working from sysFS) > - * a per-cpu variable will do just fine. > - */ > - cpu = source_ops(csdev)->cpu_id(csdev); > - per_cpu(tracer_path, cpu) = path; > - break; > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: > - stm_path = path; > - break; > - default: > - /* We can't be here */ > - break; > - } > - > + /* > + * Use the hash length of source's device name as ID > + * and map the ID to the pointer of the path. > + */ > + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); > + ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL); > + if (ret) > + goto err_source; > out: > mutex_unlock(&coresight_mutex); > return ret; > @@ -1168,8 +1150,9 @@ EXPORT_SYMBOL_GPL(coresight_enable); > > void coresight_disable(struct coresight_device *csdev) > { > - int cpu, ret; > + int ret; > struct list_head *path = NULL; > + u32 hash; > > mutex_lock(&coresight_mutex); > > @@ -1180,21 +1163,13 @@ void coresight_disable(struct coresight_device *csdev) > if (!csdev->enable || !coresight_disable_source(csdev)) > goto out; > > - switch (csdev->subtype.source_subtype) { > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > - cpu = source_ops(csdev)->cpu_id(csdev); > - path = per_cpu(tracer_path, cpu); > - per_cpu(tracer_path, cpu) = NULL; > - break; > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: > - path = stm_path; > - stm_path = NULL; > - break; > - default: > - /* We can't be here */ > - break; > - } > + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); > + /* Find the path by the hash length. */ > + path = idr_find(&path_idr, hash); > + if (path == NULL) > + return; Please add a dev_err() here as this should really not be happening. > > + idr_remove(&path_idr, hash); > coresight_disable_path(path); > coresight_release_path(path); > > @@ -1779,6 +1754,7 @@ static int __init coresight_init(void) > > static void __exit coresight_exit(void) > { > + idr_destroy(&path_idr); As far as I can tell this isn't needed. > cscfg_exit(); > etm_perf_exit(); > bus_unregister(&coresight_bustype); > -- > 2.17.1 >

3 years

[PATCH v2 0/4] coresight: etm: Correct PID tracing for non-root namespace

by Leo Yan

If a profiling program runs in a non-root PID namespace, if CoreSight driver enables PID tracing (with contextID), it can lead to mismatching issue between the context ID traced in hardware (from the root namespace) and the PIDs gathered by profiling tool (e.g. perf) in its non-root namespace. CoreSight driver has tried to address this issue for the contextID related interfaces under sysfs, but it misses to prevent user to set VMID (virtual contextID) for kernel runs in EL2 with VHE; furthermore, it misses to handle the case when the profiling tool runs in the non-root PID namespace. For this reason, this patch series is to correct contextID tracing for non-root namespace. After applied this patchset, patch 02 doesn't permit users to access virtual contextID via sysfs nodes in the non-root PID namespace, patch 03 and 04 stop to trace PID packet for non-root PID namespace. This patch is dependent on the patchset "pid: Introduce helper task_is_in_root_ns()" [1]. [1] https://lore.kernel.org/lkml/20211208083320.472503-1-leo.yan@linaro.org/ Leo Yan (4): coresight: etm4x: Add lock for reading virtual context ID comparator coresight: etm4x: Don't use virtual contextID for non-root PID namespace coresight: etm4x: Don't trace PID for non-root PID namespace coresight: etm3x: Don't trace PID for non-root PID namespace .../coresight/coresight-etm3x-core.c | 4 +++ .../coresight/coresight-etm4x-core.c | 10 +++++-- .../coresight/coresight-etm4x-sysfs.c | 30 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) -- 2.25.1

3 years

Re: [PATCH v2 4/7] connector/cn_proc: Use task_is_in_init_pid_ns()

by Leo Yan

On Tue, Dec 14, 2021 at 04:58:32PM +1100, Balbir Singh wrote: > On Wed, Dec 08, 2021 at 04:33:17PM +0800, Leo Yan wrote: > > This patch replaces open code with task_is_in_init_pid_ns() to check if > > a task is in root PID namespace. > > > > Signed-off-by: Leo Yan <leo.yan(a)linaro.org> > > --- > > drivers/connector/cn_proc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c > > index 646ad385e490..ccac1c453080 100644 > > --- a/drivers/connector/cn_proc.c > > +++ b/drivers/connector/cn_proc.c > > @@ -358,7 +358,7 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, > > * other namespaces. > > */ > > if ((current_user_ns() != &init_user_ns) || > > - (task_active_pid_ns(current) != &init_pid_ns)) > > + !task_is_in_init_pid_ns(current)) > > return; > > > > Sounds like there might scope for other wrappers - is_current_in_user_init_ns() Indeed, for new wrapper is_current_in_user_init_ns(), I searched kernel and located there have multiple places use open code. If no one works on this refactoring, I will send a new patchset for it separately. > Acked-by: Balbir Singh <bsingharora(a)gmail.com> Thank you for review, Balbir! Also thanks review from Leon and Suzuki. Leo

3 years

Re: [PATCH v2 4/4] coresight: etm3x: Don't trace PID for non-root PID namespace

by Leo Yan

Hi, On Tue, Dec 14, 2021 at 03:16:42AM +0800, kernel test robot wrote: > Hi Leo, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [also build test ERROR on v5.16-rc5] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Leo-Yan/coresight-etm-Correct-PID-… > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2585cf9dfaaddf00b069673f27bb3f8530e2039c > config: arm-buildonly-randconfig-r003-20211213 (https://download.01.org/0day-ci/archive/20211214/202112140344.viPmOWp6-lkp@…) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a2ddb6c8ac29412b1361810972e15221fa021c) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install arm cross compiling tool for clang build > # apt-get install binutils-arm-linux-gnueabi > # https://github.com/0day-ci/linux/commit/81d5f47788c40d34c8159d64d4505eb4852… > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Leo-Yan/coresight-etm-Correct-PID-tracing-for-non-root-namespace/20211213-201632 > git checkout 81d5f47788c40d34c8159d64d4505eb485254e8f > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwtracing/coresight/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp(a)intel.com> > > All errors (new ones prefixed by >>): > > >> drivers/hwtracing/coresight/coresight-etm3x-core.c:344:7: error: implicit declaration of function 'task_is_in_init_pid_ns' [-Werror,-Wimplicit-function-declaration] > if (!task_is_in_init_pid_ns(current)) > ^ > 1 error generated. > > > vim +/task_is_in_init_pid_ns +344 drivers/hwtracing/coresight/coresight-etm3x-core.c > > 301 > 302 #define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \ > 303 ETMCR_TIMESTAMP_EN | \ > 304 ETMCR_RETURN_STACK) > 305 > 306 static int etm_parse_event_config(struct etm_drvdata *drvdata, > 307 struct perf_event *event) > 308 { > 309 struct etm_config *config = &drvdata->config; > 310 struct perf_event_attr *attr = &event->attr; > 311 > 312 if (!attr) > 313 return -EINVAL; > 314 > 315 /* Clear configuration from previous run */ > 316 memset(config, 0, sizeof(struct etm_config)); > 317 > 318 if (attr->exclude_kernel) > 319 config->mode = ETM_MODE_EXCL_KERN; > 320 > 321 if (attr->exclude_user) > 322 config->mode = ETM_MODE_EXCL_USER; > 323 > 324 /* Always start from the default config */ > 325 etm_set_default(config); > 326 > 327 /* > 328 * By default the tracers are configured to trace the whole address > 329 * range. Narrow the field only if requested by user space. > 330 */ > 331 if (config->mode) > 332 etm_config_trace_mode(config); > 333 > 334 /* > 335 * At this time only cycle accurate, return stack and timestamp > 336 * options are available. > 337 */ > 338 if (attr->config & ~ETM3X_SUPPORTED_OPTIONS) > 339 return -EINVAL; > 340 > 341 config->ctrl = attr->config; > 342 > 343 /* Don't trace contextID when runs in non-root PID namespace */ > > 344 if (!task_is_in_init_pid_ns(current)) This patchset is based on another patchset [1], as described on the cover letter patch. This is why here reports for building failure. To avoid the false positive reporting, if any better practice I can follow up to resolve the dependency between two patchsets, please let me know and I will do in next time. Thanks, Leo [1] https://lore.kernel.org/lkml/20211208083320.472503-1-leo.yan@linaro.org/ > 345 config->ctrl &= ~ETMCR_CTXID_SIZE; > 346 > 347 /* > 348 * Possible to have cores with PTM (supports ret stack) and ETM > 349 * (never has ret stack) on the same SoC. So if we have a request > 350 * for return stack that can't be honoured on this core then > 351 * clear the bit - trace will still continue normally > 352 */ > 353 if ((config->ctrl & ETMCR_RETURN_STACK) && > 354 !(drvdata->etmccer & ETMCCER_RETSTACK)) > 355 config->ctrl &= ~ETMCR_RETURN_STACK; > 356 > 357 return 0; > 358 } > 359 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

3 years

Re: [PATCH] coresight: core: fix typo in a comment

by Mathieu Poirier

On Sat, Dec 11, 2021 at 05:02:21PM +0800, Jason Wang wrote: > The double `the' in the comment in line 732 is repeated. Remove one > of them from the comment. > > Signed-off-by: Jason Wang <wangborong(a)cdjrlc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..88653d1c06a4 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -729,7 +729,7 @@ static inline void coresight_put_ref(struct coresight_device *csdev) > * coresight_grab_device - Power up this device and any of the helper > * devices connected to it for trace operation. Since the helper devices > * don't appear on the trace path, they should be handled along with the > - * the master device. > + * master device. Applied. Thanks, Mathieu > */ > static int coresight_grab_device(struct coresight_device *csdev) > { > -- > 2.34.1 >

3 years

Re: [PATCH 00/22] Refactor perf cpumap

by James Clark

On 08/12/2021 02:45, Ian Rogers wrote: > Perf cpu map has various functions where a cpumap and index are passed > in order to load the cpu. A problem with this is that the wrong index > may be passed for the cpumap, causing problems like aggregation on the > wrong CPU: > https://lore.kernel.org/lkml/20211204023409.969668-1-irogers@google.com/ > > This patch set refactors the cpu map API, greatly reducing it and > explicitly passing the cpu (rather than the pair) to functions that > need it. Comments are added at the same time. > > Ian Rogers (22): > libperf: Add comments to perf_cpu_map. > perf stat: Add aggr creators that are passed a cpu. > perf stat: Switch aggregation to use for_each loop > perf stat: Switch to cpu version of cpu_map__get > perf cpumap: Switch cpu_map__build_map to cpu function > perf cpumap: Remove map+index get_socket > perf cpumap: Remove map+index get_die > perf cpumap: Remove map+index get_core > perf cpumap: Remove map+index get_node > perf cpumap: Add comments to aggr_cpu_id > perf cpumap: Remove unused cpu_map__socket > perf cpumap: Simplify equal function name. > perf cpumap: Rename empty functions. > perf cpumap: Document cpu__get_node and remove redundant function > perf cpumap: Remove map from function names that don't use a map. > perf cpumap: Remove cpu_map__cpu, use libperf function. > perf cpumap: Refactor cpu_map__build_map > perf cpumap: Rename cpu_map__get_X_aggr_by_cpu functions > perf cpumap: Move 'has' function to libperf > perf cpumap: Add some comments to cpu_aggr_map > perf cpumap: Trim the cpu_aggr_map > perf stat: Fix memory leak in check_per_pkg > > tools/lib/perf/cpumap.c | 7 +- > tools/lib/perf/include/internal/cpumap.h | 9 +- > tools/lib/perf/include/perf/cpumap.h | 1 + > tools/perf/arch/arm/util/cs-etm.c | 16 +- > tools/perf/builtin-ftrace.c | 2 +- > tools/perf/builtin-sched.c | 6 +- > tools/perf/builtin-stat.c | 273 ++++++++++++----------- > tools/perf/tests/topology.c | 10 +- > tools/perf/util/cpumap.c | 182 ++++++--------- > tools/perf/util/cpumap.h | 102 ++++++--- > tools/perf/util/cputopo.c | 2 +- > tools/perf/util/env.c | 6 +- > tools/perf/util/stat-display.c | 69 +++--- > tools/perf/util/stat.c | 9 +- > tools/perf/util/stat.h | 3 +- > 15 files changed, 361 insertions(+), 336 deletions(-) > For the whole set: Reviewed-by: James Clark <james.clark(a)arm.com> I didn't see any obvious issues with mixing up aggregation modes or CPU/idx types. Also gave perf stat a test in the different modes and didn't see an issue. But I'm wondering if it's possible to go further and add a struct around the CPU int so that the compiler checks for correctness instead. It still seems quite easy to mix up index and CPU, for example these functions are subtly different, but both use int: LIBPERF_API int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx); LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, int cpu); Something like this would make it impossible to make a mistake: struct cpu { int cpu }; I mean it's more of a coincidence that CPUs can be identified by an integer, but they are more of an object than an integer, so it could make sense to wrap it. But maybe it could be quite cumbersome to use and be overkill.

3 years

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight December 2021