On 25.03.2025 14:52, Andrew Cooper wrote: > On 25/03/2025 12:53 pm, Jan Beulich wrote: >> There's little point in allocation two uint32_t[] arrays separately. >> We'll need the bigger of the two anyway, and hence we can use that >> bigger one also for transiently storing the smaller number of items. >> >> While there also drop j (we can use i twice) and adjust the type of >> the remaining two variables on that line. >> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > > Wow this function is a mess. > > It is an improvement, so Acked-by: Andrew Cooper > <andrew.coop...@citrix.com>,
Thanks. > but the allocations could be removed > entirely by restructuring the logic some more. Perhaps. > Also, one extra observation. > >> >> --- a/xen/drivers/acpi/pmstat.c >> +++ b/xen/drivers/acpi/pmstat.c >> @@ -193,11 +193,10 @@ static int get_cpufreq_para(struct xen_s >> const struct processor_pminfo *pmpt; >> struct cpufreq_policy *policy; >> uint32_t gov_num = 0; >> - uint32_t *affected_cpus; >> - uint32_t *scaling_available_frequencies; >> + uint32_t *data; >> char *scaling_available_governors; >> struct list_head *pos; >> - uint32_t cpu, i, j = 0; >> + unsigned int cpu, i = 0; >> >> pmpt = processor_pminfo[op->cpuid]; >> policy = per_cpu(cpufreq_cpu_policy, op->cpuid); >> @@ -219,25 +218,22 @@ static int get_cpufreq_para(struct xen_s >> return -EAGAIN; >> } >> >> - if ( !(affected_cpus = xzalloc_array(uint32_t, op->u.get_para.cpu_num)) >> ) >> + if ( !(data = xzalloc_array(uint32_t, >> + max(op->u.get_para.cpu_num, >> + op->u.get_para.freq_num))) ) >> return -ENOMEM; >> + >> for_each_cpu(cpu, policy->cpus) >> - affected_cpus[j++] = cpu; >> + data[i++] = cpu; >> ret = copy_to_guest(op->u.get_para.affected_cpus, >> - affected_cpus, op->u.get_para.cpu_num); >> - xfree(affected_cpus); >> - if ( ret ) >> - return ret; >> + data, op->u.get_para.cpu_num); >> >> - if ( !(scaling_available_frequencies = >> - xzalloc_array(uint32_t, op->u.get_para.freq_num)) ) >> - return -ENOMEM; >> for ( i = 0; i < op->u.get_para.freq_num; i++ ) >> - scaling_available_frequencies[i] = >> - pmpt->perf.states[i].core_frequency * 1000; >> + data[i] = pmpt->perf.states[i].core_frequency * 1000; >> ret = copy_to_guest(op->u.get_para.scaling_available_frequencies, >> - scaling_available_frequencies, op->u.get_para.freq_num); >> - xfree(scaling_available_frequencies); >> + data, op->u.get_para.freq_num) ?: ret; >> + >> + xfree(data); >> if ( ret ) >> return ret; >> > > Not altered by this patch, but `ret` is bogus here. > > It's the number of bytes not copied, and needs transforming into -EFAULT > here and later. Oh, right - I noticed this when making the patch, then forgot again. I can make another patch, unless you have one in the works already. Jan
