Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver

["Wang, Wei W" <wei.w.wang@xxxxxxxxx>]

On 08/07/2015 14:24,  Jan Beulich wrote:
> >>> On 08.07.15 at 07:15, <wei.w.wang@xxxxxxxxx> wrote:
> > On 07/07/2015 20:14, Wei Liu wrote:
> >> On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
> >> > >>> On 07.07.15 at 10:55, <wei.liu2@xxxxxxxxxx> wrote:
> >> > > On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
> >> > >> --- a/tools/libxc/include/xenctrl.h
> >> > >> +++ b/tools/libxc/include/xenctrl.h
> >> > >> @@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
> >> > >>      uint32_t scaling_cur_freq;
> >> > >>
> >> > >>      char scaling_governor[CPUFREQ_NAME_LEN];
> >> > >> -    uint32_t scaling_max_freq;
> >> > >> -    uint32_t scaling_min_freq;
> >> > >> +
> >> > >> +    union {
> >> > >> +        uint32_t freq;
> >> > >> +        uint32_t pct;
> >> > >> +    } scaling_max;
> >> > >> +
> >> > >> +    union {
> >> > >> +        uint32_t freq;
> >> > >> +        uint32_t  pct;
> >> > >> +    } scaling_min;
> >> > >> +
> >> > >
> >> > > Don't you need struct? I don't see how union would work for you,
> >> > > you clearly need bot freq and pct at the same time.
> >> >
> >> > Why? The current driver uses freq; intel_pstate uses pct. What
> >> > looks wrong is the code below using both fields at once.
> >> >
> >>
> >> I only looked at this single patch.
> >>
> >> I got that impression from here:
> >>
> >> +        user_para->scaling_max.freq             = 
> >> sys_para->scaling_max.freq;
> >> +        user_para->scaling_min.freq             = 
> >> sys_para->scaling_min.freq;
> >> +        user_para->scaling_max.pct              = 
> >> sys_para->scaling_max.pct;
> >> +        user_para->scaling_min.pct              = 
> >> sys_para->scaling_min.pct;
> >>
> >> So using union is OK, just the code is confusing.
> >
> > This actually functions well, we just have a redundant copy here. For
> > example:
> > user_para->scaling_max.freq             = sys_para->scaling_max.freq;
> > user_para->scaling_max.pct               = sys_para->scaling_max.pct;
> >
> > The two sentences are doing the same thing (the memory location is the
> > same for freq and pct, it just depends on the driver to put what kind
> > of value (freq or pct) into the memory).
> >
> > If this causes some confusion, how about removing the
> > "user_para->scaling_max.pct               = sys_para->scaling_max.pct;"  ?
> 
> Better make it
> 
>         user_para->scaling_max = sys_para->scaling_max;
> 
> if at all possible. If not possible, something BUILD_BUG_ON()-like should imo
> be added to make sure the fields are of equal size.

Ok. I will wait for your comments on the other remaining patches to send out 
the next revised version. Thanks.

Best,
Wei

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