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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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).

> @@ -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.

> +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.

> +    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).

> --- 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)?

Jan


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