Received-SPF: pass (google.com: domain of linux-kernel+bounces-81775-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 26 Feb 2024 15:14:42 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Yicong Yang <yangyicong@huawei.com>
CC: <will@kernel.org>, <mark.rutland@arm.com>, <hejunhao3@huawei.com>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<yangyicong@hisilicon.com>, <linuxarm@huawei.com>,
	<prime.zeng@hisilicon.com>, <fanghao11@huawei.com>
Subject: Re: [PATCH v2 5/8] drivers/perf: hisi_pcie: Check the target filter
 properly
Message-ID: <20240226151442.000053ad@Huawei.com>
In-Reply-To: <20240223103359.18669-6-yangyicong@huawei.com>
References: <20240223103359.18669-1-yangyicong@huawei.com>
	<20240223103359.18669-6-yangyicong@huawei.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Fri, 23 Feb 2024 18:33:56 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> The PMU can monitor traffic of certain target Root Port or downstream
> target Endpoint. User can specify the target filter by the "port" or
> "bdf" option respectively. The PMU can only monitor the Root Port or
> Endpoint on the same PCIe core so the value of "port" or "bdf" should
> be valid and will be checked by the driver.
> 
> Currently at least and only one of "port" and "bdf" option must be set.
> If "port" filter is not set or is set explicitly to zero (default),
> driver will regard the user specifies a "bdf" option since "port" option
> is a bitmask of the target Root Ports and zero is not a valid
> value.
> 
> If user not explicitly set "port" or "bdf" filter, the driver uses "bdf"
> default value (zero) to set target filter, but driver will skip the
> check of bdf=0, although it's a valid value (meaning 0000:000:00.0).
> Then the user just gets zero.
> 
> Therefore, we need to check if both "port" and "bdf" are invalid, then
> return failure and report warning.
> 
> Testing:
> before the patch:
>                    0      hisi_pcie0_core1/rx_mrd_flux/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0/
>               24,124      hisi_pcie0_core1/rx_mrd_flux,port=1/
>                    0      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>               24,132      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
>               24,138      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
>               24,126      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/
> 
> after the patch:
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0/
>               24,153      hisi_pcie0_core1/rx_mrd_flux,port=1/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>               24,117      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
>               24,120      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
>               24,123      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Thanks for explanation on v1. I'm fine with this now.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index 6f39cb82661e..b2dde7559639 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -306,10 +306,10 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
>  	if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
>  		return false;
>  
> -	if (requester_id) {
> -		if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
> -			return false;
> -	}
> +	/* Need to explicitly set filter of "port" or "bdf" */
> +	if (!hisi_pcie_get_port(event) &&
> +	    !hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
> +		return false;
>  
>  	return true;
>  }