Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2025 10:48, Penny Zheng wrote:
> @@ -201,14 +221,14 @@ int cpufreq_add_cpu(unsigned int cpu)
>      struct cpufreq_dom *cpufreq_dom = NULL;
>      struct cpufreq_policy new_policy;
>      struct cpufreq_policy *policy;
> -    struct processor_performance *perf;
> +    const struct xen_psd_package *domain_info;
> +    const struct xen_psd_package **domain_info_ptr = &domain_info;

Why's this latter variable needed? Can't you simply ...

> +    uint32_t shared_type;
>  
>      /* to protect the case when Px was not controlled by xen */
>      if ( !processor_pminfo[cpu] || !cpu_online(cpu) )
>          return -EINVAL;
>  
> -    perf = &processor_pminfo[cpu]->perf;
> -
>      if ( !(processor_pminfo[cpu]->init & XEN_PX_INIT) )
>          return -EINVAL;
>  
> @@ -218,10 +238,15 @@ int cpufreq_add_cpu(unsigned int cpu)
>      if (per_cpu(cpufreq_cpu_policy, cpu))
>          return 0;
>  
> -    if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
> +    ret = get_psd_info(cpu, &shared_type, domain_info_ptr);

... pass &domain_info here, ...

> +    if ( ret )
> +        return ret;
> +    domain_info = *domain_info_ptr;

... also eliminating the need for this assignment? (Same again further down.)

> @@ -464,6 +505,17 @@ static void print_PPC(unsigned int platform_limit)
>      printk("\t_PPC: %d\n", platform_limit);
>  }
>  
> +static int check_psd_pminfo(uint32_t shared_type)
> +{
> +    /* check domain coordination */

Nit: Comment style (wants to start with a capital letter). Yes, there are many
bad examples in this file (some even visible in patch context here), but in new
code style guidelines should be followed.

> +    if ( shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> +         shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> +         shared_type != CPUFREQ_SHARED_TYPE_HW )
> +        return -EINVAL;
> +
> +    return 0;
> +}

Looks as if the function would rather want to return a boolean value.

And anyway - I can't really spot the need for this helper, as I also can't
spot ...

> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *perf)
>  
>      if ( perf->flags & XEN_PX_PSD )
>      {
> -        /* check domain coordination */
> -        if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> -             perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> -             perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
> -        {
> -            ret = -EINVAL;
> +        ret = check_psd_pminfo(perf->shared_type);
> +        if ( ret )
>              goto out;
> -        }
>  
>          pxpt->shared_type = perf->shared_type;
>          memcpy(&pxpt->domain_info, &perf->domain_info,

... the need for this change. And even if there is a need, a follow-on
question would be how this relates to the subject of this patch.

Jan