Re: [arm-perf:for-next/perf 6/8] drivers/perf/arm-ni.c:433:13: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first

[Robin Murphy <robin.murphy@arm.com>]

On 2025-11-04 1:32 pm, Will Deacon wrote:
> On Tue, Nov 04, 2025 at 01:26:11PM +0000, Robin Murphy wrote:
>> On 2025-11-04 12:35 pm, Will Deacon wrote:
>>> On Tue, Nov 04, 2025 at 05:43:40PM +0800, kernel test robot wrote:
>>>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-next/perf
>>>> head:   989b40b7578a2be7b0388522d33d751b257d59d9
>>>> commit: 8fa08f8835e57e1fd5e2994c9cf76c84dadc1235 [6/8] perf/arm-ni: Add NoC S3 support
>>>> config: sparc64-randconfig-002-20251104 (https://download.01.org/0day-ci/archive/20251104/202511041749.ok8zDP6u-lkp@intel.com/config)
>>>> compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project d2625a438020ad35330cda29c3def102c1687b1b)
>>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251104/202511041749.ok8zDP6u-lkp@intel.com/reproduce)
>>>>
>>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>>> the same patch/commit), kindly add following tags
>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202511041749.ok8zDP6u-lkp@intel.com/
>>>>
>>>> All warnings (new ones prefixed by >>):
>>>>
>>>>>> drivers/perf/arm-ni.c:433:13: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Wparentheses]
>>>>        432 |                 hw->event_base = (unsigned long)cd->pmu_base +
>>>>            |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>        433 |                                  is_7xx ? NI700_PMCCNTR_L : NI_PMCCNTR_L;
>>>>            |                                  ~~~~~~ ^
>>>>      drivers/perf/arm-ni.c:433:13: note: place parentheses around the '+' expression to silence this warning
>>>>        432 |                 hw->event_base = (unsigned long)cd->pmu_base +
>>>>            |                                  (
>>>>        433 |                                  is_7xx ? NI700_PMCCNTR_L : NI_PMCCNTR_L;
>>>>            |                                         ^
>>>>            |                                        )
>>>
>>> Hrm. Doesn't that mean this is completely broken or am I misreading the
>>> report?
>>
>> Bah, my arm64 W=1 build (which I did do in the hope of catching any such
>> sillies...) fails to flag that :(
>>
>> Truth is this is a fresh refactoring of the mess I originally wrote that got
>> tested by someone else 18 months ago, and I don't have any hardware to
>> functionally test it myself. Feel free to drop it and I'll send a complete
>> v2, or I can do an incremental fix if you prefer (I already realised I could
>> have streamlined the version check to a one-off in event_init as well.)
> 
> Damn, it's not ideal that we don't have any test coverage for this.
> 
> The patch is buried under some others now, so I can either revert it or
> take fixes on top. Up to you.

I've managed to break my git-send-email setup at the moment, so I hope
an inline patch is OK!

Cheers,
Robin.

----->8-----
Subject: [PATCH] perf/arm-ni: Fix and optimise register offset calculation

LKP points out an operator precedence oversight in the new NoC S3
support that, annoyingly, my local W=1 build didn't flag. In fixing
that, we can also take the similarly-missed opportunity to cache the
version check itself at event_init time.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202511041749.ok8zDP6u-lkp@intel.com/
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  drivers/perf/arm-ni.c | 21 ++++++++++++---------
  1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c
index aa824abc629e..66858c65215d 100644
--- a/drivers/perf/arm-ni.c
+++ b/drivers/perf/arm-ni.c
@@ -323,9 +323,15 @@ static int arm_ni_validate_group(struct perf_event *event)
  	return 0;
  }
  
+static bool arm_ni_is_7xx(const struct arm_ni *ni)
+{
+	return ni->part == PART_NI_700 || ni->part == PART_NI_710AE;
+}
+
  static int arm_ni_event_init(struct perf_event *event)
  {
  	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+	struct arm_ni *ni;
  
  	if (event->attr.type != event->pmu->type)
  		return -ENOENT;
@@ -333,7 +339,10 @@ static int arm_ni_event_init(struct perf_event *event)
  	if (is_sampling_event(event))
  		return -EINVAL;
  
-	event->cpu = cd_to_ni(cd)->cpu;
+	ni = cd_to_ni(cd);
+	event->cpu = ni->cpu;
+	event->hw.flags = arm_ni_is_7xx(ni);
+
  	if (NI_EVENT_TYPE(event) == NI_PMU)
  		return arm_ni_validate_group(event);
  
@@ -411,18 +420,12 @@ static void arm_ni_init_evcnt(struct hw_perf_event *hw)
  	writel_relaxed(S32_MIN, (void __iomem *)hw->event_base);
  }
  
-static bool arm_ni_is_7xx(const struct arm_ni *ni)
-{
-	return ni->part == PART_NI_700 || ni->part == PART_NI_710AE;
-}
-
  static int arm_ni_event_add(struct perf_event *event, int flags)
  {
  	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
  	struct hw_perf_event *hw = &event->hw;
  	struct arm_ni_unit *unit;
  	enum ni_node_type type = NI_EVENT_TYPE(event);
-	bool is_7xx = arm_ni_is_7xx(cd_to_ni(cd));
  	u32 reg;
  
  	if (type == NI_PMU) {
@@ -430,7 +433,7 @@ static int arm_ni_event_add(struct perf_event *event, int flags)
  			return -ENOSPC;
  		hw->idx = NI_CCNT_IDX;
  		hw->event_base = (unsigned long)cd->pmu_base +
-				 is_7xx ? NI700_PMCCNTR_L : NI_PMCCNTR_L;
+				 (hw->flags ? NI700_PMCCNTR_L : NI_PMCCNTR_L);
  		cd->ccnt = event;
  		arm_ni_init_ccnt(hw);
  	} else {
@@ -443,7 +446,7 @@ static int arm_ni_event_add(struct perf_event *event, int flags)
  		unit = (void *)hw->config_base;
  		unit->event[hw->idx] = NI_EVENT_EVENTID(event);
  		hw->event_base = (unsigned long)cd->pmu_base +
-				 is_7xx ? NI700_PMEVCNTR(hw->idx) : NI_PMEVCNTR(hw->idx);
+				 (hw->flags ? NI700_PMEVCNTR(hw->idx) : NI_PMEVCNTR(hw->idx));
  		arm_ni_init_evcnt(hw);
  		lo_hi_writeq_relaxed(le64_to_cpu(unit->pmusel), unit->pmusela);
  
-- 
2.34.1