Re: Re: [PATCH 4/7] drivers/perf: hisi_pcie: Check the target filter properly

[hejunhao]

On 2024/2/8 20:29, Jonathan Cameron wrote:
> On Sun, 4 Feb 2024 15:45:24 +0800
> Yicong Yang <yangyicong@xxxxxxxxxx> wrote:
>
> > From: Junhao He <hejunhao3@xxxxxxxxxx>
> >
> > 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/
> >       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
> Nice to include an example that works for bdf
> 			    hisi_pcie0_core1/rx_mrd_flux,bdf=1,port=0
> or something like that?

Yes, I will do that.
These combined parameter test cases have been validated.

> > 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/
> >       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
> >       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
> >
> > Signed-off-by: Junhao He <hejunhao3@xxxxxxxxxx>
> > Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> Clearly the current situation is wrong, but perhaps we can
> have a more intuitive scheme (could be added as a follow up patch)
> and have the driver figure out which port the bdf lies below?
>
> Maybe that's a job for userspace tooling rather than the driver, but
> the driver already has verification code and it wouldn't be hard
> to not just check the rp is ours, but also set the filter to specify
> that rp, or maybe just set the mask to include them all?
>
> Jonathan
>
>
> > ---
> >   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 83be3390686c..b91f03c02c57 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;
> >   }