On 03.04.2025 09:40, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Tuesday, March 25, 2025 5:58 PM
>>
>>> +#define amd_get_freq(name)
>>> \
>>
>> The macro parameter is used just ...
>>
>>> + static int amd_get_##name##_freq(const struct amd_cppc_drv_data
>>> + *data, \
>>
>> ... here, ...
>>
>>> + unsigned int *freq)
>>> \
>>> + {
>>> \
>>> + const struct xen_processor_cppc *cppc_data = data->cppc_data;
>>> \
>>> + uint64_t mul, div, res;
>>> \
>>> +
>>> \
>>> + if ( cppc_data->name##_mhz )
>>> \
>>> + {
>>> \
>>> + /* Switch to khz */
>>> \
>>> + *freq = cppc_data->name##_mhz * 1000;
>>> \
>>
>> ... twice here forthe MHz value, and ...
>>
>>> + return 0;
>>> \
>>> + }
>>> \
>>> +
>>> \
>>> + /* Read Processor Max Speed(mhz) as anchor point */
>>> \
>>> + mul = this_cpu(amd_max_freq_mhz);
>>> \
>>> + if ( !mul )
>>> \
>>> + return -EINVAL;
>>> \
>>> + div = data->caps.highest_perf;
>>> \
>>> + res = (mul * data->caps.name##_perf * 1000) / div;
>>> \
>>
>> ... here for the respective perf indicator. Why does it take ...
>>
>>> + if ( res > UINT_MAX )
>>> \
>>> + {
>>> \
>>> + printk(XENLOG_ERR
>>> \
>>> + "Frequeny exceeds maximum value UINT_MAX: %lu\n", res);
>>> \
>>> + return -EINVAL;
>>> \
>>> + }
>>> \
>>> + *freq = (unsigned int)res;
>>> \
>>> +
>>> \
>>> + return 0;
>>> \
>>> + }
>>> \
>>> +
>>> +amd_get_freq(lowest);
>>> +amd_get_freq(nominal);
>>
>> ... two almost identical functions, when one (with two extra input
>> parameters) would
>> suffice?
>>
>
> I had a draft fix here, If it doesn't what you hope for, plz let me know
> ```
> static int amd_get_lowest_and_nominal_freq(const struct amd_cppc_drv_data
> *data,
> unsigned int *lowest_freq,
> unsigned int *nominal_freq)
Why two outputs now when there was just one in the macro-ized form? I was
rather expecting new inputs to appear, to account for the prior uses of
the macro parameter. (As a result the function is now also quite a bit
more complex than it was before. In particular there was no ...
> {
> const struct xen_processor_cppc *cppc_data = data->cppc_data;
> uint64_t mul, div, res;
> uint8_t perf;
>
> if ( !lowest_freq && !nominal_freq )
> return -EINVAL;
>
> if ( lowest_freq && cppc_data->cpc.lowest_mhz )
> /* Switch to khz */
> *lowest_freq = cppc_data->cpc.lowest_mhz * 1000;
>
> if ( nominal_freq && cppc_data->cpc.nominal_mhz )
> /* Switch to khz */
> *nominal_freq = cppc_data->cpc.nominal_mhz * 1000;
>
> /* Still have unresolved frequency */
> if ( (lowest_freq && !(*lowest_freq)) ||
> (nominal_freq && !(*nominal_freq)) )
> {
> do {
> /* Calculate lowest frequency firstly if need */
> if ( lowest_freq && !(*lowest_freq) )
> perf = data->caps.lowest_perf;
> else
> perf = data->caps.nominal_perf;
>
> /* Read Processor Max Speed(MHz) as anchor point */
> mul = this_cpu(amd_max_pxfreq_mhz);
> if ( mul == INVAL_FREQ_MHZ || !mul )
> {
> printk(XENLOG_ERR
> "Failed to read valid processor max frequency as
> anchor point: %lu\n",
> mul);
> return -EINVAL;
> }
> div = data->caps.highest_perf;
> res = (mul * perf * 1000) / div;
>
> if ( res > UINT_MAX || !res )
> {
> printk(XENLOG_ERR
> "Frequeny exceeds maximum value UINT_MAX or being zero
> value: %lu\n",
> res);
> return -EINVAL;
> }
>
> if ( lowest_freq && !(*lowest_freq) )
> *lowest_freq = (unsigned int)res;
> else
> *nominal_freq = (unsigned int)res;
> } while ( nominal_freq && !(*nominal_freq) );
... loop there.)
Jan
> }
>
> return 0;
> }
> ```
