On 28.03.2025 11:51, Anthony PERARD wrote: > On Thu, Mar 27, 2025 at 02:32:04PM +0100, Jan Beulich wrote: >> HWP work made the cleanup of the "available governors" array >> conditional, neglecting the fact that the condition used may not be the > > I don't know why the cleanup was made conditional, as long as the bounce > buffer is declared with DECLARE_NAMED_HYPERCALL_BOUNCE(), > xc_hypercall_bounce_post() can be called without issue (even if > ..bounce_pre isn't used). Maybe it's all those "unlock_*" labels that is > misleading, a single "out" label which does the cleanup > unconditionally would be more than enough.
Oh, yet more cleanup to do there (independently of course). >> condition that was used to allocate the buffer (as the structure field >> is updated upon getting back EAGAIN). Throughout the function, use the >> local variable being introduced to address that. >> >> --- a/tools/libs/ctrl/xc_pm.c >> +++ b/tools/libs/ctrl/xc_pm.c >> @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc >> DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, >> user_para->scaling_available_governors, >> user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), >> XC_HYPERCALL_BUFFER_BOUNCE_BOTH); >> - >> - bool has_num = user_para->cpu_num && >> - user_para->freq_num && >> - user_para->gov_num; >> + unsigned int in_gov_num = user_para->gov_num; >> + bool has_num = user_para->cpu_num && user_para->freq_num; >> >> if ( has_num ) > > I think there's an issue here, this condition used to be true if > `gov_num` was not 0, even if `cpu_num` and `freq_num` was 0. That's not > the case anymore. Shouldn't `has_num` use also the value from > `gov_num`? Do we actually need `has_num`? Raising this question is where all of this started: https://lists.xen.org/archives/html/xen-devel/2025-03/msg01870.html. IOW with Penny's change I think the need for has_num will disappear, the latest. At this point, requesting the governors being optional, only ->gov_num shouldn't affect has_num. Once requesting frequencies becomes optional too, has_num would become a mere alias of ->cpu_num. Jan
