Re: [PATCH v12 15/23] x86: refactor psr: CDP: implement set value callback function.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-07-03 01:01:09, Jan Beulich wrote:
> >>> On 03.07.17 at 08:33, <yi.y.sun@linux.intel.com> wrote:
> > On 17-06-30 06:02:32, Jan Beulich wrote:
> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>>
> >> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE.
> >> >In the first iteration, the props->type[0] is DATA so that it does not match
> >> >'type' and the second check is false too. If we use else branch here, it will
> >> >enter the branch and return -EVINVAL. But this is not we want, right? We hope
> >> >the second iteration should be executed to set CODE.
> >>  
> >> I see. That'll then call for yet another solution; I don't think the code should
> >> stay as is.
> >> 
> > Then, how about ASSERT() at the beginning to check if input 'type' is 
> > correct?
> >     enum cbm_type {
> >         PSR_CBM_TYPE_L3,
> >         PSR_CBM_TYPE_L3_DATA,
> >         PSR_CBM_TYPE_L3_CODE,
> >         PSR_CBM_TYPE_L2,
> >     };
> > 
> >     ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 1]) ||
> >            type == props->alt_type);
> 
> Baking in ordering assumptions? No, please don't.
> 
> > We don't need 'ret' anymore with above check.
> 
> So in a release build you'd then do what in case of a bad type finding
> its way in?
> 
> Jan

To decide the return value, we have to know if input 'type' is correct or not.
There are two ways:
1. Check if input 'type' without iteration, like the above codes. Becaue you
   don't agree the ordering assumptions, this way is not good.
2. Use iteration, like the original codes. Record if the statement is hit.
   If yes, return 0. Otherwise, return -EINVAL. The original codes are below:
    for ( i = 0; i < props->cos_num; i++ )
    {
        if ( type == props->type[i] || type == props->alt_type )
        {
            val[i] = new_val;
            ret = 0;
        }
    }

I think the main issue you don't like in the original codes is that
the 'ret = 0' may happen for several times. How about below change?
    for ( i = 0; i < props->cos_num; i++ )
    {
        if ( type == props->type[i] || type == props->alt_type )
        {
            val[i] = new_val;
            if ( ret )
                ret = 0;
        }
    }

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