Re: [PATCH v9 1/3] power: add Intel uncore frequency control API to power library

[Thomas Monjalon <thomas@monjalon.net>]

06/10/2022 11:38, Tadhg Kearney:
> +API Overview for Intel Uncore
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Overview of each function in the Intel Uncore API, with explanation of what
> +they do. Each function should not be called in the fast path.
> +
> +* **Uncore Power Init**
> +    Initialize uncore power, populate frequency array and record
> +    original min & max for die on pkg.
> +
> +* **Uncore Power Exit**
> +    Exit uncore power, restoring original min & max for die on pkg.
> +
> +* **Get Uncore Power Freq**
> +    Get current uncore freq index for die on pkg.
> +
> +* **Set Uncore Power Freq**
> +    Set min & max uncore freq index for die on pkg to specified index value
> +    (min and max will be the same).
> +
> +* **Uncore Power Max**
> +    Set min & max uncore freq to maximum frequency index for die on pkg
> +    (min and max will be the same).
> +
> +* **Uncore Power Min**
> +    Set min & max uncore freq to minimum frequency index for die on pkg
> +    (min and max will be the same).
> +
> +* **Get Num Freqs**
> +    Get the number of frequencies in the index array.
> +
> +* **Get Num Pkgs**
> +    Get the number of packages (CPU's) on the system.
> +
> +* **Get Num Dies**
> +    Get the number of die's on a given package.

This is the role of doxygen documentation to explain API.
I don't understand why it is there.
Anyway I've converted it into a definition list,
which the proper RST syntax for what you do.

>          'rte_power.c',
>          'rte_power_empty_poll.c',
>          'rte_power_pmd_mgmt.c',
> +        'rte_power_intel_uncore.c',

In general, we keep such list in alphabetical order.

[...]
> +struct uncore_power_info {
> +	unsigned int die;                    /** Core die id */
> +	unsigned int pkg;                    /** Package id */
> +	uint32_t freqs[MAX_UNCORE_FREQS];/** Frequency array */
> +	uint32_t nb_freqs;                   /** Number of available freqs */
> +	FILE *f_cur_min;                     /** FD of scaling_min */
> +	FILE *f_cur_max;                     /** FD of scaling_max */
> +	uint32_t curr_idx;                   /** Freq index in freqs array */
> +	uint32_t org_min_freq;               /** Original min freq of uncore */
> +	uint32_t org_max_freq;               /** Original max freq of uncore */
> +	uint32_t init_max_freq;              /** System max uncore freq */
> +	uint32_t init_min_freq;              /** System min uncore freq */
> +} __rte_cache_aligned;

No need of doxygen syntax in a .c file.
And an alignment is wrong.

[...]
> +		RTE_LOG(DEBUG, POWER, "Invalid uncore frequency index %u, which "
> +				"should be less than %u\n", idx, ui->nb_freqs);

When you have time, it would be good to switch to dynamic logging.
See RTE_LOG_REGISTER_DEFAULT


[...]
> +#ifndef _RTE_POWER_INTEL_UNCORE_H
> +#define _RTE_POWER_INTEL_UNCORE_H

underscore prefix is reserved for reserved keywords

[...]
> +/**
> + * Exit uncore frequency management on a specific die on a package. It will restore uncore min and

Screen width is limited, while scrolling bar is infinite,
so don't hesitate to break your lines shorter when possible,
like at the end of a sentence.