The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a
few places, so add two utility functions to cover it. Also just use
perf_pmu__scan_file() instead of pmu_type() which already does the same
thing.
No functional changes.
Signed-off-by: James Clark <james.clark@arm.com>
---
tools/perf/arch/arm/util/auxtrace.c | 5 +-
tools/perf/arch/x86/util/pmu.c | 12 +--
tools/perf/util/pmu.c | 110 +++++++++++-----------------
tools/perf/util/pmu.h | 5 +-
4 files changed, 49 insertions(+), 83 deletions(-)
diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
index deeb163999ce..3cb4a6a16112 100644
--- a/tools/perf/arch/arm/util/auxtrace.c
+++ b/tools/perf/arch/arm/util/auxtrace.c
@@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err)
{
- const char *sysfs = sysfs__mountpoint();
struct perf_pmu **hisi_ptt_pmus = NULL;
struct dirent *dent;
char path[PATH_MAX];
DIR *dir = NULL;
int idx = 0;
- snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
+ perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);
dir = opendir(path);
if (!dir) {
- pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH);
+ pr_err("can't read directory '%s'\n", path);
*err = -EINVAL;
return NULL;
}
diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
index 74d69db1ea99..e3456d4b9063 100644
--- a/tools/perf/arch/x86/util/pmu.c
+++ b/tools/perf/arch/x86/util/pmu.c
@@ -15,8 +15,6 @@
#include "../../../util/pmu.h"
#include "../../../util/fncache.h"
-#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias"
-
struct pmu_alias {
char *name;
char *alias;
@@ -72,18 +70,14 @@ static int setup_pmu_alias_list(void)
char path[PATH_MAX];
DIR *dir;
struct dirent *dent;
- const char *sysfs = sysfs__mountpoint();
struct pmu_alias *pmu_alias;
char buf[MAX_PMU_NAME_LEN];
FILE *file;
int ret = -ENOMEM;
- if (!sysfs)
+ if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX))
return -1;
- snprintf(path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
-
dir = opendir(path);
if (!dir)
return -errno;
@@ -93,9 +87,7 @@ static int setup_pmu_alias_list(void)
!strcmp(dent->d_name, ".."))
continue;
- snprintf(path, PATH_MAX,
- TEMPLATE_ALIAS, sysfs, dent->d_name);
-
+ perf_pmu__pathname_scnprintf(path, PATH_MAX, dent->d_name, "alias");
if (!file_available(path))
continue;
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 2bdeb89352e7..827c1a6bb99a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -107,14 +107,10 @@ int perf_pmu__format_parse(char *dir, struct list_head *head)
static int pmu_format(const char *name, struct list_head *format)
{
char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
+ if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "format"))
return -1;
- snprintf(path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH "%s/format", sysfs, name);
-
if (!file_available(path))
return 0;
@@ -513,14 +509,10 @@ static int pmu_aliases_parse(char *dir, struct list_head *head)
static int pmu_aliases(const char *name, struct list_head *head)
{
char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
+ if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "events"))
return -1;
- snprintf(path, PATH_MAX,
- "%s/bus/event_source/devices/%s/events", sysfs, name);
-
if (!file_available(path))
return 0;
@@ -554,52 +546,16 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
return 0;
}
-/*
- * Reading/parsing the default pmu type value, which should be
- * located at:
- * /sys/bus/event_source/devices/<dev>/type as sysfs attribute.
- */
-static int pmu_type(const char *name, __u32 *type)
-{
- char path[PATH_MAX];
- FILE *file;
- int ret = 0;
- const char *sysfs = sysfs__mountpoint();
-
- if (!sysfs)
- return -1;
-
- snprintf(path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH "%s/type", sysfs, name);
-
- if (access(path, R_OK) < 0)
- return -1;
-
- file = fopen(path, "r");
- if (!file)
- return -EINVAL;
-
- if (1 != fscanf(file, "%u", type))
- ret = -1;
-
- fclose(file);
- return ret;
-}
-
/* Add all pmus in sysfs to pmu list: */
static void pmu_read_sysfs(void)
{
char path[PATH_MAX];
DIR *dir;
struct dirent *dent;
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
+ if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX))
return;
- snprintf(path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
-
dir = opendir(path);
if (!dir)
return;
@@ -696,14 +652,9 @@ static char *pmu_id(const char *name)
static int is_arm_pmu_core(const char *name)
{
char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
+ if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpus"))
return 0;
-
- /* Look for cpu sysfs (specific to arm) */
- scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
- sysfs, name);
return file_available(path);
}
@@ -969,11 +920,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
return NULL;
/*
- * Check the type first to avoid unnecessary work.
+ * Check the aliases first to avoid unnecessary work.
*/
- if (pmu_type(name, &type))
- return NULL;
-
if (pmu_aliases(name, &aliases))
return NULL;
@@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
pmu->cpus = pmu_cpumask(name);
pmu->name = strdup(name);
+
if (!pmu->name)
goto err;
+ /* Read type, and ensure that 1 value (type) is successfully assigned */
+ if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1)
+ goto err;
+
alias_name = pmu_find_alias_name(name);
if (alias_name) {
pmu->alias_name = strdup(alias_name);
@@ -1786,16 +1739,11 @@ bool pmu_have_event(const char *pname, const char *name)
static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name)
{
char path[PATH_MAX];
- const char *sysfs;
- sysfs = sysfs__mountpoint();
- if (!sysfs)
+ if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name) ||
+ !file_available(path))
return NULL;
- snprintf(path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH "%s/%s", sysfs, pmu->name, name);
- if (!file_available(path))
- return NULL;
return fopen(path, "r");
}
@@ -1849,7 +1797,6 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
{
struct stat st;
char caps_path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
DIR *caps_dir;
struct dirent *evt_ent;
@@ -1858,12 +1805,9 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
pmu->nr_caps = 0;
- if (!sysfs)
+ if (!perf_pmu__pathname_scnprintf(caps_path, PATH_MAX, pmu->name, "caps"))
return -1;
- snprintf(caps_path, PATH_MAX,
- "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
-
if (stat(caps_path, &st) < 0) {
pmu->caps_initialized = true;
return 0; /* no error if caps does not exist */
@@ -1993,3 +1937,31 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
*ucpus_ptr = unmatched_cpus;
return 0;
}
+
+int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size)
+{
+ const char *sysfs = sysfs__mountpoint();
+
+ if (!sysfs)
+ return 0;
+ return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs);
+}
+
+/*
+ * Fill 'buf' with the path to a file or folder in 'pmu_name' in
+ * sysfs. For example if pmu_name = "cs_etm" and 'filename' = "format"
+ * then pathname will be filled with
+ * "/sys/bus/event_source/devices/cs_etm/format"
+ *
+ * Return 0 if the sysfs mountpoint couldn't be found or if no
+ * characters were written.
+ */
+int perf_pmu__pathname_scnprintf(char *buf, size_t size,
+ const char *pmu_name, const char *filename)
+{
+ char base_path[PATH_MAX];
+
+ if (!perf_pmu__event_source_devices_scnprintf(base_path, PATH_MAX))
+ return 0;
+ return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 69ca0004f94f..2f2bb0286e2a 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -22,7 +22,6 @@ enum {
};
#define PERF_PMU_FORMAT_BITS 64
-#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
#define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus"
#define MAX_PMU_NAME_LEN 128
@@ -259,4 +258,8 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
char *pmu_find_real_name(const char *name);
char *pmu_find_alias_name(const char *name);
+int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size);
+int perf_pmu__pathname_scnprintf(char *buf, size_t size,
+ const char *pmu_name, const char *filename);
+
#endif /* __PMU_H */
--
2.25.1On Thu, Dec 22, 2022 at 04:03:21PM +0000, James Clark wrote:
> The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a
> few places, so add two utility functions to cover it. Also just use
> perf_pmu__scan_file() instead of pmu_type() which already does the same
> thing.
>
> No functional changes.
After I read the article: https://lwn.net/Articles/69419/, good to see
this patch uses scnprintf() to replace snprintf().
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> tools/perf/arch/arm/util/auxtrace.c | 5 +-
> tools/perf/arch/x86/util/pmu.c | 12 +--
> tools/perf/util/pmu.c | 110 +++++++++++-----------------
> tools/perf/util/pmu.h | 5 +-
> 4 files changed, 49 insertions(+), 83 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
> index deeb163999ce..3cb4a6a16112 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
> @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
>
> static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err)
> {
> - const char *sysfs = sysfs__mountpoint();
> struct perf_pmu **hisi_ptt_pmus = NULL;
> struct dirent *dent;
> char path[PATH_MAX];
> DIR *dir = NULL;
> int idx = 0;
>
> - snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
> + perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);
Nitpick: since 'path' is an array, a good practice is to use
'sizeof(path)' rather than 'PATH_MAX' for passing the length.
> dir = opendir(path);
> if (!dir) {
> - pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH);
> + pr_err("can't read directory '%s'\n", path);
> *err = -EINVAL;
> return NULL;
> }
> diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
> index 74d69db1ea99..e3456d4b9063 100644
> --- a/tools/perf/arch/x86/util/pmu.c
> +++ b/tools/perf/arch/x86/util/pmu.c
> @@ -15,8 +15,6 @@
> #include "../../../util/pmu.h"
> #include "../../../util/fncache.h"
>
> -#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias"
> -
> struct pmu_alias {
> char *name;
> char *alias;
> @@ -72,18 +70,14 @@ static int setup_pmu_alias_list(void)
> char path[PATH_MAX];
> DIR *dir;
> struct dirent *dent;
> - const char *sysfs = sysfs__mountpoint();
> struct pmu_alias *pmu_alias;
> char buf[MAX_PMU_NAME_LEN];
> FILE *file;
> int ret = -ENOMEM;
>
> - if (!sysfs)
> + if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX))
> return -1;
>
> - snprintf(path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
> -
> dir = opendir(path);
> if (!dir)
> return -errno;
> @@ -93,9 +87,7 @@ static int setup_pmu_alias_list(void)
> !strcmp(dent->d_name, ".."))
> continue;
>
> - snprintf(path, PATH_MAX,
> - TEMPLATE_ALIAS, sysfs, dent->d_name);
> -
> + perf_pmu__pathname_scnprintf(path, PATH_MAX, dent->d_name, "alias");
> if (!file_available(path))
> continue;
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 2bdeb89352e7..827c1a6bb99a 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -107,14 +107,10 @@ int perf_pmu__format_parse(char *dir, struct list_head *head)
> static int pmu_format(const char *name, struct list_head *format)
> {
> char path[PATH_MAX];
> - const char *sysfs = sysfs__mountpoint();
>
> - if (!sysfs)
> + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "format"))
> return -1;
>
> - snprintf(path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH "%s/format", sysfs, name);
> -
> if (!file_available(path))
> return 0;
>
> @@ -513,14 +509,10 @@ static int pmu_aliases_parse(char *dir, struct list_head *head)
> static int pmu_aliases(const char *name, struct list_head *head)
> {
> char path[PATH_MAX];
> - const char *sysfs = sysfs__mountpoint();
>
> - if (!sysfs)
> + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "events"))
> return -1;
>
> - snprintf(path, PATH_MAX,
> - "%s/bus/event_source/devices/%s/events", sysfs, name);
> -
> if (!file_available(path))
> return 0;
>
> @@ -554,52 +546,16 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
> return 0;
> }
>
> -/*
> - * Reading/parsing the default pmu type value, which should be
> - * located at:
> - * /sys/bus/event_source/devices/<dev>/type as sysfs attribute.
> - */
> -static int pmu_type(const char *name, __u32 *type)
> -{
> - char path[PATH_MAX];
> - FILE *file;
> - int ret = 0;
> - const char *sysfs = sysfs__mountpoint();
> -
> - if (!sysfs)
> - return -1;
> -
> - snprintf(path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH "%s/type", sysfs, name);
> -
> - if (access(path, R_OK) < 0)
> - return -1;
> -
> - file = fopen(path, "r");
> - if (!file)
> - return -EINVAL;
> -
> - if (1 != fscanf(file, "%u", type))
> - ret = -1;
> -
> - fclose(file);
> - return ret;
> -}
> -
> /* Add all pmus in sysfs to pmu list: */
> static void pmu_read_sysfs(void)
> {
> char path[PATH_MAX];
> DIR *dir;
> struct dirent *dent;
> - const char *sysfs = sysfs__mountpoint();
>
> - if (!sysfs)
> + if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX))
> return;
>
> - snprintf(path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
> -
> dir = opendir(path);
> if (!dir)
> return;
> @@ -696,14 +652,9 @@ static char *pmu_id(const char *name)
> static int is_arm_pmu_core(const char *name)
> {
> char path[PATH_MAX];
> - const char *sysfs = sysfs__mountpoint();
>
> - if (!sysfs)
> + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpus"))
> return 0;
> -
> - /* Look for cpu sysfs (specific to arm) */
> - scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
> - sysfs, name);
> return file_available(path);
> }
>
> @@ -969,11 +920,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
> return NULL;
>
> /*
> - * Check the type first to avoid unnecessary work.
> + * Check the aliases first to avoid unnecessary work.
> */
> - if (pmu_type(name, &type))
> - return NULL;
> -
> if (pmu_aliases(name, &aliases))
> return NULL;
>
> @@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
>
> pmu->cpus = pmu_cpumask(name);
> pmu->name = strdup(name);
> +
> if (!pmu->name)
> goto err;
>
> + /* Read type, and ensure that 1 value (type) is successfully assigned */
Maybe I don't understand well, seems to me a better comment is:
/* Read type, and ensure that type value is successfully assigned (return 1) */
The rest looks good to me. With addressing above two comments:
Reviewed-by: Leo Yan <leo.yan@linaro.org>
> + if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1)
> + goto err;
> +
> alias_name = pmu_find_alias_name(name);
> if (alias_name) {
> pmu->alias_name = strdup(alias_name);
> @@ -1786,16 +1739,11 @@ bool pmu_have_event(const char *pname, const char *name)
> static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name)
> {
> char path[PATH_MAX];
> - const char *sysfs;
>
> - sysfs = sysfs__mountpoint();
> - if (!sysfs)
> + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name) ||
> + !file_available(path))
> return NULL;
>
> - snprintf(path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH "%s/%s", sysfs, pmu->name, name);
> - if (!file_available(path))
> - return NULL;
> return fopen(path, "r");
> }
>
> @@ -1849,7 +1797,6 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> {
> struct stat st;
> char caps_path[PATH_MAX];
> - const char *sysfs = sysfs__mountpoint();
> DIR *caps_dir;
> struct dirent *evt_ent;
>
> @@ -1858,12 +1805,9 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>
> pmu->nr_caps = 0;
>
> - if (!sysfs)
> + if (!perf_pmu__pathname_scnprintf(caps_path, PATH_MAX, pmu->name, "caps"))
> return -1;
>
> - snprintf(caps_path, PATH_MAX,
> - "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
> -
> if (stat(caps_path, &st) < 0) {
> pmu->caps_initialized = true;
> return 0; /* no error if caps does not exist */
> @@ -1993,3 +1937,31 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
> *ucpus_ptr = unmatched_cpus;
> return 0;
> }
> +
> +int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size)
> +{
> + const char *sysfs = sysfs__mountpoint();
> +
> + if (!sysfs)
> + return 0;
> + return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs);
> +}
> +
> +/*
> + * Fill 'buf' with the path to a file or folder in 'pmu_name' in
> + * sysfs. For example if pmu_name = "cs_etm" and 'filename' = "format"
> + * then pathname will be filled with
> + * "/sys/bus/event_source/devices/cs_etm/format"
> + *
> + * Return 0 if the sysfs mountpoint couldn't be found or if no
> + * characters were written.
> + */
> +int perf_pmu__pathname_scnprintf(char *buf, size_t size,
> + const char *pmu_name, const char *filename)
> +{
> + char base_path[PATH_MAX];
> +
> + if (!perf_pmu__event_source_devices_scnprintf(base_path, PATH_MAX))
> + return 0;
> + return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);
> +}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 69ca0004f94f..2f2bb0286e2a 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -22,7 +22,6 @@ enum {
> };
>
> #define PERF_PMU_FORMAT_BITS 64
> -#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
> #define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus"
> #define MAX_PMU_NAME_LEN 128
>
> @@ -259,4 +258,8 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
>
> char *pmu_find_real_name(const char *name);
> char *pmu_find_alias_name(const char *name);
> +int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size);
> +int perf_pmu__pathname_scnprintf(char *buf, size_t size,
> + const char *pmu_name, const char *filename);
> +
> #endif /* __PMU_H */
> --
> 2.25.1
>
On 23/12/2022 02:39, Leo Yan wrote:
> On Thu, Dec 22, 2022 at 04:03:21PM +0000, James Clark wrote:
>> The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a
>> few places, so add two utility functions to cover it. Also just use
>> perf_pmu__scan_file() instead of pmu_type() which already does the same
>> thing.
>>
>> No functional changes.
>
> After I read the article: https://lwn.net/Articles/69419/, good to see
> this patch uses scnprintf() to replace snprintf().
>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> tools/perf/arch/arm/util/auxtrace.c | 5 +-
>> tools/perf/arch/x86/util/pmu.c | 12 +--
>> tools/perf/util/pmu.c | 110 +++++++++++-----------------
>> tools/perf/util/pmu.h | 5 +-
>> 4 files changed, 49 insertions(+), 83 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
>> index deeb163999ce..3cb4a6a16112 100644
>> --- a/tools/perf/arch/arm/util/auxtrace.c
>> +++ b/tools/perf/arch/arm/util/auxtrace.c
>> @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
>>
>> static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err)
>> {
>> - const char *sysfs = sysfs__mountpoint();
>> struct perf_pmu **hisi_ptt_pmus = NULL;
>> struct dirent *dent;
>> char path[PATH_MAX];
>> DIR *dir = NULL;
>> int idx = 0;
>>
>> - snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
>> + perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);
>
> Nitpick: since 'path' is an array, a good practice is to use
> 'sizeof(path)' rather than 'PATH_MAX' for passing the length.
Done
[...]
>>
>> @@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
>>
>> pmu->cpus = pmu_cpumask(name);
>> pmu->name = strdup(name);
>> +
>> if (!pmu->name)
>> goto err;
>>
>> + /* Read type, and ensure that 1 value (type) is successfully assigned */
>
> Maybe I don't understand well, seems to me a better comment is:
>
> /* Read type, and ensure that type value is successfully assigned (return 1) */
>
Changed it in V3. I was trying to portray that the function returns how
many variables were successfully assigned, which is slightly different
to 1 == success. But it's probably clearer your way.
>
> The rest looks good to me. With addressing above two comments:
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
Thanks for the review.
© 2016 - 2025 Red Hat, Inc.