> -----Original Message----- > From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx] > Sent: Thursday, July 19, 2018 9:09 PM > To: Kang, Luwei <luwei.kang@xxxxxxxxx> > Cc: kvm@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx; thomas.lendacky@xxxxxxx; bp@xxxxxxx; > konrad.wilk@xxxxxxxxxx; mattst88@xxxxxxxxx; Janakarajan.Natarajan@xxxxxxx; dwmw@xxxxxxxxxxxx; > alexander.shishkin@xxxxxxxxxxxxxxx; songliubraving@xxxxxx; kstewart@xxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; > peterz@xxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; rkrcmar@xxxxxxxxxx; david@xxxxxxxxxx; bsd@xxxxxxxxxx; marcorr@xxxxxxxxxx; > joro@xxxxxxxxxx > Subject: Re: [PATCH v11 03/12] perf/x86/intel/pt: Introduce a new function to get capability of Intel PT > > On Mon, 9 Jul 2018, Luwei Kang wrote: > > > > -u32 pt_cap_get(enum pt_capabilities cap) > > +u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap) > > Please make the arguments visually distinguishable. caps vs. cap is really not good to parse. The second one is a selector or index, so > please spell it out. Thanks for the code review. Do you mean change the "cap" to "capability" ? That is From u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap) TO u32 pt_cap_decode(u32 *caps, enum pt_capabilities capability) > > > { > > struct pt_cap_desc *cd = &pt_caps[cap]; > > - u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg]; > > + u32 c = caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg]; > > unsigned int shift = __ffs(cd->mask); > > > > return (c & cd->mask) >> shift; > > } > > +EXPORT_SYMBOL_GPL(pt_cap_decode); > > + > > +u32 pt_cap_get(enum pt_capabilities cap) { > > + return pt_cap_decode(pt_pmu.caps, cap); } > > EXPORT_SYMBOL_GPL(pt_cap_get); > > > > static ssize_t pt_cap_show(struct device *cdev, diff --git > > a/arch/x86/include/asm/intel_pt.h b/arch/x86/include/asm/intel_pt.h > > index 4270421..a741d33 100644 > > --- a/arch/x86/include/asm/intel_pt.h > > +++ b/arch/x86/include/asm/intel_pt.h > > @@ -26,9 +26,11 @@ enum pt_capabilities { #if > > defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL) void > > cpu_emergency_stop_pt(void); extern u32 pt_cap_get(enum > > pt_capabilities cap); > > +extern u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap); > > #else > > static inline void cpu_emergency_stop_pt(void) {} static inline u32 > > pt_cap_get(enum pt_capabilities cap) { return 0; } > > +static u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap) { > > +return 0; } > > This will throw warnings when included into code which does not use this stub function and rightfully so. Stubs are 'static inline' for > a reason. Get it. Will fix it. Have any comments on other x86 patches? Thanks, Luwei Kang
