On Wed, 12 Oct 2016, Dave Hansen wrote: > On 10/12/2016 06:34 AM, Thomas Gleixner wrote: > >> > + if (c->x86 == 6 && > >> > + c->x86_model == INTEL_FAM6_XEON_PHI_KNL && > >> > + phir3mwait) { > >> > + u64 prev; > >> > + > >> > + rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev); > >> > + if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0) > >> > + wrmsrl(MSR_PHI_MISC_THD_FEATURE, > >> > + prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT); > > The codingstyle here is just convoluted crap. What's wrong with writing it > > proper? > > > > if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) { > > u64 msr; > > > > rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr); > > msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT; > > wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr); > > > > } > > > > No horrible to read line breaks, no redundant check for x->x86 == 6 because > > model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the > > conditional is pointless as the feature is default disabled. And even if it > > is enabled the extra msr write is not a problem at all. This is early init > > code and not some hot path. > > Hi Thomas, > > We really do need to check for family=6 (c->x86==6). > INTEL_FAM6_XEON_PHI_KNL is just for the model and doesn't check family. > It implies that you've already checked for family 6.
Indeed. It came to me after sending the mail and closing the notebook to head out for more conference fun. I expected someone to notice it :) > Looking at the name, though, it's pretty clear that the naming can > easily trip folks up. > > I do think we've probably screwed up the way we use our 'struct > x86_cpu_id' mechanism. Maybe we should be providing the > vendor/family/model sets from a common place to the drivers, instead of > making them all repeat it individually. > > Like have a big header full of: > > DECLARE_CPU(INTEL_XEON_PHI_KNL, INTEL..., 6, MODEL_XYZ...); > > Once we have that, everybody can just do: > > if(cpu_is(c, INTEL_XEON_PHI_KNL)) > ... > > and get all the checking they need. Right, and we should do the following: __u8 x86; __u8 x86_vendor; __u8 x86_model; __u8 x86_mask; u32 x86_fvm; set x86_fvm to family | vendor << 8 | model << 16; and then do the comparison on that instead of checking 3 bytes in a row. Thanks, tglx