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 02:10:27, Jan Beulich wrote:
> >>> On 31.05.17 at 10:05, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > 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.
> 
> But that's not what I did suggest, and doing so will make the
> handling at the checking site more clumsy.
> 
The 'psr_get_feat_and_type' make things complex here. I'd like to discard
it and directly implement the functionality of it in 'psr_get_info' and
'psr_get_val' to make codes clear.

> Jan

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