CoreSight

coresight@lists.linaro.org

4 participants
2607 discussions

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.

4 years, 1 month

Re: [PATCH 02/22] perf stat: Add aggr creators that are passed a cpu.

by Arnaldo Carvalho de Melo

Em Tue, Dec 07, 2021 at 06:45:47PM -0800, Ian Rogers escreveu: > The cpu_map and index can get confused. Add variants of the cpu_map__get > routines that are passed a cpu. Make the existing cpu_map__get routines > use the new functions with a view to remove them when no longer used. Looks ok from a quick lock > Signed-off-by: Ian Rogers <irogers(a)google.com> > --- > tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++----------------- > tools/perf/util/cpumap.h | 6 ++- > 2 files changed, 51 insertions(+), 34 deletions(-) > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c > index 87d3eca9b872..49fba2c53822 100644 > --- a/tools/perf/util/cpumap.c > +++ b/tools/perf/util/cpumap.c > @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu) > return ret ?: value; > } > > -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, > - void *data __maybe_unused) > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused) > { > - int cpu; > struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > - > id.socket = cpu_map__get_socket_id(cpu); > return id; > } > > +struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, > + void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_socket_aggr_by_cpu(map->map[idx], data); > +} > + > static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer) > { > struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer; > @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu) > return ret ?: value; > } > > -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data) > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data) > { > - int cpu, die; > - struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > - > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > + struct aggr_cpu_id id; > + int die; > > die = cpu_map__get_die_id(cpu); > /* There is no die_id on legacy system. */ > @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat > * with the socket ID and then add die to > * make a unique ID. > */ > - id = cpu_map__get_socket(map, idx, data); > + id = cpu_map__get_socket_aggr_by_cpu(cpu, data); > if (cpu_map__aggr_cpu_id_is_empty(id)) > return id; > > @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat > return id; > } > > +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, > + void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_die_aggr_by_cpu(map->map[idx], data); > +} > + > int cpu_map__get_core_id(int cpu) > { > int value, ret = cpu__get_topology_int(cpu, "core_id", &value); > @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu) > return cpu__get_node(cpu); > } > > -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data) > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data) > { > - int cpu; > - struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > - > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > - > - cpu = cpu_map__get_core_id(cpu); > + struct aggr_cpu_id id; > + int core = cpu_map__get_core_id(cpu); > > /* cpu_map__get_die returns a struct with socket and die set*/ > - id = cpu_map__get_die(map, idx, data); > + id = cpu_map__get_die_aggr_by_cpu(cpu, data); > if (cpu_map__aggr_cpu_id_is_empty(id)) > return id; > > @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da > * core_id is relative to socket and die, we need a global id. > * So we combine the result from cpu_map__get_die with the core id > */ > - id.core = cpu; > + id.core = core; > return id; > + > } > > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused) > +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_core_aggr_by_cpu(map->map[idx], data); > +} > + > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused) > { > struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > > + id.node = cpu_map__get_node_id(cpu); > + return id; > +} > + > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data) > +{ > if (idx < 0 || idx >= map->nr) > - return id; > + return cpu_map__empty_aggr_cpu_id(); > > - id.node = cpu_map__get_node_id(map->map[idx]); > - return id; > + return cpu_map__get_node_aggr_by_cpu(map->map[idx], data); > } > > int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp) > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h > index a27eeaf086e8..c62d67704425 100644 > --- a/tools/perf/util/cpumap.h > +++ b/tools/perf/util/cpumap.h > @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size); > size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size); > size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp); > int cpu_map__get_socket_id(int cpu); > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_die_id(int cpu); > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_core_id(int cpu); > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_node_id(int cpu); > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data); > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data); > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp); > int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep); > int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep); > -- > 2.34.1.400.ga245620fadb-goog -- - Arnaldo

4 years, 1 month

Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map.

by Arnaldo Carvalho de Melo

Em Wed, Dec 08, 2021 at 06:34:14AM -0800, Ian Rogers escreveu: > On Wed, Dec 8, 2021 at 4:06 AM John Garry <john.garry(a)huawei.com> wrote: > > > > On 08/12/2021 02:45, Ian Rogers wrote: > > > diff --git a/tools/lib/perf/include/internal/cpumap.h b/tools/lib/perf/include/internal/cpumap.h > > > index 840d4032587b..1c1726f4a04e 100644 > > > --- a/tools/lib/perf/include/internal/cpumap.h > > > +++ b/tools/lib/perf/include/internal/cpumap.h > > > @@ -4,9 +4,16 @@ > > > > > > #include <linux/refcount.h> > > > > > > +/** > > > + * A sized, reference counted, sorted array of integers representing CPU > > > + * numbers. This is commonly used to capture which CPUs a PMU is associated > > > + * with. > > > + */ > > > struct perf_cpu_map { > > > refcount_t refcnt; > > > + /** Length of the map array. */ > > > int nr; > > > + /** The CPU values. */ > > > int map[]; > > > > would simply more distinct names for the variables help instead of or in > > addition to comments? Well, in this case the typical usage doesn't help, as 'struct perf_cpu_map' are being used simply as "map" where it should be cpu_map, so we would have: cpu_map->nr And all should be obvious, no? Otherwise we would have redundant 'cpu', like: cpu_map->nr_cpus And 'map' should really be entries, so: cpu_map->entries[index]; Would be clear enough, o? > Thanks John! I agree. The phrase that is often used is intention > revealing names. The kernel style for naming is to be brief: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#naming > These names are both brief. nr is a little unusual, of course an > integer is a number - size and length are common names in situations > like these. In this case number makes sense as it is the number of > CPUs in the array, and there is a certain readability in saying number > of CPUs and not length or size of CPUs. The name map I have issue > with, it is always a smell if you are calling a variable a data type. > Given the convention in the context of this code I decided to leave > it. Something like array_of_cpu_values would be more intention > revealing but when run through the variable name shrinkifier could end > up as just being array, which would be little better than map. > > The guidance on comments is that they are good and to focus on the > what of what the code is doing: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting > refcnt was intention revealing enough and so I didn't add a comment to it. > > > Generally developers don't always check comments where the struct is > > defined when the meaning could be judged intuitively > > Agreed. I think there could be a follow up to change to better names. > As I was lacking a better suggestion I think for the time being, and > in this patch set, we can keep things as they are. > > Thanks, > Ian > > > Thanks, > > John > > -- - Arnaldo

4 years, 1 month

[PATCH] perf cs-etm: Remove duplicate and incorrect aux size checks

by James Clark

There are two checks, one is for size when running without admin, but this one is covered by the driver and reported on in more detail here (builtin-record.c): pr_err("Permission error mapping pages.\n" "Consider increasing " "/proc/sys/kernel/perf_event_mlock_kb,\n" "or try again with a smaller value of -m/--mmap_pages.\n" "(current value: %u,%u)\n", This had the effect of artificially limiting the aux buffer size to a value smaller than what was allowed because perf_event_mlock_kb wasn't taken into account. The second is to check for a power of two, but this is covered here (evlist.c): pr_info("rounding mmap pages size to %s (%lu pages)\n", buf, pages); Signed-off-by: James Clark <james.clark(a)arm.com> --- tools/perf/arch/arm/util/cs-etm.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index 293a23bf8be3..8a3d54a86c9c 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -407,25 +407,6 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, } - /* Validate auxtrace_mmap_pages provided by user */ - if (opts->auxtrace_mmap_pages) { - unsigned int max_page = (KiB(128) / page_size); - size_t sz = opts->auxtrace_mmap_pages * (size_t)page_size; - - if (!privileged && - opts->auxtrace_mmap_pages > max_page) { - opts->auxtrace_mmap_pages = max_page; - pr_err("auxtrace too big, truncating to %d\n", - max_page); - } - - if (!is_power_of_2(sz)) { - pr_err("Invalid mmap size for %s: must be a power of 2\n", - CORESIGHT_ETM_PMU_NAME); - return -EINVAL; - } - } - if (opts->auxtrace_snapshot_mode) pr_debug2("%s snapshot size: %zu\n", CORESIGHT_ETM_PMU_NAME, opts->auxtrace_snapshot_size); -- 2.28.0

4 years, 1 month

Re: [RFC PATCH] Change coresight tmc from misc device to cdev device

by Mathieu Poirier

Hi Jay, On Wed, Dec 01, 2021 at 03:09:42PM +0800, Jay Chen wrote: > For coresight in the server scenario, if there are too many > cpu cores and too many tmc peripherals, the error of insufficient > device numbers will occur by misc device > There really is more than 128 miscdevice allocated in your system? How may TMC do you have? > Signed-off-by: Jay Chen <jkchen(a)linux.alibaba.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 53 +++++++++++++++++++ > .../hwtracing/coresight/coresight-tmc-core.c | 13 ++--- > drivers/hwtracing/coresight/coresight-tmc.h | 4 +- > include/linux/coresight.h | 23 ++++++++ > 4 files changed, 83 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..b0cca3060f4e 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -17,6 +17,7 @@ > #include <linux/coresight.h> > #include <linux/of_platform.h> > #include <linux/delay.h> > +#include <linux/cdev.h> > #include <linux/pm_runtime.h> > > #include "coresight-etm-perf.h" > @@ -1567,6 +1568,58 @@ void coresight_release_platform_data(struct coresight_device *csdev, > coresight_remove_conns_sysfs_group(csdev); > } > > +int coresight_cdev_register(struct coresight_cdev *cdev, const struct file_operations *fops, > + const char *name) > +{ > + struct device *device; > + > + if (alloc_chrdev_region(&cdev->devno, 0, 1, name)) { > + pr_err("failed to create the coresight cdev region\n"); > + return -EFAULT; > + } > + > + cdev->class = class_create(THIS_MODULE, name); > + if (IS_ERR(cdev->class)) { > + pr_err("failed to create the coresight class\n"); > + goto coresight_unregister_region; > + } >From the above a new class is created for every TMC, even if they are all the same. A better way to do this would be to create a single TMC class with a new cdev for every new TMC. To do this alloc_chrdev_region() and class_create() have to be called when the module is added to the system. Have a look at funnel_init() on how to refactor the TMC core driver with an init function and rpmsg_chrdev_init() for the chrdev region allocation and the class creation. >From there every time a TMC is probe cdev_init() and cdev_add() can be called - look at rpmsg_chrdev_probe() for an example. Instead of using an IDA to pick out a minor number you should be able to use dev_list->nr_idx - 1. Thanks, Mathieu > + > + cdev_init(&cdev->cdev, fops); > + if (cdev_add(&cdev->cdev, cdev->devno, 1)) { > + pr_err("failed to add the coresight cdev\n"); > + goto coresight_free_class; > + } > + > + device = device_create(cdev->class, NULL, cdev->devno, > + cdev, name); > + if (IS_ERR(device)) { > + pr_err("failed to create device for coresight\n"); > + goto coresight_del_cdev; > + } > + > + return 0; > + > +coresight_del_cdev: > + cdev_del(&cdev->cdev); > +coresight_free_class: > + class_destroy(cdev->class); > +coresight_unregister_region: > + unregister_chrdev_region(cdev->devno, 1); > + > + return -EFAULT; > +} > +EXPORT_SYMBOL_GPL(coresight_cdev_register); > + > +int coresight_cdev_unregister(struct coresight_cdev *cdev) > +{ > + device_destroy(cdev->class, cdev->devno); > + cdev_del(&cdev->cdev); > + class_destroy(cdev->class); > + unregister_chrdev_region(cdev->devno, 1); > + return 0; > +} > +EXPORT_SYMBOL_GPL(coresight_cdev_unregister); > + > struct coresight_device *coresight_register(struct coresight_desc *desc) > { > int ret; > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > index 74c6323d4d6a..c4aae8ff65eb 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -147,7 +147,7 @@ static int tmc_open(struct inode *inode, struct file *file) > { > int ret; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > > ret = tmc_read_prepare(drvdata); > if (ret) > @@ -179,7 +179,7 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, > char *bufp; > ssize_t actual; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); > if (actual <= 0) > return 0; > @@ -200,7 +200,7 @@ static int tmc_release(struct inode *inode, struct file *file) > { > int ret; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > > ret = tmc_read_unprepare(drvdata); > if (ret) > @@ -529,10 +529,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > goto out; > } > > - drvdata->miscdev.name = desc.name; > - drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; > - drvdata->miscdev.fops = &tmc_fops; > - ret = misc_register(&drvdata->miscdev); > + ret = coresight_cdev_register(&drvdata->cdev, &tmc_fops, desc.name); > if (ret) > coresight_unregister(drvdata->csdev); > else > @@ -572,7 +569,7 @@ static void tmc_remove(struct amba_device *adev) > * etb fops in this case, device is there until last file > * handler to this device is closed. > */ > - misc_deregister(&drvdata->miscdev); > + coresight_cdev_unregister(&drvdata->cdev); > coresight_unregister(drvdata->csdev); > } > > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index b91ec7dde7bc..c37e2ed1cb9d 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -166,7 +166,7 @@ struct etr_buf { > * struct tmc_drvdata - specifics associated to an TMC component > * @base: memory mapped base address for this component. > * @csdev: component vitals needed by the framework. > - * @miscdev: specifics to handle "/dev/xyz.tmc" entry. > + * @cdev: specifics to handle "/dev/xyz.tmc" entry. > * @spinlock: only one at a time pls. > * @pid: Process ID of the process being monitored by the session > * that is using this component. > @@ -188,7 +188,7 @@ struct etr_buf { > struct tmc_drvdata { > void __iomem *base; > struct coresight_device *csdev; > - struct miscdevice miscdev; > + struct coresight_cdev cdev; > spinlock_t spinlock; > pid_t pid; > bool reading; > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 93a2922b7653..aebe8b8327ad 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -9,6 +9,7 @@ > #include <linux/device.h> > #include <linux/io.h> > #include <linux/perf_event.h> > +#include <linux/cdev.h> > #include <linux/sched.h> > > /* Peripheral id registers (0xFD0-0xFEC) */ > @@ -252,6 +253,12 @@ struct coresight_device { > void *active_cscfg_ctxt; > }; > > +struct coresight_cdev { > + dev_t devno; > + struct class *class; > + struct cdev cdev; > +}; > + > /* > * coresight_dev_list - Mapping for devices to "name" index for device > * names. > @@ -506,6 +513,10 @@ void coresight_relaxed_write64(struct coresight_device *csdev, > u64 val, u32 offset); > void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset); > > +int coresight_cdev_register(struct coresight_cdev *cdev, const struct file_operations *fops, > + const char *name); > + > +int coresight_cdev_unregister(struct coresight_cdev *cdev); > #else > static inline struct coresight_device * > coresight_register(struct coresight_desc *desc) { return NULL; } > @@ -581,6 +592,18 @@ static inline void coresight_write64(struct coresight_device *csdev, u64 val, u3 > { > } > > +static inline int coresight_cdev_register(struct coresight_cdev *cdev, > + const struct file_operations *fops, > + const char *name) > +{ > + return 0; > +} > + > +static inline int coresight_cdev_unregister(struct coresight_cdev *cdev) > +{ > + return 0; > +} > + > #endif /* IS_ENABLED(CONFIG_CORESIGHT) */ > > extern int coresight_get_cpu(struct device *dev); > -- > 2.27.0 >

4 years, 1 month

Re: [PATCH 19/22] perf cpumap: Move 'has' function to libperf

by Mathieu Poirier

On Tue, Dec 07, 2021 at 06:46:04PM -0800, Ian Rogers wrote: > Make the cpu map argument const for consistency with the rest of the > API. Modify cpu_map__idx accordingly. > > Signed-off-by: Ian Rogers <irogers(a)google.com> > --- > tools/lib/perf/cpumap.c | 7 ++++++- > tools/lib/perf/include/internal/cpumap.h | 2 +- > tools/lib/perf/include/perf/cpumap.h | 1 + > tools/perf/arch/arm/util/cs-etm.c | 16 ++++++++-------- For the coresight part: Reviewed-by: Mathieu Poirier <mathieu.poirier(a)linaro.org> > tools/perf/builtin-sched.c | 6 +++--- > tools/perf/tests/topology.c | 2 +- > tools/perf/util/cpumap.c | 5 ----- > tools/perf/util/cpumap.h | 2 -- > tools/perf/util/cputopo.c | 2 +- > 9 files changed, 21 insertions(+), 22 deletions(-) > > diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c > index adaad3dddf6e..3c36a06771af 100644 > --- a/tools/lib/perf/cpumap.c > +++ b/tools/lib/perf/cpumap.c > @@ -268,7 +268,7 @@ bool perf_cpu_map__empty(const struct perf_cpu_map *map) > return map ? map->map[0] == -1 : true; > } > > -int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu) > +int perf_cpu_map__idx(const struct perf_cpu_map *cpus, int cpu) > { > int low = 0, high = cpus->nr; > > @@ -288,6 +288,11 @@ int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu) > return -1; > } > > +bool perf_cpu_map__has(const struct perf_cpu_map *cpus, int cpu) > +{ > + return perf_cpu_map__idx(cpus, cpu) != -1; > +} > + > int perf_cpu_map__max(struct perf_cpu_map *map) > { > // cpu_map__trim_new() qsort()s it, cpu_map__default_new() sorts it as well. > diff --git a/tools/lib/perf/include/internal/cpumap.h b/tools/lib/perf/include/internal/cpumap.h > index 1c1726f4a04e..6a0ff0abcbc4 100644 > --- a/tools/lib/perf/include/internal/cpumap.h > +++ b/tools/lib/perf/include/internal/cpumap.h > @@ -21,6 +21,6 @@ struct perf_cpu_map { > #define MAX_NR_CPUS 2048 > #endif > > -int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu); > +int perf_cpu_map__idx(const struct perf_cpu_map *cpus, int cpu); > > #endif /* __LIBPERF_INTERNAL_CPUMAP_H */ > diff --git a/tools/lib/perf/include/perf/cpumap.h b/tools/lib/perf/include/perf/cpumap.h > index 7c27766ea0bf..3f1c0afa3ccd 100644 > --- a/tools/lib/perf/include/perf/cpumap.h > +++ b/tools/lib/perf/include/perf/cpumap.h > @@ -20,6 +20,7 @@ LIBPERF_API int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx); > LIBPERF_API int perf_cpu_map__nr(const struct perf_cpu_map *cpus); > LIBPERF_API bool perf_cpu_map__empty(const struct perf_cpu_map *map); > LIBPERF_API int perf_cpu_map__max(struct perf_cpu_map *map); > +LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, int cpu); > > #define perf_cpu_map__for_each_cpu(cpu, idx, cpus) \ > for ((idx) = 0, (cpu) = perf_cpu_map__cpu(cpus, idx); \ > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c > index 293a23bf8be3..76c66780617c 100644 > --- a/tools/perf/arch/arm/util/cs-etm.c > +++ b/tools/perf/arch/arm/util/cs-etm.c > @@ -204,8 +204,8 @@ static int cs_etm_set_option(struct auxtrace_record *itr, > > /* Set option of each CPU we have */ > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(event_cpus, i) || > - !cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(event_cpus, i) || > + !perf_cpu_map__has(online_cpus, i)) > continue; > > if (option & BIT(ETM_OPT_CTXTID)) { > @@ -542,8 +542,8 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused, > /* cpu map is not empty, we have specific CPUs to work with */ > if (!perf_cpu_map__empty(event_cpus)) { > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(event_cpus, i) || > - !cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(event_cpus, i) || > + !perf_cpu_map__has(online_cpus, i)) > continue; > > if (cs_etm_is_ete(itr, i)) > @@ -556,7 +556,7 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused, > } else { > /* get configuration for all CPUs in the system */ > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(online_cpus, i)) > continue; > > if (cs_etm_is_ete(itr, i)) > @@ -741,8 +741,8 @@ static int cs_etm_info_fill(struct auxtrace_record *itr, > } else { > /* Make sure all specified CPUs are online */ > for (i = 0; i < perf_cpu_map__nr(event_cpus); i++) { > - if (cpu_map__has(event_cpus, i) && > - !cpu_map__has(online_cpus, i)) > + if (perf_cpu_map__has(event_cpus, i) && > + !perf_cpu_map__has(online_cpus, i)) > return -EINVAL; > } > > @@ -763,7 +763,7 @@ static int cs_etm_info_fill(struct auxtrace_record *itr, > offset = CS_ETM_SNAPSHOT + 1; > > for (i = 0; i < cpu__max_cpu() && offset < priv_size; i++) > - if (cpu_map__has(cpu_map, i)) > + if (perf_cpu_map__has(cpu_map, i)) > cs_etm_get_metadata(i, &offset, itr, info); > > perf_cpu_map__put(online_cpus); > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c > index 4527f632ebe4..9da1da4749c9 100644 > --- a/tools/perf/builtin-sched.c > +++ b/tools/perf/builtin-sched.c > @@ -1617,10 +1617,10 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel, > if (curr_thread && thread__has_color(curr_thread)) > pid_color = COLOR_PIDS; > > - if (sched->map.cpus && !cpu_map__has(sched->map.cpus, cpu)) > + if (sched->map.cpus && !perf_cpu_map__has(sched->map.cpus, cpu)) > continue; > > - if (sched->map.color_cpus && cpu_map__has(sched->map.color_cpus, cpu)) > + if (sched->map.color_cpus && perf_cpu_map__has(sched->map.color_cpus, cpu)) > cpu_color = COLOR_CPUS; > > if (cpu != this_cpu) > @@ -1639,7 +1639,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel, > color_fprintf(stdout, color, " "); > } > > - if (sched->map.cpus && !cpu_map__has(sched->map.cpus, this_cpu)) > + if (sched->map.cpus && !perf_cpu_map__has(sched->map.cpus, this_cpu)) > goto out; > > timestamp__scnprintf_usec(timestamp, stimestamp, sizeof(stimestamp)); > diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c > index 0cb7b015b4b9..cb29ea7ec409 100644 > --- a/tools/perf/tests/topology.c > +++ b/tools/perf/tests/topology.c > @@ -112,7 +112,7 @@ static int check_cpu_topology(char *path, struct perf_cpu_map *map) > TEST_ASSERT_VAL("Session header CPU map not set", session->header.env.cpu); > > for (i = 0; i < session->header.env.nr_cpus_avail; i++) { > - if (!cpu_map__has(map, i)) > + if (!perf_cpu_map__has(map, i)) > continue; > pr_debug("CPU %d, core %d, socket %d\n", i, > session->header.env.cpu[i].core_id, > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c > index 0e325559c33c..8a72ee996722 100644 > --- a/tools/perf/util/cpumap.c > +++ b/tools/perf/util/cpumap.c > @@ -463,11 +463,6 @@ int cpu__setup_cpunode_map(void) > return 0; > } > > -bool cpu_map__has(struct perf_cpu_map *cpus, int cpu) > -{ > - return perf_cpu_map__idx(cpus, cpu) != -1; > -} > - > size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size) > { > int i, cpu, start = -1; > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h > index 15043e764fa6..832fc53f3c11 100644 > --- a/tools/perf/util/cpumap.h > +++ b/tools/perf/util/cpumap.h > @@ -78,8 +78,6 @@ struct cpu_aggr_map *cpu_aggr_map__new(const struct perf_cpu_map *cpus, > aggr_cpu_id_get_t f, > void *data); > > -bool cpu_map__has(struct perf_cpu_map *cpus, int cpu); > - > bool aggr_cpu_id__equal(const struct aggr_cpu_id *a, const struct aggr_cpu_id *b); > bool aggr_cpu_id__is_empty(const struct aggr_cpu_id *a); > struct aggr_cpu_id aggr_cpu_id__empty(void); > diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c > index 51b429c86f98..8affb37d90e7 100644 > --- a/tools/perf/util/cputopo.c > +++ b/tools/perf/util/cputopo.c > @@ -218,7 +218,7 @@ struct cpu_topology *cpu_topology__new(void) > tp->core_cpus_list = addr; > > for (i = 0; i < nr; i++) { > - if (!cpu_map__has(map, i)) > + if (!perf_cpu_map__has(map, i)) > continue; > > ret = build_cpu_topology(tp, i); > -- > 2.34.1.400.ga245620fadb-goog >

4 years, 1 month

Re: [PATCH v1 1/7] pid: Introduce helper task_is_in_root_ns()

by Leo Yan

Hi Leon, On Mon, Dec 06, 2021 at 08:49:01AM +0200, Leon Romanovsky wrote: > On Sun, Dec 05, 2021 at 10:50:59PM +0800, Leo Yan wrote: [...] > > +static inline bool task_is_in_root_ns(struct task_struct *tsk) > > It is bad that this name doesn't reflect PID nature of this namespace. > Won't it better to name it task_is_in_init_pid_ns()? Yes, task_is_in_init_pid_ns() is more clear. Will respin for this. Thank you for suggestion! Leo

4 years, 1 month

[PATCH v1 0/7] pid: Introduce helper task_is_in_root_ns()

by Leo Yan

The kernel uses open code to check if a process is in root PID namespace or not in several places. Suggested by Suzuki, this patch set is to create a helper function task_is_in_root_ns() so we can use it replace open code. To test this patch set, I built Arm64 kernel with enabling all relevant modules, and verified the kernel with CoreSight module on Arm64 Juno board. Leo Yan (7): pid: Introduce helper task_is_in_root_ns() coresight: etm3x: Use task_is_in_root_ns() to check PID namespace coresight: etm4x: Use task_is_in_root_ns() to check PID namespace connector/cn_proc: Use task_is_in_root_ns() to check PID namespace coda: Use task_is_in_root_ns() audit: Use task_is_in_root_ns() taskstats: Use task_is_in_root_ns() drivers/connector/cn_proc.c | 2 +- drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 8 ++++---- drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 8 ++++---- fs/coda/inode.c | 2 +- fs/coda/psdev.c | 2 +- include/linux/pid_namespace.h | 5 +++++ kernel/audit.c | 2 +- kernel/taskstats.c | 2 +- 8 files changed, 18 insertions(+), 13 deletions(-) -- 2.25.1

4 years, 1 month

[PATCH v3 0/6] coresight: syscfg: dynamic load

by Mike Leach

1) API updated to allow dynamic load and unload of configurations and features. Dependency management between loaded sets is added. 2) New configuration and feature sets can be added using a loadable module. An example in /samples/coresight is provided to demonstrate this. 3) configfs can be used to activate a configuration which will then be used when controlling tracing using sysfs. Applies and tested on coresight/next - which is 5.16-rc1 Changes since v2: (requested by Mathieu) a) Split first patch into two patches to introduce the concept of owner, then use it to implement dynamic load / unload. b) Renamed configfs configuration attributes used to enable and set preset on configurations when using CoreSight with sysfs c) Updated docs to reflect use of enable and preset attributes for sysfs CoreSight control. Changes since v1: a) Original set split to divide related changes into smaller sets. Removed RFC flag. b) Revised config activation for sysfs to simplify common function. (patch 4). c) Minor changes requested by Mathieu added. Mike Leach (6): coresight: configuration: Update API to introduce load owner concept coresight: configuration: Update API to permit dynamic load/unload coresight: syscfg: Update load API for config loadable modules coresight: syscfg: Example CoreSight configuration loadable module coresight: configfs: Allow configfs to activate configuration Documentation: coresight: Update coresight configuration docs .../trace/coresight/coresight-config.rst | 62 +++- MAINTAINERS | 1 + .../coresight/coresight-cfg-preload.c | 9 +- .../hwtracing/coresight/coresight-config.h | 9 +- .../coresight/coresight-etm4x-core.c | 11 +- .../coresight/coresight-syscfg-configfs.c | 87 +++++ .../coresight/coresight-syscfg-configfs.h | 4 + .../hwtracing/coresight/coresight-syscfg.c | 315 ++++++++++++++++-- .../hwtracing/coresight/coresight-syscfg.h | 39 ++- samples/Kconfig | 9 + samples/Makefile | 1 + samples/coresight/Makefile | 4 + samples/coresight/coresight-cfg-sample.c | 73 ++++ 13 files changed, 586 insertions(+), 38 deletions(-) create mode 100644 samples/coresight/Makefile create mode 100644 samples/coresight/coresight-cfg-sample.c -- 2.17.1

4 years, 1 month

Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device

by Mathieu Poirier

On Thu, Nov 25, 2021 at 04:39:46PM +0800, Yicong Yang wrote: > Hi Mathieu, > > Thanks for the comments! Replies inline. > > On 2021/11/25 2:51, Mathieu Poirier wrote: > > Hi Yicong, > > > > On Tue, Nov 16, 2021 at 05:06:21PM +0800, Yicong Yang wrote: > >> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex > >> integrated Endpoint(RCiEP) device, providing the capability > >> to dynamically monitor and tune the PCIe traffic(tune), > >> and trace the TLP headers(trace). > >> > > > > Is there a reason to put "tune" and "trace" are whithin parentheses? > > > > yeah. I want to use these single word to denote the related function, and > place them in the parantheses near the function's description to make sure > readers can match them. > Probably a better idea to simply write "TLP headers" and "PCIe traffic" when referring to them. Otherwise it is very confusing because the reader doesn't know if you mean the acronym or the real definition of "tune" and "trace". > >> Add the driver for the device to enable the trace function. The driver > >> will create PMU device for each PTT device, and users can start trace > >> through perf command. > >> > >> Signed-off-by: Yicong Yang <yangyicong(a)hisilicon.com> > >> --- > >> drivers/Makefile | 1 + > >> drivers/hwtracing/Kconfig | 2 + > >> drivers/hwtracing/hisilicon/Kconfig | 8 + > >> drivers/hwtracing/hisilicon/Makefile | 2 + > >> drivers/hwtracing/hisilicon/hisi_ptt.c | 1146 ++++++++++++++++++++++++ > > > > Not sure about the "hisilicon" directory. Right now we have "coresight", > > "intel_th" and "stm" - they concentrate on the technology rather than the > > vendor. I think "ptt" would be just fine: > > > > drivers/hwtracing/ptt/Kconfig > > drivers/hwtracing/ptt/Makefile > > drivers/hwtracing/ptt/hisi_ptt.c > > > > That way if other vendors want to introduce support for the same kind of > > technology we can simply put them under "drivers/hwtracing/ptt/" and things are > > still accurate. > > > > sure. sounds sensible. will rename the directory to "ptt" if no objection. > > > > >> 5 files changed, 1159 insertions(+) > >> create mode 100644 drivers/hwtracing/hisilicon/Kconfig > >> create mode 100644 drivers/hwtracing/hisilicon/Makefile > >> create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c > >> > >> diff --git a/drivers/Makefile b/drivers/Makefile > >> index be5d40ae1488..bb0dc9b55ea2 100644 > >> --- a/drivers/Makefile > >> +++ b/drivers/Makefile > >> @@ -176,6 +176,7 @@ obj-$(CONFIG_USB4) += thunderbolt/ > >> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/ > >> obj-y += hwtracing/intel_th/ > >> obj-$(CONFIG_STM) += hwtracing/stm/ > >> +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ > >> obj-$(CONFIG_ANDROID) += android/ > >> obj-$(CONFIG_NVMEM) += nvmem/ > >> obj-$(CONFIG_FPGA) += fpga/ > >> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig > >> index 13085835a636..e3796b17541a 100644 > >> --- a/drivers/hwtracing/Kconfig > >> +++ b/drivers/hwtracing/Kconfig > >> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" > >> > >> source "drivers/hwtracing/intel_th/Kconfig" > >> > >> +source "drivers/hwtracing/hisilicon/Kconfig" > >> + > >> endmenu > >> diff --git a/drivers/hwtracing/hisilicon/Kconfig b/drivers/hwtracing/hisilicon/Kconfig > >> new file mode 100644 > >> index 000000000000..9c3ab80d99fe > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/Kconfig > >> @@ -0,0 +1,8 @@ > >> +# SPDX-License-Identifier: GPL-2.0-only > >> +config HISI_PTT > >> + tristate "HiSilicon PCIe Tune and Trace Device" > >> + depends on PCI && HAS_DMA && HAS_IOMEM > > > > What Arm architecture are you aiming this for? Can it be found on both armv7 > > and armv8? > > > > currently the device will appear on Kunpeng 930, which is armv8. Ok, then please make it dependent on armv8. > > >> + help > >> + HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP > >> + device, provides support for PCIe traffic tuning and > >> + tracing TLP headers to the memory. > > > > Please indicate what the name of the driver will be when compiled as a module. > > > > sure. will add the module's name here. > > >> diff --git a/drivers/hwtracing/hisilicon/Makefile b/drivers/hwtracing/hisilicon/Makefile > >> new file mode 100644 > >> index 000000000000..908c09a98161 > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/Makefile > >> @@ -0,0 +1,2 @@ > >> +# SPDX-License-Identifier: GPL-2.0 > >> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o > >> diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c b/drivers/hwtracing/hisilicon/hisi_ptt.c > >> new file mode 100644 > >> index 000000000000..e11e9b6cc2a8 > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c > >> @@ -0,0 +1,1146 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Driver for HiSilicon PCIe tune and trace device > >> + * > >> + * Copyright (c) 2021 HiSilicon Limited. > >> + */ > >> + > >> +#include <linux/bitfield.h> > >> +#include <linux/bitops.h> > >> +#include <linux/delay.h> > >> +#include <linux/dma-iommu.h> > >> +#include <linux/dma-mapping.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/io.h> > >> +#include <linux/iommu.h> > >> +#include <linux/iopoll.h> > >> +#include <linux/kfifo.h> > >> +#include <linux/module.h> > >> +#include <linux/notifier.h> > >> +#include <linux/pci.h> > >> +#include <linux/perf_event.h> > >> +#include <linux/sizes.h> > >> +#include <linux/sysfs.h> > >> +#include <linux/vmalloc.h> > >> +#include <linux/workqueue.h> > >> + > >> +#define HISI_PTT_TRACE_ADDR_SIZE 0x0800 > >> +#define HISI_PTT_TRACE_ADDR_BASE_LO_0 0x0810 > >> +#define HISI_PTT_TRACE_ADDR_BASE_HI_0 0x0814 > >> +#define HISI_PTT_TRACE_ADDR_STRIDE 0x8 > >> +#define HISI_PTT_TRACE_CTRL 0x0850 > >> +#define HISI_PTT_TRACE_CTRL_EN BIT(0) > >> +#define HISI_PTT_TRACE_CTRL_RST BIT(1) > >> +#define HISI_PTT_TRACE_CTRL_RXTX_SEL GENMASK(3, 2) > >> +#define HISI_PTT_TRACE_CTRL_TYPE_SEL GENMASK(7, 4) > >> +#define HISI_PTT_TRACE_CTRL_DATA_FORMAT BIT(14) > >> +#define HISI_PTT_TRACE_CTRL_FILTER_MODE BIT(15) > >> +#define HISI_PTT_TRACE_CTRL_TARGET_SEL GENMASK(31, 16) > >> +#define HISI_PTT_TRACE_INT_STAT 0x0890 > >> +#define HISI_PTT_TRACE_INT_STAT_MASK GENMASK(3, 0) > >> +#define HISI_PTT_TRACE_INT_MASK 0x0894 > >> +#define HISI_PTT_TRACE_WR_STS 0x08a0 > >> +#define HISI_PTT_TRACE_WR_STS_WRITE GENMASK(27, 0) > >> +#define HISI_PTT_TRACE_WR_STS_BUFFER GENMASK(29, 28) > >> +#define HISI_PTT_TRACE_STS 0x08b0 > >> +#define HISI_PTT_TRACE_IDLE BIT(0) > >> +#define HISI_PTT_DEVICE_RANGE 0x0fe0 > >> +#define HISI_PTT_LOCATION 0x0fe8 > >> +#define HISI_PTT_CORE_ID GENMASK(15, 0) > >> +#define HISI_PTT_SICL_ID GENMASK(31, 16) > > > > Extra spaces between #define and the literals. Please align the values with > > that of HISI_PTT_TRACE_DMA_IRQ below. > > > > Spaces are intended to distinguish the definition of the register offset and > related register fields. also used in include/uapi/linux/pci_regs.h. > Interesting and definitely not the norm... > HISI_PTT_TRACE_DMA_IRQ is well aligned with tab but seems with problem display > in the mail text, maybe because the tab is not 8 characters when display. > In my VI terminal the '0' of HISI_PTT_TRACE_DMA_IRQ is align with the '3' in GENMASK(31, 16) and I suspect it is the same for other people. > >> + > >> +#define HISI_PTT_TRACE_DMA_IRQ 0 > >> +#define HISI_PTT_TRACE_BUFLETS_CNT 4 > >> +#define HISI_PTT_TRACE_BUFLET_SIZE SZ_4M > >> +#define HISI_PTT_TRACE_BUFFER_SIZE (HISI_PTT_TRACE_BUFLET_SIZE * \ > >> + HISI_PTT_TRACE_BUFLETS_CNT) > >> +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE 16 > >> + > >> +/* Delay time for filter updating work */ > >> +#define HISI_PTT_WORK_DELAY_MS 100UL > >> +/* Wait time for DMA hardware to reset */ > >> +#define HISI_PTT_RESET_WAIT_MS 1000UL > >> +/* Poll timeout and interval for waiting hardware work to finish */ > >> +#define HISI_PTT_WAIT_TIMEOUT_US 1000000UL > >> +#define HISI_PTT_WAIT_POLL_INTERVAL_US 100UL > >> + > >> +#define HISI_PCIE_CORE_PORT_ID(devfn) (PCI_FUNC(devfn) << 1) > >> + > >> +enum hisi_ptt_trace_status { > >> + HISI_PTT_TRACE_STATUS_OFF = 0, > >> + HISI_PTT_TRACE_STATUS_ON, > >> +}; > >> + > >> +struct hisi_ptt_dma_buflet { > > > > I'm not sure what a "buflet" is... Probably best to just call this > > hisi_ptt_dma_buffer" if it pertains to a buffer. On that note it is hard to > > know due to the lack of proper structure documentation. Please have a look at > > how "coresight_device" is documented. The same applies to the rest of the > > structures declared below. > > > > It's mentioned in section 5 of hisi_ptt.rst as "Driver will allocate each DMA buffer > (we call it buflet) of 4MiB ...". I just noticed the documentation in patch 5 - I will read it before continuing with this set. > > The total DMA buffer of PTT device is divided into 4 parts, and each part is called > a 'buflet', which means a small buffer (there exists other words with similiar > format: droplet, wavelet, ...). > > The device is designed like this to make sure we won't lose any data when we have to change > the buffer address, the device can continue to write to the next buflet and don't > neet to pause. > > >> + struct list_head list; > >> + unsigned int size; > >> + dma_addr_t dma; > >> + > >> + /* > >> + * The address of the buflet holding the trace data. > >> + * See Documentation/trace/hisi-ptt.rst for the details > > > > As of this writing, hisi-ptt.rst doesn't exist. > > > > The comment intends to direct the user to the documentation if the user > want to know details about the data format. The data format is decribed > in the section 4 of the doc. > > >> + * of the data format. > >> + */ > >> + void *addr; > >> + int index; > >> +}; > >> + > >> +struct hisi_ptt_trace_ctrl { > >> + enum hisi_ptt_trace_status status; > >> + struct perf_output_handle handle; > >> + struct list_head trace_buf; > >> + /* > >> + * The index of the buffer which trace data > >> + * currently is writing to. > >> + */ > >> + u32 buf_index; > >> + > >> + int default_cpu; > >> + u32 buflet_size; > >> + bool is_port; > >> + u32 format:1; /* Format of the traced TLP headers */ > >> + u32 type:4; /* Type of the TLP headers to trace */ > >> + u32 direction:2; /* Direction of the TLP headers to trace */ > >> + u32 filter:16; /* Root port or Requester to filter the TLP headers */ > >> + > >> + phys_addr_t rmr_addr; > >> + size_t rmr_length; > >> + bool has_rmr; > >> +}; > >> + > >> +struct hisi_ptt_filter_desc { > >> + struct list_head list; > >> + struct pci_dev *pdev; > >> + u16 val; > >> +}; > >> + > >> +struct hisi_ptt_pmu_buf { > >> + size_t length; > >> + int nr_pages; > >> + void *base; > >> + long pos; > >> +}; > >> + > >> +/* Structure containing the information for filter updating */ > >> +struct hisi_ptt_filter_update_info { > >> + struct pci_dev *pdev; > >> + u32 port_devid; > >> + bool is_port; > >> + bool is_add; > >> + u16 val; > >> +}; > >> + > >> +struct hisi_ptt { > >> + struct hisi_ptt_trace_ctrl trace_ctrl; > >> + struct notifier_block hisi_ptt_nb; > >> + struct pmu hisi_ptt_pmu; > >> + void __iomem *iobase; > >> + struct pci_dev *pdev; > >> + /* > >> + * Use the mutex to protect the filter list and > >> + * serialize the perf process. > >> + */ > >> + struct mutex mutex; > >> + const char *name; > >> + u16 core_id; > >> + u16 sicl_id; > >> + /* PCI device range managed by the PTT device */ > >> + u32 upper; > >> + u32 lower; > >> + u8 busnr; > >> + > >> + /* > >> + * The trace TLP headers can either be filtered by certain > >> + * root port, or by the requester ID. Organize the filters > >> + * by @port_filters and @req_filters here. The mask of all > >> + * the valid ports is also cached for doing sanity check > >> + * of user input. > >> + */ > >> + struct list_head port_filters; > >> + struct list_head req_filters; > >> + u16 port_mask; > >> + > >> + /* > >> + * We use a delayed work here to avoid indefinitely waiting for > >> + * the hisi_ptt->mutex which protecting the filter list. The > >> + * work will be delayed only if the mutex can not be held, > >> + * otherwise no delay will be applied. > >> + */ > >> + struct delayed_work work; > >> + spinlock_t filter_update_lock; > >> + DECLARE_KFIFO(filter_update_kfifo, struct hisi_ptt_filter_update_info, > >> + HISI_PTT_FILTER_UPDATE_FIFO_SIZE); > >> +}; > > > > It might be a good idea to spinoff a hisi_ptt.h for the above declarations. > > > > I'm not sure whether it is common or permitted to provide a header which is only used by > this driver with a single file. I've seen it done many times. > > >> + > >> +static inline struct hisi_ptt *to_hisi_ptt(struct pmu *pmu) > >> +{ > >> + return container_of(pmu, struct hisi_ptt, hisi_ptt_pmu); > >> +} > > > > I'm not sure there is any value to this inline function. I suggest to simply > > call container_of() whenever it is needed. > > > > ok. > > >> + > >> +static u16 hisi_ptt_get_filter_val(struct pci_dev *pdev) > >> +{ > >> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) > >> + return BIT(HISI_PCIE_CORE_PORT_ID(PCI_SLOT(pdev->devfn))); > >> + > >> + return PCI_DEVID(pdev->bus->number, pdev->devfn); > >> +} > >> + > >> +static int hisi_ptt_wait_trace_hw_idle(struct hisi_ptt *hisi_ptt) > >> +{ > >> + u32 val; > >> + > >> + return readl_poll_timeout(hisi_ptt->iobase + HISI_PTT_TRACE_STS, val, > >> + val & HISI_PTT_TRACE_IDLE, > >> + HISI_PTT_WAIT_POLL_INTERVAL_US, > >> + HISI_PTT_WAIT_TIMEOUT_US); > >> +} > >> + > >> +static void hisi_ptt_free_trace_buf(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl; > >> + struct device *dev = &hisi_ptt->pdev->dev; > >> + struct hisi_ptt_dma_buflet *buflet, *tbuflet; > >> + > >> + list_for_each_entry_safe(buflet, tbuflet, &ctrl->trace_buf, list) { > >> + list_del(&buflet->list); > >> + > >> + if (ctrl->has_rmr) > >> + memunmap(buflet->addr); > >> + else > >> + dma_free_coherent(dev, buflet->size, buflet->addr, > >> + buflet->dma); > >> + > >> + kfree(buflet); > >> + } > >> +} > >> + > >> +static int hisi_ptt_alloc_trace_buf(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl; > >> + struct device *dev = &hisi_ptt->pdev->dev; > >> + struct hisi_ptt_dma_buflet *buflet; > >> + int i, ret; > >> + > >> + hisi_ptt->trace_ctrl.buf_index = 0; > >> + > >> + /* Make sure the trace buffer is empty before allocating */ > >> + if (!list_empty(&ctrl->trace_buf)) { > >> + list_for_each_entry(buflet, &ctrl->trace_buf, list) > >> + memset(buflet->addr, 0, buflet->size); > >> + return 0; > >> + } > >> + > >> + for (i = 0; i < HISI_PTT_TRACE_BUFLETS_CNT; ++i) { > >> + buflet = kzalloc(sizeof(*buflet), GFP_KERNEL); > >> + if (!buflet) { > >> + ret = -ENOMEM; > >> + goto err; > >> + } > >> + > >> + if (ctrl->has_rmr) { > >> + phys_addr_t base = ctrl->rmr_addr + i * ctrl->buflet_size; > >> + > >> + buflet->dma = base; > >> + buflet->addr = memremap(base, ctrl->buflet_size, MEMREMAP_WB); > >> + } else { > >> + buflet->addr = dma_alloc_coherent(dev, ctrl->buflet_size, > >> + &buflet->dma, GFP_KERNEL); > >> + } > >> + > >> + if (!buflet->addr) { > >> + kfree(buflet); > >> + ret = -ENOMEM; > >> + goto err; > >> + } > >> + > >> + memset(buflet->addr, 0, buflet->size); > >> + > >> + buflet->index = i; > >> + buflet->size = ctrl->buflet_size; > >> + list_add_tail(&buflet->list, &ctrl->trace_buf); > >> + } > >> + > >> + return 0; > >> +err: > >> + hisi_ptt_free_trace_buf(hisi_ptt); > >> + return ret; > >> +} > >> + > >> +static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt) > >> +{ > >> + writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + hisi_ptt->trace_ctrl.status = HISI_PTT_TRACE_STATUS_OFF; > >> +} > >> + > >> +static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl; > >> + struct hisi_ptt_dma_buflet *cur; > >> + u32 val; > >> + > >> + /* Check device idle before start trace */ > >> + if (hisi_ptt_wait_trace_hw_idle(hisi_ptt)) { > >> + pci_err(hisi_ptt->pdev, "Failed to start trace, the device is still busy.\n"); > >> + return -EBUSY; > >> + } > >> + > >> + /* Reset the DMA before start tracing */ > >> + val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + val |= HISI_PTT_TRACE_CTRL_RST; > >> + writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + > >> + /* > >> + * We'll be in the perf context where preemption is disabled, > >> + * so use busy loop here. > >> + */ > >> + mdelay(HISI_PTT_RESET_WAIT_MS); > >> + > >> + val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + val &= ~HISI_PTT_TRACE_CTRL_RST; > >> + writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + > >> + /* clear the interrupt status */ > >> + writel(HISI_PTT_TRACE_INT_STAT_MASK, hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT); > >> + writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_INT_MASK); > >> + > >> + list_for_each_entry(cur, &ctrl->trace_buf, list) { > >> + writel(lower_32_bits(cur->dma), > >> + hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_BASE_LO_0 + > >> + cur->index * HISI_PTT_TRACE_ADDR_STRIDE); > >> + writel(upper_32_bits(cur->dma), > >> + hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_BASE_HI_0 + > >> + cur->index * HISI_PTT_TRACE_ADDR_STRIDE); > >> + } > >> + writel(ctrl->buflet_size, hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_SIZE); > >> + > >> + /* set the trace control register */ > >> + val = FIELD_PREP(HISI_PTT_TRACE_CTRL_TYPE_SEL, ctrl->type); > >> + val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_RXTX_SEL, ctrl->direction); > >> + val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_DATA_FORMAT, ctrl->format); > >> + val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_TARGET_SEL, hisi_ptt->trace_ctrl.filter); > >> + if (!hisi_ptt->trace_ctrl.is_port) > >> + val |= HISI_PTT_TRACE_CTRL_FILTER_MODE; > >> + > >> + val |= HISI_PTT_TRACE_CTRL_EN; > >> + writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL); > >> + > >> + ctrl->status = HISI_PTT_TRACE_STATUS_ON; > >> + > >> + return 0; > >> +} > >> + > >> +static int hisi_ptt_update_aux(struct hisi_ptt *hisi_ptt, int index, bool stop) > >> +{ > >> + struct perf_output_handle *handle = &hisi_ptt->trace_ctrl.handle; > >> + struct perf_event *event = handle->event; > >> + struct hisi_ptt_dma_buflet *cur; > >> + struct hisi_ptt_pmu_buf *buf; > >> + > >> + buf = perf_get_aux(handle); > >> + if (!buf || !handle->size) > >> + return -EINVAL; > >> + > >> + list_for_each_entry(cur, &hisi_ptt->trace_ctrl.trace_buf, list) > >> + if (cur->index == index) > >> + break; > >> + > >> + memcpy(buf->base + buf->pos, cur->addr, cur->size); > >> + memset(cur->addr, 0, cur->size); > >> + buf->pos += cur->size; > >> + > >> + if (stop) { > >> + perf_aux_output_end(handle, buf->pos); > >> + } else if (buf->length - buf->pos < cur->size) { > >> + perf_aux_output_skip(handle, buf->length - buf->pos); > >> + perf_aux_output_end(handle, buf->pos); > >> + > >> + buf = perf_aux_output_begin(handle, event); > >> + if (!buf) > >> + return -EINVAL; > >> + > >> + buf->pos = handle->head % buf->length; > >> + if (buf->length - buf->pos < cur->size) { > >> + perf_aux_output_end(handle, 0); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static irqreturn_t hisi_ptt_isr(int irq, void *context) > >> +{ > >> + struct hisi_ptt *hisi_ptt = context; > >> + u32 status, buf_idx; > >> + > >> + status = readl(hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT); > >> + buf_idx = ffs(status) - 1; > >> + > >> + /* Clear the interrupt status of buflet @buf_idx */ > >> + writel(status, hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT); > >> + > >> + /* > >> + * Update the AUX buffer and cache the current buflet index, > >> + * as we need to know this and save the data when the trace > >> + * is ended out of the interrupt handler. End the trace > >> + * if the updating fails. > >> + */ > >> + if (hisi_ptt_update_aux(hisi_ptt, buf_idx, false)) > >> + hisi_ptt_trace_end(hisi_ptt); > >> + else > >> + hisi_ptt->trace_ctrl.buf_index = (buf_idx + 1) % HISI_PTT_TRACE_BUFLETS_CNT; > >> + > >> + return IRQ_HANDLED; > >> +} > >> + > >> +static irqreturn_t hisi_ptt_irq(int irq, void *context) > >> +{ > >> + struct hisi_ptt *hisi_ptt = context; > >> + u32 status; > >> + > >> + status = readl(hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT); > >> + if (!(status & HISI_PTT_TRACE_INT_STAT_MASK)) > >> + return IRQ_NONE; > >> + > >> + return IRQ_WAKE_THREAD; > >> +} > >> + > >> +static void hisi_ptt_irq_free_vectors(void *pdev) > >> +{ > >> + pci_free_irq_vectors(pdev); > >> +} > >> + > >> +static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct pci_dev *pdev = hisi_ptt->pdev; > >> + int ret; > >> + > >> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI); > >> + if (ret < 0) { > >> + pci_err(pdev, "failed to allocate irq vector, ret = %d.\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = devm_add_action_or_reset(&pdev->dev, hisi_ptt_irq_free_vectors, pdev); > >> + if (ret < 0) > >> + return ret; > >> + > >> + ret = devm_request_threaded_irq(&pdev->dev, > >> + pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ), > >> + hisi_ptt_irq, hisi_ptt_isr, 0, > >> + "hisi-ptt", hisi_ptt); > >> + if (ret) { > >> + pci_err(pdev, "failed to request irq %d, ret = %d.\n", > >> + pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ), ret); > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static void hisi_ptt_update_filters(struct work_struct *work) > >> +{ > >> + struct delayed_work *delayed_work = to_delayed_work(work); > >> + struct hisi_ptt_filter_update_info info; > >> + struct hisi_ptt_filter_desc *filter; > >> + struct list_head *target_list; > >> + struct hisi_ptt *hisi_ptt; > >> + > >> + hisi_ptt = container_of(delayed_work, struct hisi_ptt, work); > >> + > >> + if (!mutex_trylock(&hisi_ptt->mutex)) { > >> + schedule_delayed_work(&hisi_ptt->work, HISI_PTT_WORK_DELAY_MS); > >> + return; > >> + } > >> + > >> + while (kfifo_get(&hisi_ptt->filter_update_kfifo, &info)) { > >> + target_list = info.is_port ? &hisi_ptt->port_filters : > >> + &hisi_ptt->req_filters; > >> + > >> + if (info.is_add) { > >> + filter = kzalloc(sizeof(*filter), GFP_KERNEL); > >> + if (!filter) > >> + continue; > >> + > >> + filter->pdev = info.pdev; > >> + filter->val = info.val; > >> + > >> + list_add_tail(&filter->list, target_list); > >> + } else { > >> + list_for_each_entry(filter, target_list, list) > >> + if (filter->val == info.val) { > >> + list_del(&filter->list); > >> + kfree(filter); > >> + break; > >> + } > >> + } > >> + > >> + /* Update the available port mask */ > >> + if (!info.is_port) > >> + continue; > >> + > >> + if (info.is_add) > >> + hisi_ptt->port_mask |= info.val; > >> + else > >> + hisi_ptt->port_mask &= ~info.val; > >> + } > >> + > >> + mutex_unlock(&hisi_ptt->mutex); > >> +} > >> + > >> +static void hisi_ptt_update_fifo_in(struct hisi_ptt *hisi_ptt, > >> + struct hisi_ptt_filter_update_info *info) > >> +{ > >> + struct pci_dev *root_port = pcie_find_root_port(info->pdev); > >> + > >> + if (!root_port) > >> + return; > >> + > >> + info->port_devid = PCI_DEVID(root_port->bus->number, root_port->devfn); > >> + if (info->port_devid < hisi_ptt->lower || > >> + info->port_devid > hisi_ptt->upper) > >> + return; > >> + > >> + info->is_port = pci_pcie_type(info->pdev) == PCI_EXP_TYPE_ROOT_PORT; > >> + info->val = hisi_ptt_get_filter_val(info->pdev); > >> + > >> + if (kfifo_in_spinlocked(&hisi_ptt->filter_update_kfifo, info, 1, > >> + &hisi_ptt->filter_update_lock)) > >> + schedule_delayed_work(&hisi_ptt->work, 0); > >> + else > >> + pci_warn(hisi_ptt->pdev, > >> + "filter update fifo overflow for target %s\n", > >> + pci_name(info->pdev)); > >> +} > >> + > >> +/* > >> + * A PCI bus notifier is used here for dynamically updating the filter > >> + * list. > >> + */ > >> +static int hisi_ptt_notifier_call(struct notifier_block *nb, unsigned long action, > >> + void *data) > >> +{ > >> + struct hisi_ptt *hisi_ptt = container_of(nb, struct hisi_ptt, hisi_ptt_nb); > >> + struct hisi_ptt_filter_update_info info; > >> + struct device *dev = data; > >> + struct pci_dev *pdev = to_pci_dev(dev); > >> + > >> + info.pdev = pdev; > >> + > >> + switch (action) { > >> + case BUS_NOTIFY_ADD_DEVICE: > >> + info.is_add = true; > >> + break; > >> + case BUS_NOTIFY_DEL_DEVICE: > >> + info.is_add = false; > >> + break; > >> + default: > >> + return 0; > >> + } > >> + > >> + hisi_ptt_update_fifo_in(hisi_ptt, &info); > >> + > >> + return 0; > >> +} > >> + > >> +static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data) > >> +{ > >> + struct hisi_ptt_filter_update_info info = { > >> + .pdev = pdev, > >> + .is_add = true, > >> + }; > >> + struct hisi_ptt *hisi_ptt = data; > >> + > >> + hisi_ptt_update_fifo_in(hisi_ptt, &info); > >> + > >> + return 0; > >> +} > >> + > >> +static void hisi_ptt_release_filters(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct hisi_ptt_filter_desc *filter, *tfilter; > >> + > >> + list_for_each_entry_safe(filter, tfilter, &hisi_ptt->req_filters, list) { > >> + list_del(&filter->list); > >> + kfree(filter); > >> + } > >> + > >> + list_for_each_entry_safe(filter, tfilter, &hisi_ptt->port_filters, list) { > >> + list_del(&filter->list); > >> + kfree(filter); > >> + } > >> +} > >> + > >> +static void hisi_ptt_init_ctrls(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct pci_dev *pdev = hisi_ptt->pdev; > >> + struct pci_bus *bus; > >> + u32 reg; > >> + > >> + INIT_DELAYED_WORK(&hisi_ptt->work, hisi_ptt_update_filters); > >> + spin_lock_init(&hisi_ptt->filter_update_lock); > >> + INIT_KFIFO(hisi_ptt->filter_update_kfifo); > >> + INIT_LIST_HEAD(&hisi_ptt->port_filters); > >> + INIT_LIST_HEAD(&hisi_ptt->req_filters); > >> + > >> + /* > >> + * The device range register provides the information about the > >> + * root ports which the RCiEP can control and trace. The RCiEP > >> + * and the root ports it support are on the same PCIe core, with > >> + * same domain number but maybe different bus number. The device > >> + * range register will tell us which root ports we can support, > >> + * Bit[31:16] indicates the upper BDF numbers of the root port, > >> + * while Bit[15:0] indicates the lower. > >> + */ > >> + reg = readl(hisi_ptt->iobase + HISI_PTT_DEVICE_RANGE); > >> + hisi_ptt->upper = reg >> 16; > >> + hisi_ptt->lower = reg & 0xffff; > >> + hisi_ptt->busnr = PCI_BUS_NUM(hisi_ptt->upper); > >> + > >> + reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION); > >> + hisi_ptt->core_id = FIELD_GET(HISI_PTT_CORE_ID, reg); > >> + hisi_ptt->sicl_id = FIELD_GET(HISI_PTT_SICL_ID, reg); > >> + > >> + bus = pci_find_bus(pci_domain_nr(pdev->bus), hisi_ptt->busnr); > >> + if (bus) > >> + pci_walk_bus(bus, hisi_ptt_init_filters, hisi_ptt); > > > > What happens if bus is NULL? Should this be reported to the caller and cause > > hisi_ptt_probe() to fail? Please add a comment that describes the scenario. > > > > It's ok if the list is NULL in the probe process, > as the device maybe hotplugged after the PTT driver probe, in which case > we can detect the event and update the list as we register a bus notifier. > Please add the above as a comment in the code. > >> + > >> + /* Initialize trace controls */ > >> + INIT_LIST_HEAD(&hisi_ptt->trace_ctrl.trace_buf); > >> + hisi_ptt->trace_ctrl.buflet_size = HISI_PTT_TRACE_BUFLET_SIZE; > >> + hisi_ptt->trace_ctrl.default_cpu = cpumask_first(cpumask_of_node(dev_to_node(&pdev->dev))); > >> +} > >> + > >> +#define HISI_PTT_PMU_FILTER_IS_PORT BIT(19) > >> +#define HISI_PTT_PMU_FILTER_VAL_MASK GENMASK(15, 0) > >> +#define HISI_PTT_PMU_DIRECTION_MASK GENMASK(23, 20) > >> +#define HISI_PTT_PMU_TYPE_MASK GENMASK(31, 24) > >> +#define HISI_PTT_PMU_FORMAT_MASK GENMASK(35, 32) > >> + > >> +static ssize_t available_filters_show(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); > >> + struct hisi_ptt_filter_desc *filter; > >> + int pos = 0; > >> + > >> + if (list_empty(&hisi_ptt->port_filters)) > >> + return sysfs_emit(buf, "#### No available filter ####\n"); > >> + > >> + mutex_lock(&hisi_ptt->mutex); > >> + pos += sysfs_emit_at(buf, pos, "#### Root Ports ####\n"); > >> + list_for_each_entry(filter, &hisi_ptt->port_filters, list) > >> + pos += sysfs_emit_at(buf, pos, "%s 0x%05lx\n", > >> + pci_name(filter->pdev), > >> + hisi_ptt_get_filter_val(filter->pdev) | > >> + HISI_PTT_PMU_FILTER_IS_PORT); > >> + > >> + pos += sysfs_emit_at(buf, pos, "#### Requesters ####\n"); > >> + list_for_each_entry(filter, &hisi_ptt->req_filters, list) > >> + pos += sysfs_emit_at(buf, pos, "%s 0x%05x\n", > >> + pci_name(filter->pdev), > >> + hisi_ptt_get_filter_val(filter->pdev)); > >> + > >> + mutex_unlock(&hisi_ptt->mutex); > >> + return pos; > >> +} > >> +static DEVICE_ATTR_ADMIN_RO(available_filters); > >> + > >> +PMU_FORMAT_ATTR(filter, "config:0-19"); > >> +PMU_FORMAT_ATTR(direction, "config:20-23"); > >> +PMU_FORMAT_ATTR(type, "config:24-31"); > >> +PMU_FORMAT_ATTR(format, "config:32-35"); > >> + > >> +static struct attribute *hisi_ptt_pmu_format_attrs[] = { > >> + &format_attr_filter.attr, > >> + &format_attr_direction.attr, > >> + &format_attr_type.attr, > >> + &format_attr_format.attr, > >> + NULL > >> +}; > >> + > >> +static struct attribute_group hisi_ptt_pmu_format_group = { > >> + .name = "format", > >> + .attrs = hisi_ptt_pmu_format_attrs, > >> +}; > >> + > >> +static struct attribute *hisi_ptt_pmu_filter_attrs[] = { > >> + &dev_attr_available_filters.attr, > >> + NULL > >> +}; > >> + > >> +static struct attribute_group hisi_ptt_pmu_filter_group = { > >> + .attrs = hisi_ptt_pmu_filter_attrs, > >> +}; > >> + > >> +static const struct attribute_group *hisi_ptt_pmu_groups[] = { > >> + &hisi_ptt_pmu_format_group, > >> + &hisi_ptt_pmu_filter_group, > >> + NULL > >> +}; > >> + > >> +/* > >> + * The supported value of the direction parameter. See hisi_ptt.rst > >> + * documentation for more details. > >> + */ > >> +static u32 hisi_ptt_trace_available_direction[] = { > >> + 0, > >> + 1, > >> + 2, > >> + 3, > >> +}; > >> + > >> +/* Different types can be set simultaneously */ > >> +static u32 hisi_ptt_trace_available_type[] = { > >> + 1, /* posted_request */ > >> + 2, /* non-posted_request */ > >> + 4, /* completion */ > >> +}; > >> + > >> +static u32 hisi_ptt_trace_availble_format[] = { > >> + 0, /* 4DW */ > >> + 1, /* 8DW */ > >> +}; > >> + > >> +/* > >> + * Check whether the config is valid or not. Some configs are multi-selectable > >> + * and can be set simultaneously, while some are single selectable (onehot). > >> + * Use this function to check the non-onehot configs while > >> + * hisi_ptt_trace_valid_config_onehot() for the onehot ones. > >> + */ > >> +static int hisi_ptt_trace_valid_config(u32 val, u32 *available_list, u32 list_size) > >> +{ > >> + int i; > >> + > >> + /* > >> + * The non-onehot configs cannot be 0. Walk the available > >> + * list and clear the valid bits of the config. If there > >> + * is any resident bit after the walk then the config is > >> + * invalid. > >> + */ > >> + for (i = 0; i < list_size; i++) > >> + val &= ~available_list[i]; > >> + > >> + return val ? -EINVAL : 0; > >> +} > >> + > >> +static int hisi_ptt_trace_valid_config_onehot(u32 val, u32 *available_list, u32 list_size) > >> +{ > >> + int i, ret = -EINVAL; > >> + > >> + for (i = 0; i < list_size; i++) > >> + if (val == available_list[i]) { > >> + ret = 0; > >> + break; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static int hisi_ptt_trace_init_filter(struct hisi_ptt *hisi_ptt, u64 config) > >> +{ > >> + unsigned long val, port_mask = hisi_ptt->port_mask; > >> + struct hisi_ptt_filter_desc *filter; > >> + int ret = -EINVAL; > >> + > >> + hisi_ptt->trace_ctrl.is_port = FIELD_GET(HISI_PTT_PMU_FILTER_IS_PORT, config); > >> + val = FIELD_GET(HISI_PTT_PMU_FILTER_VAL_MASK, config); > >> + > >> + /* > >> + * Port filters are defined as bit mask. For port filters, check > >> + * the bits in the @val are within the range of hisi_ptt->port_mask > >> + * and whether it's empty or not, otherwise user has specified > >> + * some unsupported root ports. > >> + * > >> + * For Requester ID filters, walk the available filter list to see > >> + * whether we have one matched. > >> + */ > >> + if (hisi_ptt->trace_ctrl.is_port && > >> + bitmap_subset(&val, &port_mask, BITS_PER_LONG)) { > >> + ret = 0; > >> + } else { > >> + list_for_each_entry(filter, &hisi_ptt->req_filters, list) > >> + if (filter->val == val) { > >> + ret = 0; > >> + break; > >> + } > >> + } > >> + > >> + if (ret) > >> + return ret; > >> + > >> + hisi_ptt->trace_ctrl.filter = val; > >> + return 0; > >> +} > >> + > >> +static int hisi_ptt_pmu_event_init(struct perf_event *event) > >> +{ > >> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu); > >> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl; > >> + int ret; > >> + u32 val; > >> + > >> + if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type) > >> + return -ENOENT; > >> + > >> + mutex_lock(&hisi_ptt->mutex); > >> + > >> + ret = hisi_ptt_trace_init_filter(hisi_ptt, event->attr.config); > >> + if (ret < 0) > >> + goto out; > >> + > >> + val = FIELD_GET(HISI_PTT_PMU_DIRECTION_MASK, event->attr.config); > >> + ret = hisi_ptt_trace_valid_config_onehot(val, hisi_ptt_trace_available_direction, > >> + ARRAY_SIZE(hisi_ptt_trace_available_direction)); > >> + if (ret < 0) > >> + goto out; > >> + ctrl->direction = val; > >> + > >> + val = FIELD_GET(HISI_PTT_PMU_TYPE_MASK, event->attr.config); > >> + > >> + ret = hisi_ptt_trace_valid_config(val, hisi_ptt_trace_available_type, > >> + ARRAY_SIZE(hisi_ptt_trace_available_type)); > >> + if (ret < 0) > >> + goto out; > >> + ctrl->type = val; > >> + > >> + val = FIELD_GET(HISI_PTT_PMU_FORMAT_MASK, event->attr.config); > >> + ret = hisi_ptt_trace_valid_config_onehot(val, hisi_ptt_trace_availble_format, > >> + ARRAY_SIZE(hisi_ptt_trace_availble_format)); > >> + if (ret < 0) > >> + goto out; > >> + ctrl->format = val; > >> + > >> +out: > >> + mutex_unlock(&hisi_ptt->mutex); > >> + return ret; > >> +} > >> + > >> +static void *hisi_ptt_pmu_setup_aux(struct perf_event *event, void **pages, > >> + int nr_pages, bool overwrite) > >> +{ > >> + struct hisi_ptt_pmu_buf *buf; > >> + struct page **pagelist; > >> + int i; > >> + > >> + if (overwrite) { > >> + dev_warn(event->pmu->dev, "Overwrite mode is not supported\n"); > >> + return NULL; > >> + } > >> + > >> + /* If the pages size less than buflets, we cannot start trace */ > >> + if (nr_pages < HISI_PTT_TRACE_BUFFER_SIZE / PAGE_SIZE) > >> + return NULL; > >> + > >> + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > >> + if (!buf) > >> + return NULL; > >> + > >> + pagelist = kcalloc(nr_pages, sizeof(*pagelist), GFP_KERNEL); > >> + if (!pagelist) { > >> + kfree(buf); > >> + return NULL; > >> + } > >> + > >> + for (i = 0; i < nr_pages; i++) > >> + pagelist[i] = virt_to_page(pages[i]); > >> + > >> + buf->base = vmap(pagelist, nr_pages, VM_MAP, PAGE_KERNEL); > >> + if (!buf->base) { > >> + kfree(pagelist); > >> + kfree(buf); > >> + return NULL; > >> + } > >> + > >> + buf->nr_pages = nr_pages; > >> + buf->length = nr_pages * PAGE_SIZE; > >> + buf->pos = 0; > >> + > >> + kfree(pagelist); > >> + return buf; > >> +} > >> + > >> +static void hisi_ptt_pmu_free_aux(void *aux) > >> +{ > >> + struct hisi_ptt_pmu_buf *buf = aux; > >> + > >> + vunmap(buf->base); > >> + kfree(buf); > >> +} > >> + > >> +static void hisi_ptt_pmu_start(struct perf_event *event, int flags) > >> +{ > >> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu); > >> + struct perf_output_handle *handle = &hisi_ptt->trace_ctrl.handle; > >> + struct hw_perf_event *hwc = &event->hw; > >> + struct hisi_ptt_pmu_buf *buf; > >> + int cpu = event->cpu; > >> + int ret; > >> + > >> + hwc->state = 0; > >> + mutex_lock(&hisi_ptt->mutex); > >> + if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON) { > >> + pci_dbg(hisi_ptt->pdev, "trace has already started\n"); > >> + goto stop; > >> + } > >> + > >> + if (cpu == -1) > >> + cpu = hisi_ptt->trace_ctrl.default_cpu; > >> + > >> + /* > >> + * Handle the interrupt on the same cpu which starts the trace to avoid > >> + * context mismatch. Otherwise we'll trigger the WARN from the perf > >> + * core in event_function_local(). > >> + */ > >> + WARN_ON(irq_set_affinity(pci_irq_vector(hisi_ptt->pdev, HISI_PTT_TRACE_DMA_IRQ), > >> + cpumask_of(cpu))); > >> + > >> + ret = hisi_ptt_alloc_trace_buf(hisi_ptt); > >> + if (ret) { > >> + pci_dbg(hisi_ptt->pdev, "alloc trace buf failed, ret = %d\n", ret); > >> + goto stop; > >> + } > >> + > >> + buf = perf_aux_output_begin(handle, event); > >> + if (!buf) { > >> + pci_dbg(hisi_ptt->pdev, "aux output begin failed\n"); > >> + goto stop; > >> + } > >> + > >> + buf->pos = handle->head % buf->length; > >> + > >> + ret = hisi_ptt_trace_start(hisi_ptt); > >> + if (ret) { > >> + pci_dbg(hisi_ptt->pdev, "trace start failed, ret = %d\n", ret); > >> + perf_aux_output_end(handle, 0); > >> + goto stop; > >> + } > >> + > >> + mutex_unlock(&hisi_ptt->mutex); > >> + return; > >> +stop: > >> + event->hw.state |= PERF_HES_STOPPED; > >> + mutex_unlock(&hisi_ptt->mutex); > >> +} > >> + > >> +static void hisi_ptt_pmu_stop(struct perf_event *event, int flags) > >> +{ > >> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu); > >> + struct hw_perf_event *hwc = &event->hw; > >> + > >> + if (hwc->state & PERF_HES_STOPPED) > >> + return; > >> + > >> + mutex_lock(&hisi_ptt->mutex); > >> + if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON) { > >> + hisi_ptt_trace_end(hisi_ptt); > >> + WARN(hisi_ptt_wait_trace_hw_idle(hisi_ptt), "Device is still busy"); > >> + hisi_ptt_update_aux(hisi_ptt, hisi_ptt->trace_ctrl.buf_index, true); > >> + } > >> + mutex_unlock(&hisi_ptt->mutex); > >> + > >> + hwc->state |= PERF_HES_STOPPED; > >> + perf_event_update_userpage(event); > >> + hwc->state |= PERF_HES_UPTODATE; > >> +} > >> + > >> +static int hisi_ptt_pmu_add(struct perf_event *event, int flags) > >> +{ > >> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu); > >> + struct hw_perf_event *hwc = &event->hw; > >> + int cpu = event->cpu; > >> + > >> + if (cpu == -1 && smp_processor_id() != hisi_ptt->trace_ctrl.default_cpu) > >> + return 0; > >> + > >> + hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE; > >> + > >> + if (flags & PERF_EF_START) { > >> + hisi_ptt_pmu_start(event, PERF_EF_RELOAD); > >> + if (hwc->state & PERF_HES_STOPPED) > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static void hisi_ptt_pmu_del(struct perf_event *event, int flags) > >> +{ > >> + hisi_ptt_pmu_stop(event, PERF_EF_UPDATE); > >> +} > >> + > >> +static void hisi_ptt_unregister_pmu(void *priv) > >> +{ > >> + perf_pmu_unregister(priv); > >> +} > >> + > >> +static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt) > >> +{ > >> + char *pmu_name; > >> + int ret; > >> + > >> + hisi_ptt->hisi_ptt_pmu = (struct pmu) { > >> + .module = THIS_MODULE, > >> + .capabilities = PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE, > >> + .task_ctx_nr = perf_sw_context, > >> + .attr_groups = hisi_ptt_pmu_groups, > >> + .event_init = hisi_ptt_pmu_event_init, > >> + .setup_aux = hisi_ptt_pmu_setup_aux, > >> + .free_aux = hisi_ptt_pmu_free_aux, > >> + .start = hisi_ptt_pmu_start, > >> + .stop = hisi_ptt_pmu_stop, > >> + .add = hisi_ptt_pmu_add, > >> + .del = hisi_ptt_pmu_del, > >> + }; > >> + > >> + pmu_name = devm_kasprintf(&hisi_ptt->pdev->dev, GFP_KERNEL, "hisi_ptt%u_%u", > >> + hisi_ptt->sicl_id, hisi_ptt->core_id); > >> + if (!pmu_name) > >> + return -ENOMEM; > >> + > >> + ret = perf_pmu_register(&hisi_ptt->hisi_ptt_pmu, pmu_name, -1); > >> + if (ret) > >> + return ret; > >> + > >> + return devm_add_action_or_reset(&hisi_ptt->pdev->dev, > >> + hisi_ptt_unregister_pmu, > >> + &hisi_ptt->hisi_ptt_pmu); > >> +} > >> + > >> +/* > >> + * Get RMR address if provided by the firmware. > >> + * Return 0 if the IOMMU doesn't present or the policy of the > >> + * IOMMU domain is passthrough or we get a usable RMR region. > >> + * Otherwise a negative value is returned. > >> + */ > >> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt) > >> +{ > >> + struct pci_dev *pdev = hisi_ptt->pdev; > >> + struct iommu_domain *iommu_domain; > >> + struct iommu_resv_region *region; > >> + LIST_HEAD(list); > >> + > >> + /* > >> + * Use direct DMA if IOMMU does not present or the policy of the > >> + * IOMMU domain is passthrough. > >> + */ > >> + iommu_domain = iommu_get_domain_for_dev(&pdev->dev); > >> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY) > >> + return 0; > >> + > >> + iommu_get_resv_regions(&pdev->dev, &list); > >> + list_for_each_entry(region, &list, list) > >> + if (region->type == IOMMU_RESV_DIRECT && > >> + region->length >= HISI_PTT_TRACE_BUFFER_SIZE) { > >> + hisi_ptt->trace_ctrl.has_rmr = true; > >> + hisi_ptt->trace_ctrl.rmr_addr = region->start; > >> + hisi_ptt->trace_ctrl.rmr_length = region->length; > >> + break; > >> + } > >> + > >> + iommu_put_resv_regions(&pdev->dev, &list); > >> + return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM; > >> +} > >> + > >> +static int hisi_ptt_probe(struct pci_dev *pdev, > >> + const struct pci_device_id *id) > >> +{ > >> + struct hisi_ptt *hisi_ptt; > >> + int ret; > >> + > >> + hisi_ptt = devm_kzalloc(&pdev->dev, sizeof(*hisi_ptt), GFP_KERNEL); > >> + if (!hisi_ptt) > >> + return -ENOMEM; > >> + > >> + mutex_init(&hisi_ptt->mutex); > >> + hisi_ptt->pdev = pdev; > >> + > >> + /* > >> + * Lifetime of pci_dev is longer than hisi_ptt, > >> + * so directly reference to the pci name string. > >> + */ > >> + hisi_ptt->name = pci_name(hisi_ptt->pdev); > >> + pci_set_drvdata(pdev, hisi_ptt); > >> + > >> + ret = pcim_enable_device(pdev); > >> + if (ret) { > >> + pci_err(pdev, "failed to enable device, ret = %d.\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = pcim_iomap_regions(pdev, BIT(2), hisi_ptt->name); > >> + if (ret) { > >> + pci_err(pdev, "failed to remap io memory, ret = %d.\n", ret); > >> + return ret; > >> + } > >> + > >> + hisi_ptt->iobase = pcim_iomap_table(pdev)[2]; > >> + > >> + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64)); > >> + if (ret) { > >> + pci_err(pdev, "failed to set 64 bit dma mask, ret = %d.\n", ret); > >> + return ret; > >> + } > >> + pci_set_master(pdev); > >> + > >> + ret = hisi_ptt_register_irq(hisi_ptt); > >> + if (ret) > >> + return ret; > >> + > >> + hisi_ptt_init_ctrls(hisi_ptt); > >> + > >> + ret = hisi_ptt_register_pmu(hisi_ptt); > >> + if (ret) { > >> + pci_err(pdev, "failed to register pmu device, ret = %d", ret); > >> + return ret; > >> + } > > > > If this fails the pci_free_irq_vectors() is not called. > > > > It'll be called as we've registered a devm callback when > allocating the irq vector. See hisi_ptt_register_irq(). Ah yes - thanks for pointing that out. > > > I am out of time for today - I will continue tomorrow. > > > > Sure. Thanks! > Yicong > > > Thanks, > > Mathieu > > > >> + > >> + ret = hisi_ptt_get_rmr(hisi_ptt); > >> + if (ret) { > >> + pci_err(pdev, "failed to get RMR region, ret = %d", ret); > >> + return ret; > >> + } > >> + > >> + /* Register the bus notifier for dynamically updating the filter list */ > >> + hisi_ptt->hisi_ptt_nb.notifier_call = hisi_ptt_notifier_call; > >> + ret = bus_register_notifier(&pci_bus_type, &hisi_ptt->hisi_ptt_nb); > >> + if (ret) > >> + pci_warn(pdev, "failed to register filter update notifier, ret = %d", ret); > >> + > >> + return 0; > >> +} > >> + > >> +void hisi_ptt_remove(struct pci_dev *pdev) > >> +{ > >> + struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev); > >> + > >> + bus_unregister_notifier(&pci_bus_type, &hisi_ptt->hisi_ptt_nb); > >> + > >> + /* Cancel any work that has been queued */ > >> + cancel_delayed_work_sync(&hisi_ptt->work); > >> + > >> + if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON) > >> + hisi_ptt_trace_end(hisi_ptt); > >> + > >> + hisi_ptt_free_trace_buf(hisi_ptt); > >> + hisi_ptt_release_filters(hisi_ptt); > >> +} > >> + > >> +static const struct pci_device_id hisi_ptt_id_tbl[] = { > >> + { PCI_DEVICE(PCI_VENDOR_ID_HUAWEI, 0xa12e) }, > >> + { } > >> +}; > >> +MODULE_DEVICE_TABLE(pci, hisi_ptt_id_tbl); > >> + > >> +static struct pci_driver hisi_ptt_driver = { > >> + .name = "hisi_ptt", > >> + .id_table = hisi_ptt_id_tbl, > >> + .probe = hisi_ptt_probe, > >> + .remove = hisi_ptt_remove, > >> +}; > >> +module_pci_driver(hisi_ptt_driver); > >> + > >> +MODULE_LICENSE("GPL v2"); > >> +MODULE_AUTHOR("Yicong Yang <yangyicong(a)hisilicon.com>"); > >> +MODULE_DESCRIPTION("Driver for HiSilicon PCIe tune and trace device"); > >> -- > >> 2.33.0 > >> > > . > >

4 years, 1 month

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight