| Message ID | 0ea4559261ea394f827c9aee5168c77a60aaee03.1684946389.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] perf/arm-cmn: Fix DTC reset | expand |
I've been running this all day on Graviton2 and Graviton3 instances with my reproduction method and have not seen it reappear. From the evidence, this has solved the issue. -Geoff On Wed, 24 May 2023, Robin Murphy wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > It turns out that my naive DTC reset logic fails to work as intended, > since, after checking with the hardware designers, the PMU actually > needs to be fully enabled in order to correctly clear any pending > overflows. Therefore, invert the sequence to start with turning on both > enables so that we can reliably get the DTCs into a known state, then > moving to our normal counters-stopped state from there. Since all the > DTM counters have already been unpaired during the initial discovery > pass, we just need to additionally reset the cycle counters to ensure > that no other unexpected overflows occur during this period. > > Fixes: 0ba64770a2f2 ("perf: Add Arm CMN-600 PMU driver") > Reported-by: Geoff Blake <blakgeof@amazon.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > v2: Changed just about everything > > drivers/perf/arm-cmn.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index b57a0472ea0d..cd2648980540 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1899,9 +1899,10 @@ static int arm_cmn_init_dtc(struct arm_cmn *cmn, struct arm_cmn_node *dn, int id > if (dtc->irq < 0) > return dtc->irq; > > - writel_relaxed(0, dtc->base + CMN_DT_PMCR); > + writel_relaxed(CMN_DT_DTC_CTL_DT_EN, dtc->base + CMN_DT_DTC_CTL); > + writel_relaxed(CMN_DT_PMCR_PMU_EN | CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR); > + writeq_relaxed(0, dtc->base + CMN_DT_PMCCNTR); > writel_relaxed(0x1ff, dtc->base + CMN_DT_PMOVSR_CLR); > - writel_relaxed(CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR); > > return 0; > } > @@ -1961,7 +1962,7 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn) > dn->type = CMN_TYPE_CCLA; > } > > - writel_relaxed(CMN_DT_DTC_CTL_DT_EN, cmn->dtc[0].base + CMN_DT_DTC_CTL); > + arm_cmn_set_state(cmn, CMN_STATE_DISABLED); > > return 0; > } > -- > 2.39.2.101.g768bb238c484.dirty > >
On Wed, 24 May 2023 17:44:32 +0100, Robin Murphy wrote: > It turns out that my naive DTC reset logic fails to work as intended, > since, after checking with the hardware designers, the PMU actually > needs to be fully enabled in order to correctly clear any pending > overflows. Therefore, invert the sequence to start with turning on both > enables so that we can reliably get the DTCs into a known state, then > moving to our normal counters-stopped state from there. Since all the > DTM counters have already been unpaired during the initial discovery > pass, we just need to additionally reset the cycle counters to ensure > that no other unexpected overflows occur during this period. > > [...] Applied to will (for-next/perf), thanks! [1/1] perf/arm-cmn: Fix DTC reset https://git.kernel.org/will/c/71746c995cac Cheers,
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c index b57a0472ea0d..cd2648980540 100644 --- a/drivers/perf/arm-cmn.c +++ b/drivers/perf/arm-cmn.c @@ -1899,9 +1899,10 @@ static int arm_cmn_init_dtc(struct arm_cmn *cmn, struct arm_cmn_node *dn, int id if (dtc->irq < 0) return dtc->irq; - writel_relaxed(0, dtc->base + CMN_DT_PMCR); + writel_relaxed(CMN_DT_DTC_CTL_DT_EN, dtc->base + CMN_DT_DTC_CTL); + writel_relaxed(CMN_DT_PMCR_PMU_EN | CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR); + writeq_relaxed(0, dtc->base + CMN_DT_PMCCNTR); writel_relaxed(0x1ff, dtc->base + CMN_DT_PMOVSR_CLR); - writel_relaxed(CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR); return 0; } @@ -1961,7 +1962,7 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn) dn->type = CMN_TYPE_CCLA; } - writel_relaxed(CMN_DT_DTC_CTL_DT_EN, cmn->dtc[0].base + CMN_DT_DTC_CTL); + arm_cmn_set_state(cmn, CMN_STATE_DISABLED); return 0; }
It turns out that my naive DTC reset logic fails to work as intended, since, after checking with the hardware designers, the PMU actually needs to be fully enabled in order to correctly clear any pending overflows. Therefore, invert the sequence to start with turning on both enables so that we can reliably get the DTCs into a known state, then moving to our normal counters-stopped state from there. Since all the DTM counters have already been unpaired during the initial discovery pass, we just need to additionally reset the cycle counters to ensure that no other unexpected overflows occur during this period. Fixes: 0ba64770a2f2 ("perf: Add Arm CMN-600 PMU driver") Reported-by: Geoff Blake <blakgeof@amazon.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- v2: Changed just about everything drivers/perf/arm-cmn.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)