diff mbox series

[v2,1/7] perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH

Message ID 20221222160328.3639989-2-james.clark@arm.com (mailing list archive)
State New, archived
Headers show
Series perf cs_etm: Basic support for virtual/kernel timestamps | expand

Commit Message

James Clark Dec. 22, 2022, 4:03 p.m. UTC
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(-)

Comments

Leo Yan Dec. 23, 2022, 2:39 a.m. UTC | #1
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.

>  	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
>
James Clark Jan. 3, 2023, 4:21 p.m. UTC | #2
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.
diff mbox series

Patch

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 */