Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 14 Jan 2020 14:39:58 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     "Sudarikov, Roman" <roman.sudarikov@linux.intel.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, peterz@infradead.org,
        mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        linux-kernel@vger.kernel.org, eranian@google.com,
        bgregg@netflix.com, ak@linux.intel.com, kan.liang@linux.intel.com,
        alexander.antonov@intel.com
Subject: Re: [PATCH v3 1/2] perf x86: Infrastructure for exposing an Uncore
 unit to PMON mapping
Message-ID: <20200114133958.GE170376@krava>
References: <20200113135444.12027-1-roman.sudarikov@linux.intel.com>
 <20200113135444.12027-2-roman.sudarikov@linux.intel.com>
 <20200113143430.GA390411@kroah.com>
 <9bfcc058-8fde-9b24-3d82-255004e7f057@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9bfcc058-8fde-9b24-3d82-255004e7f057@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 14, 2020 at 04:24:34PM +0300, Sudarikov, Roman wrote:

SNIP

> > >   {
> > >   	struct intel_uncore_pmu *pmus;
> > > @@ -950,10 +976,19 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
> > >   			attr_group->attrs[j] = &type->event_descs[j].attr.attr;
> > >   		type->events_group = &attr_group->group;
> > > -	}
> > > +	} else
> > > +		type->events_group = &empty_group;
> > Why???
> Hi Greg,
> 
> Technically, what I'm trying to do is to add an attribute which depends on
> the uncore pmu type and BIOS support. New attribute is added to the end of
> the attribute groups array. It appears that the events attribute group is
> optional for most of the uncore pmus for x86/intel, i.e. events_group =
> NULL.
> 
> NULL element in the middle of the attribute groups array "hides" all others
> attribute groups which follows that element.
> 
> To work around it, embedded NULL elements should be either removed from
> the attribute groups array [1] or replaced with empty attribute; see
> implementation above.
> 
> If both approaches are incorrect then please advice what would be correct
> solution for that case.

hi,
I think Greg is reffering to the recent cleanup where we used attribute
groups with is_vissible callbacks, you can check changes below:

b7c9b3927337 perf/x86/intel: Use ->is_visible callback for default group
6a9f4efe78af perf/x86: Use update attribute groups for default attributes
b657688069a2 perf/x86/intel: Use update attributes for skylake format
3ea40ac77261 perf/x86: Use update attribute groups for extra format
1f157286829c perf/x86: Use update attribute groups for caps
baa0c83363c7 perf/x86: Use the new pmu::update_attrs attribute group

jirka

> 
> [1] https://lore.kernel.org/lkml/20191210091451.6054-3-roman.sudarikov@linux.intel.com/
> 
> Thanks,
> Roman
> > Didn't we fix up the x86 attributes to work properly and not mess around
> > with trying to merge groups and the like?  Please don't perpetuate that
> > more...
> > 
> > thanks,
> > 
> > greg k-h
> 
>