Re: [PATCH v3 09/24] arm64: Keep track of CPU feature registers

[Suzuki K. Poulose]

On 15/10/15 11:36, Catalin Marinas wrote:
> Hi Suzuki,
>
> Some minor comments below.

> > The feature bits are classified as one of SCALAR_MIN, SCALAR_MAX and DISCRETE
> > depending on the implication of the possible values. This information
> > is used to decide the safe value for a feature.
>
> The "SCALAR..." etc. in the comment needs updating.

Sorry about that. Will fix it.

> > +#define FTR_STRICT	true
> > +#define FTR_NONSTRICT	false
>
> Please add a comment on what STRICT/NONSTRICT mean.

Sure

> > +static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp)
> > +{
> > +	return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
> > +}
> > +
> > +static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
> > +{
> > +	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
> > +}
>
> Slightly inconsistent naming: since you are prefixing everything with
> arm64_, do the same for ftr_mask.

OK, will add it.

> > +static struct arm64_ftr_reg arm64_ftr_regs[] = {
> > +
> > +	/* This array should be always kept in the ascending order of id  */
>
> Do we have a sanity check somewhere? You could also call sort() on this
> array and we wouldn't have to worry.

No we don't have a sanity check. I will take a look.

> > +/*
> > + * get_arm64_sys_reg - Lookup a feature register entry using its
> > + * sys_reg() encoding. With the array arm64_ftr_regs sorted in the
> > + * ascending order, we use binary search to find a matching entry.
> > + *
> > + * returns - Upon success,  matching ftr_reg entry for id.
> > + *         - NULL on failure. It is upto the caller to decide
> > + *	     the impact of a failure.
> > + */
> > +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
>
> Nitpick: the * near the function name.
>
> Also rename it to get_arm64_ftr_reg() to match the actual return type.

Sure, will fix all of these. Thanks for the detailed look



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/