Re: [Xen-devel] [PATCH v11 07/23] x86: refactor psr: L3 CAT: implement get value flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-05-31 01:45:45, Jan Beulich wrote:
> >>> On 31.05.17 at 09:30, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-05-30 08:05:02, Jan Beulich wrote:
> >> >>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> 
> >> > --- a/xen/arch/x86/psr.c
> >> > +++ b/xen/arch/x86/psr.c
> >> > @@ -476,23 +476,34 @@ static struct psr_socket_info 
> >> > *get_socket_info(unsigned int socket)
> >> >      return socket_info + socket;
> >> >  }
> >> >  
> >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket,
> >> > +                                               enum cbm_type type,
> >> > +                                               enum psr_feat_type 
> >> > *feat_type)
> >> > +{
> >> > +    const struct psr_socket_info *info = get_socket_info(socket);
> >> > +
> >> > +    if ( IS_ERR(info) )
> >> > +        return ERR_PTR(PTR_ERR(info));
> >> > +
> >> > +    *feat_type = psr_cbm_type_to_feat_type(type);
> >> > +    if ( *feat_type >= ARRAY_SIZE(info->features) )
> >> > +        return NULL;
> >> 
> >> Note how this return is not being taken care of by ...
> >> 
> >> > +    return info->features[*feat_type];
> >> > +}
> >> > +
> >> >  int psr_get_info(unsigned int socket, enum cbm_type type,
> >> >                   uint32_t data[], unsigned int array_len)
> >> >  {
> >> > -    const struct psr_socket_info *info = get_socket_info(socket);
> >> >      const struct feat_node *feat;
> >> >      enum psr_feat_type feat_type;
> >> >  
> >> >      ASSERT(data);
> >> >  
> >> > -    if ( IS_ERR(info) )
> >> > -        return PTR_ERR(info);
> >> > -
> >> > -    feat_type = psr_cbm_type_to_feat_type(type);
> >> > -    if ( feat_type >= ARRAY_SIZE(info->features) )
> >> > -        return -ENOENT;
> >> > +    feat = psr_get_feat_and_type(socket, type, &feat_type);
> >> > +    if ( IS_ERR(feat) )
> >> > +        return PTR_ERR(feat);
> >> 
> >> ... the check here. I think you want to alter the return above.
> >> 
> > This NULL is taken care by below code:
> >     if ( !feat || !feat_props[feat_type] ) 
> 
> Oh, I see.
> 
> > The returned errors are handled separately. For PTR_ERR, it is handled
> > above. For NULL, it is handled below.
> > 
> > I checked IS_ERR, I think it can handle the NULL case. The NULL will not
> > be treated as an error.
> 
> "it can handle the NULL case" is rather ambiguous: The NULL
> case normally (including the case here) also is an error, and
> IS_ERR() _does not_ detect this error. Hence using it one the
> result of a function that may return NULL is at least
> questionable (IS_ERR_OR_NULL() is intended to be used in such
> cases).
> 
> So while indeed the code is correct as is, I'd still like to ask you
> to make the suggested change so that the code also ends up
> being visibly correct at the first glance.
> 
Thank you! Will use 'IS_ERR_OR_NULL()' to check result.

> >> And of course I wonder why you replace code here that was
> >> only introduced one or two patches earlier. Perhaps that earlier
> >> patch should do things this way right away?
> >> 
> > Because the helper function 'psr_get_feat_and_type' is only used by
> > 'psr_get_info' if we implement it in previous patch. This seems
> > unnecessary. So, I introduce this helper function in this patch.
> > Shall I move it to previous patch?
> 
> I'd prefer if you did. When taking such decisions, please also consider
> the amount of churn you cause as well as reviewability.
> 
Got it, thanks!

> Jan

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