[MODERATED] Re: [PATCH 4/4] walnut 4

[mark gross <mgross@linux.intel.com>]

On Thu, Mar 07, 2019 at 02:56:31PM +0100, speck for Greg KH wrote:
> I was working on the backport to 4.9.y and ran across merge issues with
> this patch.  I understand the constraints on which you are working with
> here, but please, let me do a tiny rant about how you all are doing
> "extra work" with regards to the creation of sysfs files:
> 
> On Tue, Feb 19, 2019 at 04:58:11PM +0100, speck for Peter Zijlstra wrote:
> > @@ -4123,8 +4179,11 @@ static struct attribute *intel_pmu_caps_
> >         NULL
> >  };
> >  
> > +DEVICE_BOOL_ATTR(allow_tsx_force_abort, 0644, allow_tsx_force_abort);
> > +
> >  static struct attribute *intel_pmu_attrs[] = {
> >  	&dev_attr_freeze_on_smi.attr,
> > +	NULL, /* &dev_attr_allow_tsx_force_abort.attr.attr */
> >  	NULL,
> >  };
> >  
> > @@ -4623,6 +4682,15 @@ __init int intel_pmu_init(void)
> >  		tsx_attr = hsw_tsx_events_attrs;
> >  		intel_pmu_pebs_data_source_skl(
> >  			boot_cpu_data.x86_model == INTEL_FAM6_SKYLAKE_X);
> > +
> > +		if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
> > +			x86_pmu.flags |= PMU_FL_TFA;
> > +			x86_pmu.get_event_constraints = skl_get_event_constraints;
> > +			x86_pmu.enable_all = intel_skl_pmu_enable_all;
> > +			x86_pmu.commit_scheduling = intel_skl_commit_scheduling;
> > +			intel_pmu_attrs[1] = &dev_attr_allow_tsx_force_abort.attr.attr;
> > +		}
> > +
> >  		pr_cont("Skylake events, ");
> >  		name = "skylake";
> >  		break;
> 
> sysfs files have the ability to be "shown" or not, on their own.  There
> is a "is_visable()" callback for every sysfs group.
> 
> This allows you to set a whole bunch of groups to a device, and then,
> when the device is created, the kobject core will call back to you and
> ask "should I show this file?"  At that point in time, you can do your
> check for "boot_cpu_has(...)" and the like to see if you really should
> be showing the file or not.
> 
> This saves you all the horrid mess of the merge_attr() function, trying
> to overload NULL pointers here in this attribute list, and other such
> hacks.
> 
> I guess no one stopped to think why _only_ the perf cpu code has a
> function like merge_attr(), and how the rest of the kernel could get
> away without needing a hack like that :(
> 
> Anyway, rant over.  I'll see if I can squeze this into the 4.9.y tree as
> it was before the major revamp of the perf cpu sysfs files.
> 
> In the future, someone should just switch all of this to attribute
> groups, like the rest of the kernel uses, so no such crazyness is
> needed.

If peterz doesn't beat me to it I want to mentor one of the more Jr engineers
I'm working with to do this.  (and if I can't convince one of them to do this
with my help then I'll do it by the end of next week.)

But, its fine if someone beats me to it too.  There will be other opportunities
;)

--mark