Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Jun 2022 13:32:20 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Atish Patra <atishp@atishpatra.org>,
        Anup Patel <anup@brainfault.org>,
        =?iso-8859-1?Q?Jo=E3o_M=E1rio?= Domingos 
        <joao.mario@tecnico.ulisboa.pt>, linux@yadro.com,
        Nikita Shubin <n.shubin@yadro.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        linux-riscv@lists.infradead.org
Subject: Re: [PATCH v4 2/5] perf tools riscv: Add support for get_cpuid_str
 function
Message-ID: <YrXnFAaT/B1In/+t@kernel.org>
References: <20220624160117.3206-1-nikita.shubin@maquefel.me>
 <20220624160117.3206-3-nikita.shubin@maquefel.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220624160117.3206-3-nikita.shubin@maquefel.me>
Precedence: bulk

Em Fri, Jun 24, 2022 at 07:00:52PM +0300, Nikita Shubin escreveu:
> From: Nikita Shubin <n.shubin@yadro.com>
> 
> The get_cpuid_str function returns the string that
> contains values of MVENDORID, MARCHID and MIMPID in
> hex format separated by coma.
> 
> The values themselves are taken from first cpu entry
> in "/proc/cpuid" that contains "mvendorid", "marchid"
> and "mimpid".
> 
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  tools/perf/arch/riscv/util/Build    |   1 +
>  tools/perf/arch/riscv/util/header.c | 109 ++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+)
>  create mode 100644 tools/perf/arch/riscv/util/header.c
> 
> diff --git a/tools/perf/arch/riscv/util/Build b/tools/perf/arch/riscv/util/Build
> index 7d3050134ae0..603dbb5ae4dc 100644
> --- a/tools/perf/arch/riscv/util/Build
> +++ b/tools/perf/arch/riscv/util/Build
> @@ -1,4 +1,5 @@
>  perf-y += perf_regs.o
> +perf-y += header.o
>  
>  perf-$(CONFIG_DWARF) += dwarf-regs.o
>  perf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> diff --git a/tools/perf/arch/riscv/util/header.c b/tools/perf/arch/riscv/util/header.c
> new file mode 100644
> index 000000000000..53e8ddf7990b
> --- /dev/null
> +++ b/tools/perf/arch/riscv/util/header.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Implementation of get_cpuid().
> + *
> + * Author: Nikita Shubin <n.shubin@yadro.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <api/fs/fs.h>
> +#include <errno.h>
> +#include "../../util/debug.h"
> +#include "../../util/header.h"
> +
> +#define CPUINFO_MVEN	"mvendorid"
> +#define CPUINFO_MARCH	"marchid"
> +#define CPUINFO_MIMP	"mimpid"
> +#define CPUINFO		"/proc/cpuinfo"
> +
> +static char *_get_field(const char *line)
> +{
> +	char *line2, *nl;
> +
> +	line2 = strrchr(line, ' ');
> +	if (!line2)
> +		return NULL;
> +
> +	line2++;
> +	nl = strrchr(line, '\n');
> +	if (!nl)
> +		return NULL;
> +
> +	return strndup(line2, nl - line2);
> +}
> +
> +static char *_get_cpuid(void)
> +{
> +	char *line = NULL;
> +	char *mvendorid = NULL;
> +	char *marchid = NULL;
> +	char *mimpid = NULL;
> +	char *cpuid = NULL;
> +	int read;
> +	unsigned long line_sz;
> +	FILE *cpuinfo;
> +
> +	cpuinfo = fopen(CPUINFO, "r");
> +	if (cpuinfo == NULL)
> +		return cpuid;
> +
> +	while ((read = getline(&line, &line_sz, cpuinfo)) != -1) {
> +		if (!strncmp(line, CPUINFO_MVEN, strlen(CPUINFO_MVEN))) {
> +			mvendorid = _get_field(line);
> +			if (!mvendorid)
> +				goto free;
> +		} else if (!strncmp(line, CPUINFO_MARCH, strlen(CPUINFO_MARCH))) {
> +			marchid = _get_field(line);
> +			if (!marchid)
> +				goto free;
> +		} else if (!strncmp(line, CPUINFO_MIMP, strlen(CPUINFO_MIMP))) {
> +			mimpid = _get_field(line);
> +			if (!mimpid)
> +				goto free;
> +
> +			break;
> +		}
> +	}
> +
> +	if (!mvendorid || !marchid || !mimpid) {
> +		cpuid = NULL;
> +		goto free;
> +	}
> +
> +	if (asprintf(&cpuid, "%s-%s-%s", mvendorid, marchid, mimpid) < 0)
> +		cpuid = NULL;
> +
> +free:
> +	fclose(cpuinfo);
> +
> +	if (mvendorid)
> +		free(mvendorid);
> +
> +	if (marchid)
> +		free(marchid);
> +
> +	if (mimpid)
> +		free(mimpid);

just use:

	
	free(mvendorid);
	free(marchid);
	free(mimpid);

fewer lines, free() accepts NULL.

> +
> +	return cpuid;
> +}
> +
> +int get_cpuid(char *buffer, size_t sz)
> +{
> +	char *cpuid = _get_cpuid();
> +
> +	if (sz < strlen(cpuid)) {
> +		free(cpuid);
> +		return -EINVAL;
> +	}
> +
> +	scnprintf(buffer, sz, "%s", cpuid);

You're leaking cpuid here.

> +	return 0;
> +}
> +
> +char *
> +get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> +{
> +	return _get_cpuid();
> +}
> -- 
> 2.35.1

-- 

- Arnaldo