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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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