Re: [PATCH v3 3/4] lib/power: add get and set API for scaling freq min and max with pstate mode

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 23-May-22 9:21 PM, Kevin Laatz wrote:
> Add new get/set API to allow the user or application to set the minimum
> and maximum frequencies to use when scaling.
> Previously, the frequency range was determined by the HW capabilities of
> the CPU. With this new API, the user or application can constrain this
> if required.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> 
> ---

<snip>

> +int
> +rte_power_pmd_mgmt_get_scaling_freq_max(unsigned int lcore)
> +{
> +	if (lcore >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, POWER, "Invalid lcore ID: %u\n", lcore);
> +		return -EINVAL;
> +	}
> +
> +	if (scale_freq_max[lcore] == UINT32_MAX)
> +		RTE_LOG(DEBUG, POWER, "Scaling freq max config not set. Using sysfs max freq.\n");
> +
> +	return scale_freq_max[lcore];

If you're using 0 as "not set" you should return 0 when it's not set :)

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
> + *
> + * Set the min frequency to be used for frequency scaling.

...or zero to use defaults?

> + *
> + * @note Supported by: Pstate mode.
> + *
> + * @param lcore
> + *   The ID of the lcore to set the min frequency for.
> + * @param min
> + *   The value, in KiloHertz, to set the minimum frequency to.
> + * @return
> + *   0 on success
> + *   <0 on error
> + */
> +__rte_experimental
> +int
> +rte_power_pmd_mgmt_set_scaling_freq_min(unsigned int lcore, unsigned int min);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
> + *
> + * Set the max frequency to be used for frequency scaling.

...or zero to use defaults?

> + *
> + * @note Supported by: Pstate mode.
> + *
> + * @param lcore
> + *   The ID of the lcore to set the max frequency for.
> + * @param max
> + *   The value, in KiloHertz, to set the maximum frequency to.
> + *   If 'max' is 0, it is considered 'not set'.
> + * @return
> + *   0 on success
> + *   <0 on error
> + */
> +__rte_experimental
> +int
> +rte_power_pmd_mgmt_set_scaling_freq_max(unsigned int lcore, unsigned int max);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
> + *
> + * Get the current configured min frequency used for frequency scaling.
> + *
> + * @note Supported by: Pstate mode.
> + *
> + * @param lcore
> + *   The ID of the lcore to get the min frequency for.
> + * @return
> + *   0 if no value has been configured via the 'set' API.
> + *   >0 if a minimum frequency has been configured. Value is the minimum frequency
> + *   , in KiloHertz, used for frequency scaling.
> + *   <0 on error
> + */
> +__rte_experimental
> +int
> +rte_power_pmd_mgmt_get_scaling_freq_min(unsigned int lcore);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
> + *
> + * Get the current configured max frequency used for frequency scaling.
> + *
> + * @note Supported by: Pstate mode.
> + *
> + * @param lcore
> + *   The ID of the lcore to get the max frequency for.
> + * @return
> + *   UINT32_MAX if no value has been configured via the 'set' API.

The comment now mismatches the code.

Once the above is fixed,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly