>>> On 08.07.15 at 07:15, <wei.w.w...@intel.com> 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.l...@citrix.com> 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel