Re: [PATCH v4 06/24] x86: refactor psr: implement get hw info flow.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-01-10 06:46:03, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -115,6 +115,9 @@ struct feat_ops {
> >                           struct psr_socket_info *info);
> >      /* get_max_cos_max is used to get feature's cos_max. */
> >      unsigned int (*get_max_cos_max)(const struct feat_node *feat);
> > +    /* get_feat_info is used to get feature HW info. */
> > +    bool (*get_feat_info)(const struct feat_node *feat, enum cbm_type type,
> > +                          uint32_t dat[], uint32_t array_len);
> 
> data, value, or val would all seem okay, but dat suggests an acronym
> of other than data (which I think it is meant to be).
> 
Ok, will change it to data.

> > @@ -220,9 +223,24 @@ static unsigned int l3_cat_get_max_cos_max(const struct 
> > feat_node *feat)
> >      return feat->info.l3_cat_info.cos_max;
> >  }
> >  
> > +static bool l3_cat_get_feat_info(const struct feat_node *feat,
> > +                                 enum cbm_type type,
> > +                                 uint32_t dat[], uint32_t array_len)
> 
> array_len wants to be size_t or unsigned int. And more generally,
> please avoid fixed width types when you don't really mean such.
> 
Got it, thank you!

> > +int psr_get_info(unsigned int socket, enum cbm_type type,
> > +                 uint32_t dat[], uint32_t array_len)
> > +{
> > +    struct psr_socket_info *info = get_socket_info(socket);
> > +    struct feat_node *feat_tmp;
> 
> With the hook function taking a pointer to const I don#t see why
> this one can't be const, too.
> 
Do you mean feat_tmp? Thanks!

> > +    if ( IS_ERR(info) )
> > +        return PTR_ERR(info);
> > +
> > +    list_for_each_entry(feat_tmp, &info->feat_list, list)
> > +        if ( feat_tmp->ops.get_feat_info(feat_tmp, type, dat, array_len) )
> 
> Wouldn't the type check better be done here than inside each
> function? That would then also allow for terminating the loop
> earlier (when the type was found, instead of when a function
> returns success).
> 
Ok, thanks for the suggestion!

> > --- a/xen/include/asm-x86/psr.h
> > +++ b/xen/include/asm-x86/psr.h
> > @@ -33,6 +33,11 @@
> >  /* L3 CDP Enable bit*/
> >  #define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
> >  
> > +/* Used by psr_get_info() */
> > +#define CBM_LEN  0
> > +#define COS_MAX  1
> > +#define CDP_FLAG 2
> 
> These needing putting in a header means that you want to prefix
> them, e.g. by PSR_. Also with the next value used e.g. as array
> dimension, I think you also want to name that value (currently 3)
> and use it instead of a plain number which would need to be
> adjusted everywhere once a value gets added here.
> 
> Also - is CDP_FLAG really a suitable name for flags? Wouldn't
> PSR_FLAGS be better (as being more general)?
> 
Thanks for the suggestions!

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel