linux-kernel - Re: [PATCH] drivers/perf: riscv: Remove redundant macro check

Open Source and information security mailing list archives

Message-ID: <20240709-fame-uptown-c936014cd66a@spud>
Date: Tue, 9 Jul 2024 22:04:55 +0100
From: Conor Dooley <conor@...nel.org>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: Xiao Wang <xiao.w.wang@...el.com>,
	Alexandre Ghiti <alexghiti@...osinc.com>, paul.walmsley@...ive.com,
	palmer@...belt.com, aou@...s.berkeley.edu, atishp@...shpatra.org,
	anup@...infault.org, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/perf: riscv: Remove redundant macro check

On Tue, Jul 09, 2024 at 01:57:47PM -0700, Charlie Jenkins wrote:
> On Tue, Jul 09, 2024 at 09:44:17PM +0100, Conor Dooley wrote:
> > On Tue, Jul 09, 2024 at 01:29:42PM -0700, Charlie Jenkins wrote:
> > > On Mon, Jul 08, 2024 at 01:22:11PM +0100, Conor Dooley wrote:
> > > > On Mon, Jul 08, 2024 at 08:12:24PM +0800, Xiao Wang wrote:
> > > > > The macro CONFIG_RISCV_PMU must have been defined when riscv_pmu.c gets
> > > > > compiled, so this patch removes the redundant check.
> > > > 
> > > > Did you investigate why this define was added? Why do you think that it
> > > > is redundant, rather than checking the incorrect config option?
> > > 
> > > This file is only compiled with CONFIG_RISCV_PMU:
> > 
> > I might be ill, but I can still read. I was not disagreeing with Xiao
> > that the condition is redundant as written - I want to know whether they
> > made sure that this check was intentionally using CONFIG_RISCV_PMU in the
> > first place, or if another option should have been here instead.
> 
> Makes sense! Looking through the lists I see this RFC from Atish where
> he introduced a different config option for this
> "CONFIG_RISCV_PMU_COMMON"[1]. I wonder if something got confused in the
> development of these two patches.

Perhaps.. What I was worried about was the wrong option being here
(maybe that it should have been RISCV_PMU_SBI or similar) and depending
on how the kernel is configured, userspace would get the wrong info
here. But maybe it is innocuous your theory would suggest, and there's
nothing to worry about. But that's for someone with a functioning brain
to figure out ;)

Cheers,
Conor.

> 
> Link:
> https://lore.kernel.org/lkml/20240217005738.3744121-12-atishp@rivosinc.com/
> [1]
> 
> > > # drivers/perf/Makefile
> > > obj-$(CONFIG_RISCV_PMU) += riscv_pmu.o
> > > 
> > > So having this check does seem redundant. I am copying Alex as it looks
> > > like he wrote this.
> > > 
> > > > > 
> > > > > Signed-off-by: Xiao Wang <xiao.w.wang@...el.com>
> > > > > ---
> > > > >  drivers/perf/riscv_pmu.c | 2 --
> > > > >  1 file changed, 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c
> > > > > index 0a02e85a8951..7644147d50b4 100644
> > > > > --- a/drivers/perf/riscv_pmu.c
> > > > > +++ b/drivers/perf/riscv_pmu.c
> > > > > @@ -39,7 +39,6 @@ void arch_perf_update_userpage(struct perf_event *event,
> > > > >  	userpg->cap_user_time_short = 0;
> > > > >  	userpg->cap_user_rdpmc = riscv_perf_user_access(event);
> > > > >  
> > > > > -#ifdef CONFIG_RISCV_PMU
> > > > >  	/*
> > > > >  	 * The counters are 64-bit but the priv spec doesn't mandate all the
> > > > >  	 * bits to be implemented: that's why, counter width can vary based on
> > > > > @@ -47,7 +46,6 @@ void arch_perf_update_userpage(struct perf_event *event,
> > > > >  	 */
> > > > >  	if (userpg->cap_user_rdpmc)
> > > > >  		userpg->pmc_width = to_riscv_pmu(event->pmu)->ctr_get_width(event->hw.idx) + 1;
> > > > > -#endif
> > > > >  
> > > > >  	do {
> > > > >  		rd = sched_clock_read_begin(&seq);
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > linux-riscv mailing list
> > > > > linux-riscv@...ts.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > 
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives