Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 8 Mar 2021 14:14:40 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     "Jin, Yao" <yao.jin@linux.intel.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v3] perf pmu: Validate raw event with sysfs exported
 format bits
Message-ID: <YEYjQOYI7utqnCq6@krava>
References: <20210308031506.23019-1-yao.jin@linux.intel.com>
 <YEX91MTGMU41zeuF@krava>
 <c06cdd0d-fee2-ab6d-1d22-49a6590996ea@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c06cdd0d-fee2-ab6d-1d22-49a6590996ea@linux.intel.com>
Precedence: bulk

On Mon, Mar 08, 2021 at 08:57:49PM +0800, Jin, Yao wrote:
> Hi Jiri,
> 
> On 3/8/2021 6:35 PM, Jiri Olsa wrote:
> > On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 44ef28302fc7..03ab1e6d0418 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -1812,3 +1812,39 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> > >   	return nr_caps;
> > >   }
> > > +
> > > +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> > > +				   char *name)
> > > +{
> > > +	struct perf_pmu_format *format;
> > > +	__u64 masks = 0, bits;
> > > +	char buf[100];
> > > +	unsigned int i;
> > > +
> > > +	list_for_each_entry(format, &pmu->format, list)	{
> > > +		/*
> > > +		 * Skip extra configs such as config1/config2.
> > > +		 */
> > > +		if (format->value > 0)
> > > +			continue;
> > 
> > sorry I did not notice before, but could you please use more direct
> > approach like:
> > 
> > 		if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
> > 			break;
> > 		}
> > 
> > this will be more obvious, also no need for the comment.. I spent some
> > time looking what's the value for ;-)
> > 
> > thanks,
> > jirka
> > 
> 
> Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!
> 
> While it can't break the loop, because we need to iterate over the whole
> list to get the total valid bits. So like:
> 
> if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
> 	continue;
> 
> Is it right?

sure, what I meant was to process only PERF_PMU_FORMAT_VALUE_CONFIG
and then call break, because there's no need to iterate further

jirka