Re: [PATCH v2 09/15] powercap/intel_rapl: Cleanup Power Limits support

[Ville Syrjälä]

On Wed, Apr 19, 2023 at 10:44:13AM +0800, Zhang Rui wrote:
> The same set of operations are shared by different Powert Limits,
> including Power Limit get/set, Power Limit enable/disable, clamping
> enable/disable, time window get/set, and max power get/set, etc.
> 
> But the same operation for different Power Limit has different
> primitives because they use different registers/register bits.
> 
> A lot of dirty/duplicate code was introduced to handle this difference.
> 
> Introduce a universal way to issue Power Limit operations.
> Instead of using hardcoded primitive name directly, use Power Limit id
> + operation type, and hide all the Power Limit difference details in a
> central place, get_pl_prim(). Two helpers, rapl_read_pl_data() and
> rapl_write_pl_data(), are introduced at the same time to simplify the
> code for issuing Power Limit operations.
> 
> Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> Tested-by: Wang Wendy <wendy.wang@xxxxxxxxx>
> ---
>  drivers/powercap/intel_rapl_common.c | 343 ++++++++++++---------------
>  include/linux/intel_rapl.h           |   1 -
>  2 files changed, 146 insertions(+), 198 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index 8e77df42257a..7f80c35e5c86 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
<snip>
> @@ -818,6 +778,33 @@ static int rapl_write_data_raw(struct rapl_domain *rd,
>  	return ret;
>  }
>  
> +static int rapl_read_pl_data(struct rapl_domain *rd, int pl,
> +			      enum pl_prims pl_prim, bool xlate, u64 *data)
> +{
> +	enum rapl_primitives prim = get_pl_prim(pl, pl_prim);
> +
> +	if (!is_pl_valid(rd, pl))
> +		return -EINVAL;
> +
> +	return rapl_read_data_raw(rd, prim, xlate, data);
> +}
> +
> +static int rapl_write_pl_data(struct rapl_domain *rd, int pl,
> +			       enum pl_prims pl_prim,
> +			       unsigned long long value)
> +{
> +	enum rapl_primitives prim = get_pl_prim(pl, pl_prim);
> +
> +	if (!is_pl_valid(rd, pl))
> +		return -EINVAL;
> +
> +	if (rd->state & DOMAIN_STATE_BIOS_LOCKED) {
> +		pr_warn("%s:%s:%s locked by BIOS\n", rd->rp->name, rd->name, pl_names[pl]);
> +		return -EACCES;

This seems to be causing a lot of WARN level dmesg spam [1] during
suspend/resume on several machines. I suppose previously the
warning was only printed when trying to change the limits explicitly,
but now it gets printed in a lot more cases even if the user didn't
try to change anything.

[1] https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13594/fi-snb-2520m/igt@i915_suspend@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

-- 
Ville Syrjälä
Intel