All of lore.kernel.org
 help / color / mirror / Atom feed
From: James Clark <james.clark@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-arm-kernel@lists.infradead.org, zhangshaokun@hisilicon.com
Cc: Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally
Date: Mon, 9 Oct 2023 09:59:19 +0100	[thread overview]
Message-ID: <ffb41c00-1df8-e4bb-deff-c2d1cfb15ec0@arm.com> (raw)
In-Reply-To: <20231009075631.193208-1-anshuman.khandual@arm.com>



On 09/10/2023 08:56, Anshuman Khandual wrote:
> PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate
> PMU version implementations where the register is available and reading it
> is valid . Hence checking for bus slot event presence is redundant and can
> be dropped.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This applies on v6.6-rc5.
>  
>  drivers/perf/arm_pmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 1e72b486c033..9fc1b6da5106 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -1129,7 +1129,7 @@ static void __armv8pmu_probe_pmu(void *info)
>  			     pmceid, ARMV8_PMUV3_MAX_COMMON_EVENTS);
>  
>  	/* store PMMIR register for sysfs */
> -	if (is_pmuv3p4(pmuver) && (pmceid_raw[1] & BIT(31)))
> +	if (is_pmuv3p4(pmuver))
>  		cpu_pmu->reg_pmmir = read_pmmir();
>  	else
>  		cpu_pmu->reg_pmmir = 0;


This does have the side effect of showing non-zero values in caps/slots
even when the STALL_SLOT event isn't implemented. I think that's the
scenario that the original commit (f5be3a61fd) was trying to avoid:

  /sys/bus/event_source/devices/armv8_pmuv3_0/caps/slots is exposed
  under sysfs. [If] Both ARMv8.4-PMU and STALL_SLOT event are
  implemented, it returns the slots from PMMIR_EL1, otherwise it will
  return 0.

I can't really think of a scenario where that would be an issue, and the
availability of the STALL_SLOT event is already discoverable from
userspace through the events folder, so it's probably fine.

Adding the original author just in case. But otherwise:

Reviewed-by: James Clark <james.clark@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: James Clark <james.clark@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-arm-kernel@lists.infradead.org, zhangshaokun@hisilicon.com
Cc: Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally
Date: Mon, 9 Oct 2023 09:59:19 +0100	[thread overview]
Message-ID: <ffb41c00-1df8-e4bb-deff-c2d1cfb15ec0@arm.com> (raw)
In-Reply-To: <20231009075631.193208-1-anshuman.khandual@arm.com>



On 09/10/2023 08:56, Anshuman Khandual wrote:
> PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate
> PMU version implementations where the register is available and reading it
> is valid . Hence checking for bus slot event presence is redundant and can
> be dropped.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This applies on v6.6-rc5.
>  
>  drivers/perf/arm_pmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 1e72b486c033..9fc1b6da5106 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -1129,7 +1129,7 @@ static void __armv8pmu_probe_pmu(void *info)
>  			     pmceid, ARMV8_PMUV3_MAX_COMMON_EVENTS);
>  
>  	/* store PMMIR register for sysfs */
> -	if (is_pmuv3p4(pmuver) && (pmceid_raw[1] & BIT(31)))
> +	if (is_pmuv3p4(pmuver))
>  		cpu_pmu->reg_pmmir = read_pmmir();
>  	else
>  		cpu_pmu->reg_pmmir = 0;


This does have the side effect of showing non-zero values in caps/slots
even when the STALL_SLOT event isn't implemented. I think that's the
scenario that the original commit (f5be3a61fd) was trying to avoid:

  /sys/bus/event_source/devices/armv8_pmuv3_0/caps/slots is exposed
  under sysfs. [If] Both ARMv8.4-PMU and STALL_SLOT event are
  implemented, it returns the slots from PMMIR_EL1, otherwise it will
  return 0.

I can't really think of a scenario where that would be an issue, and the
availability of the STALL_SLOT event is already discoverable from
userspace through the events folder, so it's probably fine.

Adding the original author just in case. But otherwise:

Reviewed-by: James Clark <james.clark@arm.com>

  reply	other threads:[~2023-10-09  8:59 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-09  7:56 [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally Anshuman Khandual
2023-10-09  7:56 ` Anshuman Khandual
2023-10-09  8:59 ` James Clark [this message]
2023-10-09  8:59   ` James Clark
2023-10-10  2:27   ` Anshuman Khandual
2023-10-10  2:27     ` Anshuman Khandual
2023-10-12  9:24   ` Mark Rutland
2023-10-12  9:24     ` Mark Rutland
2023-10-12 10:58     ` Anshuman Khandual
2023-10-12 10:58       ` Anshuman Khandual
2023-10-12 13:17       ` Mark Rutland
2023-10-12 13:17         ` Mark Rutland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ffb41c00-1df8-e4bb-deff-c2d1cfb15ec0@arm.com \
    --to=james.clark@arm.com \
    --cc=anshuman.khandual@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=will@kernel.org \
    --cc=zhangshaokun@hisilicon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.