Re: [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Missing explicit header inclusions for POSIX libc functions (`free()`) and constants (`UINT_MAX`), violating musl libc build compatibility guidelines.
- [Low] The newly added header `gtrace.h` is completely unused, its macro is re-defined locally, and it is structurally broken due to missing type dependencies.
- [Low] The function `parse_event()` is used without including its corresponding header file.
--

--- Patch [12]: [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities ---
commit dc36885ec60bdf3420446c3aec420428e7cdc32a
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

perf tools: Add RISC-V trace PMU record capabilities

Introduce the required auxiliary API functions allowing the perf core
to interact with gtrace perf driver. On RISC-V systems the gtrace
PMU uses the RISC-V E-Trace components and optionally an ATB Bridge
component for connecting to ARM-side sinks, if it is present on the SoC.

> diff --git a/tools/perf/arch/riscv/util/auxtrace.c b/tools/perf/arch/riscv/util/auxtrace.c
> new file mode 100644
> index 0000000000000..39c31be9f8966
> --- /dev/null
> +++ b/tools/perf/arch/riscv/util/auxtrace.c
> @@ -0,0 +1,217 @@
[ ... ]
> +#include <internal/lib.h>
> +#include "../../../util/auxtrace.h"
> +#include "../../../util/cpumap.h"
> +#include "../../../util/debug.h"
> +#include "../../../util/event.h"
> +#include "../../../util/evlist.h"
> +#include "../../../util/evsel.h"
> +#include "../../../util/pmu.h"
> +#include "../../../util/record.h"
> +#include "../../../util/session.h"
> +
> +
> +#define GTRACE_PMU_NAME	"gtrace"
> +#define GTRACE_AUXTRACE_PRIV_SIZE	sizeof(u64)

[Severity: Low]
Is this macro definition duplicating the one provided in the newly added
tools/perf/util/gtrace.h? It looks like that header might have been intended
to be included here instead of redefining the macro locally.

> +#define KiB(x)	((x) * 1024)
> +#define MiB(x)	((x) * 1024 * 1024)
[ ... ]
> +static int rvtrace_set_auxtrace_mmap_page(struct record_opts *opts)
> +{
> +	bool privileged = perf_event_paranoid_check(-1);
> +
> +	if (!opts->full_auxtrace)
> +		return 0;
> +
> +	if (opts->full_auxtrace && !opts->auxtrace_mmap_pages) {
> +		if (privileged) {
> +			opts->auxtrace_mmap_pages = MiB(16) / page_size;
> +		} else {
> +			opts->auxtrace_mmap_pages = KiB(128) / page_size;
> +			if (opts->mmap_pages == UINT_MAX)

[Severity: Low]
Does this code rely on implicit header inclusion for UINT_MAX? Including
limits.h directly might be required to ensure musl libc compatibility.

> +				opts->mmap_pages = KiB(256) / page_size;
> +		}
> +	}
[ ... ]
> +static int rvtrace_recording_options(struct auxtrace_record *itr, struct evlist *evlist,
> +				     struct record_opts *opts)
> +{
[ ... ]
> +	/* Add dummy event to keep tracking */
> +	err = parse_event(evlist, "dummy:u");

[Severity: Low]
Could this trigger an implicit function declaration warning? It seems
util/parse-events.h is missing from the includes at the top of the file.

> +	if (err)
> +		return err;
[ ... ]
> +static void rvtrace_recording_free(struct auxtrace_record *itr)
> +{
> +	struct rvtrace_recording *ptr = container_of(itr, struct rvtrace_recording, itr);
> +
> +	free(ptr);

[Severity: Low]
Does this code rely on implicit header inclusion for free()? Explicitly
including stdlib.h is usually necessary for musl compatibility.

> +}
[ ... ]
> diff --git a/tools/perf/util/gtrace.h b/tools/perf/util/gtrace.h
> new file mode 100644
> index 0000000000000..4b87bdaf55c69
> --- /dev/null
> +++ b/tools/perf/util/gtrace.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2026 Qualcomm Technologies, Inc.
> + */
> +
> +#ifndef INCLUDE__UTIL_PERF_GTRACE_H__
> +#define INCLUDE__UTIL_PERF_GTRACE_H__
> +
> +#define GTRACE_AUXTRACE_PRIV_SIZE      sizeof(u64)

[Severity: Low]
Is this header file completely unused, given that GTRACE_AUXTRACE_PRIV_SIZE
was redefined locally in tools/perf/arch/riscv/util/auxtrace.c?

Additionally, does this macro need linux/types.h to be included so that u64
is defined when this header is used?

> +
> +#endif
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=12