[PATCH] drivers/perf: riscv: Remove redundant macro check

[Conor Dooley]

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 at intel.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 at lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20240709/883c0fe9/attachment.sig>