Re: [Xen-devel] [PATCH RESEND v5 07/24] x86: refactor psr: implement get value flow.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Feb 07, 2017 at 10:47:01AM +0800, Yi Sun wrote:
> Hi,
> 
> Thanks for reviewing! I agree with your comments except below one. Could you
> please check my response?
> 
> On 17-01-31 15:29:34, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 19, 2017 at 02:01:09PM +0800, Yi Sun wrote:
> > > +int psr_get_val(struct domain *d, unsigned int socket,
> > > +                uint64_t *val, enum cbm_type type)
> > >  {
> > > -    return 0;
> > > +    const struct psr_socket_info *info = get_socket_info(socket);
> > > +    unsigned int cos = d->arch.psr_cos_ids[socket];
> > > +    const struct feat_node *feat;
> > > +    enum psr_feat_type feat_type;
> > > +
> > > +    if ( IS_ERR(info) )
> > > +        return PTR_ERR(info);
> > > +
> > > +    feat_type = psr_cbm_type_to_feat_type(type);
> > > +    list_for_each_entry(feat, &info->feat_list, list)
> > > +    {
> > > +        if ( feat->feature != feat_type )
> > > +            continue;
> > > +
> > > +        if ( feat->ops.get_val(feat, cos, type, val) )
> > > +            /* Found */
> > 
> > No need. The 'psr_get_info' does not have this.
> > 
> > > +            return 0;
> > > +    }
> > > +
> > > +    return -ENOENT;
> > 
> > This function looks quite similar to 'psr_get_info'.
> > 
> > Perhaps it may make sense to have an common one that has an
> > extra argument (whether to call get_val or get_feat_info)?
> > 
> > And then psr_get_val and psr_get_info can call in this common
> > code with this extra argument attached?
> >
> Yes, the both functions are almost same. But I feel not good to combine them 
> to
> one function. I think it makes the interface not explicit. As there are only 3
> interfaces exposed by psr.c, I want to keep current implementation. Is that
> acceptable to you?

Keep the interface as is.

You would have _three_ functions:

psr_get_val
psr_get_info

and the one both of them would call which would be called:

__psr_get

which would do the heavy lifting.
> 
> Thanks,
> Sun Yi

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