diff mbox series

[v3,2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Message ID 20240626-misc_perf_fixes-v3-2-de3f8ed88dab@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series Assorted fixes in RISC-V PMU driver | expand

Commit Message

Atish Kumar Patra June 26, 2024, 7:23 a.m. UTC 
From: Samuel Holland <samuel.holland@sifive.com>

Currently, we stop all the counters while a new cpu is brought online.
However, the hpmevent to counter mappings are not reset. The firmware may
have some stale encoding in their mapping structure which may lead to
undesirable results. We have not encountered such scenario though.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 drivers/perf/riscv_pmu_sbi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Samuel Holland June 26, 2024, 1:24 p.m. UTC | #1 
On 2024-06-26 2:23 AM, Atish Patra wrote:
> From: Samuel Holland <samuel.holland@sifive.com>
> 
> Currently, we stop all the counters while a new cpu is brought online.
> However, the hpmevent to counter mappings are not reset. The firmware may
> have some stale encoding in their mapping structure which may lead to
> undesirable results. We have not encountered such scenario though.
> 

This needs:

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

otherwise your commit message looks fine to me.

> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index a2e4005e1fd0..94bc369a3454 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -762,7 +762,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
>  	 * which may include counters that are not enabled yet.
>  	 */
>  	sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
> -		  0, pmu->cmask, 0, 0, 0, 0);
> +		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
>  }
>  
>  static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
>
Atish Kumar Patra June 26, 2024, 4:18 p.m. UTC | #2 
On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2024-06-26 2:23 AM, Atish Patra wrote:
> > From: Samuel Holland <samuel.holland@sifive.com>
> >
> > Currently, we stop all the counters while a new cpu is brought online.
> > However, the hpmevent to counter mappings are not reset. The firmware may
> > have some stale encoding in their mapping structure which may lead to
> > undesirable results. We have not encountered such scenario though.
> >
>
> This needs:
>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>

Oops. Sorry I missed that.

@Alexandre Ghiti @Palmer Dabbelt : Can you add that while picking up
the patch or should I respin a v4 ?

> otherwise your commit message looks fine to me.
>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  drivers/perf/riscv_pmu_sbi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index a2e4005e1fd0..94bc369a3454 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -762,7 +762,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
> >        * which may include counters that are not enabled yet.
> >        */
> >       sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
> > -               0, pmu->cmask, 0, 0, 0, 0);
> > +               0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
> >  }
> >
> >  static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
> >
>
Conor Dooley June 26, 2024, 4:37 p.m. UTC | #3 
On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> <samuel.holland@sifive.com> wrote:
> >
> > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > From: Samuel Holland <samuel.holland@sifive.com>
> > >
> > > Currently, we stop all the counters while a new cpu is brought online.
> > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > have some stale encoding in their mapping structure which may lead to
> > > undesirable results. We have not encountered such scenario though.
> > >
> >
> > This needs:
> >
> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> >
> 
> Oops. Sorry I missed that.
> 
> @Alexandre Ghiti

What's Alex going to be able to do?

> @Palmer Dabbelt : Can you add that while picking up
> the patch or should I respin a v4 ?

b4 should pick the signoff up though. "perf: RISC-V: Check standard
event availability" seems to be missing your signoff though...
Conor Dooley June 26, 2024, 4:39 p.m. UTC | #4 
On Wed, Jun 26, 2024 at 05:37:07PM +0100, Conor Dooley wrote:
> On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> > On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> > <samuel.holland@sifive.com> wrote:
> > >
> > > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > > From: Samuel Holland <samuel.holland@sifive.com>
> > > >
> > > > Currently, we stop all the counters while a new cpu is brought online.
> > > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > > have some stale encoding in their mapping structure which may lead to
> > > > undesirable results. We have not encountered such scenario though.
> > > >
> > >
> > > This needs:
> > >
> > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > >
> > 
> > Oops. Sorry I missed that.
> > 
> > @Alexandre Ghiti
> 
> What's Alex going to be able to do?
> 
> > @Palmer Dabbelt : Can you add that while picking up
> > the patch or should I respin a v4 ?
> 
> b4 should pick the signoff up though. "perf: RISC-V: Check standard
> event availability" seems to be missing your signoff though...

Huh, this doesn't really make sense. I meant:
	b4 should pick the signoff up, though "perf: RISC-V: Check standard
	event availability" seems to be missing your signoff...
Atish Kumar Patra June 26, 2024, 8:40 p.m. UTC | #5 
On Wed, Jun 26, 2024 at 9:39 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 05:37:07PM +0100, Conor Dooley wrote:
> > On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> > > On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> > > <samuel.holland@sifive.com> wrote:
> > > >
> > > > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > > > From: Samuel Holland <samuel.holland@sifive.com>
> > > > >
> > > > > Currently, we stop all the counters while a new cpu is brought online.
> > > > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > > > have some stale encoding in their mapping structure which may lead to
> > > > > undesirable results. We have not encountered such scenario though.
> > > > >
> > > >
> > > > This needs:
> > > >
> > > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > > >
> > >
> > > Oops. Sorry I missed that.
> > >
> > > @Alexandre Ghiti
> >
> > What's Alex going to be able to do?
> >

He is collecting the fixes patches in the RISC-V tree and pinged for
revision for this patch last week.

> > > @Palmer Dabbelt : Can you add that while picking up
> > > the patch or should I respin a v4 ?
> >
> > b4 should pick the signoff up though. "perf: RISC-V: Check standard
> > event availability" seems to be missing your signoff though...
>
> Huh, this doesn't really make sense. I meant:
>         b4 should pick the signoff up, though "perf: RISC-V: Check standard
>         event availability" seems to be missing your signoff...

Strange. I modified and sent the patch using b4 as well. It's missing
my sign off too.
Conor Dooley June 26, 2024, 10:11 p.m. UTC | #6 
On Wed, Jun 26, 2024 at 01:40:52PM -0700, Atish Kumar Patra wrote:

> > > > @Palmer Dabbelt : Can you add that while picking up
> > > > the patch or should I respin a v4 ?
> > >
> > > b4 should pick the signoff up though. "perf: RISC-V: Check standard
> > > event availability" seems to be missing your signoff though...
> >
> > Huh, this doesn't really make sense. I meant:
> >         b4 should pick the signoff up, though "perf: RISC-V: Check standard
> >         event availability" seems to be missing your signoff...
> 
> Strange. I modified and sent the patch using b4 as well. It's missing
> my sign off too.

`b4 shazam` should pick up trailers provided on the list, signoffs
included. `b4 shazam -s` will add yours. TBH, I am not sure why that is
not the default behaviour.

Cheers,
Conor.
Konstantin Ryabitsev June 28, 2024, 2:29 p.m. UTC | #7 
On Wed, Jun 26, 2024 at 11:11:54PM GMT, Conor Dooley wrote:
> > Strange. I modified and sent the patch using b4 as well. It's missing
> > my sign off too.
> 
> `b4 shazam` should pick up trailers provided on the list, signoffs
> included. `b4 shazam -s` will add yours. TBH, I am not sure why that is
> not the default behaviour.

Some projects don't use the DCO model, so they don't require Signed-off-by
trailers.

-K
diff mbox series

Patch

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index a2e4005e1fd0..94bc369a3454 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -762,7 +762,7 @@  static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
 	 * which may include counters that are not enabled yet.
 	 */
 	sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
-		  0, pmu->cmask, 0, 0, 0, 0);
+		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
 }
 
 static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)