Re: [PATCH v2 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> On 10/4/2023 5:44 PM, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> > 
> >> PMF driver sends changing inputs from each subystem to TA for evaluating
> >> the conditions in the policy binary.
> >>
> >> Add initial support of plumbing in the PMF driver for Smart PC to get
> >> information from other subsystems in the kernel.
> >>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

> >> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> >> new file mode 100644
> >> index 000000000000..3113bde051d9
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/amd/pmf/spc.c
> >> @@ -0,0 +1,119 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * AMD Platform Management Framework Driver - Smart PC Capabilities
> >> + *
> >> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> >> + * All Rights Reserved.
> >> + *
> >> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> + *          Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> + */
> >> +
> >> +#include <acpi/button.h>
> >> +#include <linux/power_supply.h>
> >> +#include <linux/units.h>
> >> +#include "pmf.h"
> >> +
> >> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >> +{
> >> +	u16 max, avg = 0;
> >> +	int i;
> >> +
> >> +	memset(dev->buf, 0, sizeof(dev->m_table));
> >> +	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> >> +	memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> >> +
> >> +	in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> >> +	in->ev_info.skin_temperature = dev->m_table.skin_temp;
> >> +
> >> +	/* get the avg C0 residency of all the cores */
> >> +	for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++)
> >> +		avg += dev->m_table.avg_core_c0residency[i];
> >> +
> >> +	/* get the max C0 residency of all the cores */
> >> +	max = dev->m_table.avg_core_c0residency[0];
> >> +	for (i = 1; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> >> +		if (dev->m_table.avg_core_c0residency[i] > max)
> >> +			max = dev->m_table.avg_core_c0residency[i];
> >> +	}
> > 
> > My comments weren't either answered adequately or changes made here.
> > Please check the v1 comments. I hope it's not because you feel hurry to 
> > get the next version out...
> > 
> > I'm still unsure if the u16 thing can overflow because I don't know what's 
> > the max value for avg_core_c0residency[i].
> 
> the highest value for avg_core_c0residency[i] is merely a small number
> and hence I retained the avg variable as u16. Not sure if there a
> 'real' case where it can overflow.

Okay, if you think it's fine, no problem with it then (not that there's 
a big advantage having it as u16 instead of e.g. unsigned int).

> Sorry, I missed to merge both into a single for loop. I will address
> this in v3.

Thanks.

-- 
 i.