Hi,
On 12/4/23 11:15, Shyam Sundar S K wrote:
> In the current code, the metrics table information was required only
> for auto-mode or CnQF at a given time. Hence keeping the return type
> of amd_pmf_set_dram_addr() as static made sense.
>
> But with the addition of Smart PC builder feature, the metrics table
> information has to be shared by the Smart PC also and this feature
> resides outside of core.c.
>
> To make amd_pmf_set_dram_addr() visible outside of core.c make it
> as a non-static function and move the allocation of memory for
> metrics table from amd_pmf_init_metrics_table() to amd_pmf_set_dram_addr()
> as amd_pmf_set_dram_addr() is the common function to set the DRAM
> address.
>
> Add a suspend handler that can free up the allocated memory for getting
> the metrics table information.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
> drivers/platform/x86/amd/pmf/core.c | 42 ++++++++++++++++++++++-------
> drivers/platform/x86/amd/pmf/pmf.h | 1 +
> 2 files changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index ec92d1cc0dac..f50b7ec9a625 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -251,29 +251,35 @@ static const struct pci_device_id pmf_pci_ids[] = {
> { }
> };
>
> -static void amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> {
> u64 phys_addr;
> u32 hi, low;
>
> + /* Get Metrics Table Address */
> + dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> + if (!dev->buf)
> + return -ENOMEM;
> +
> phys_addr = virt_to_phys(dev->buf);
> hi = phys_addr >> 32;
> low = phys_addr & GENMASK(31, 0);
>
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, hi, NULL);
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, low, NULL);
> +
> + return 0;
> }
>
> int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> {
> - /* Get Metrics Table Address */
> - dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> - if (!dev->buf)
> - return -ENOMEM;
> + int ret;
>
> INIT_DELAYED_WORK(&dev->work_buffer, amd_pmf_get_metrics);
>
> - amd_pmf_set_dram_addr(dev);
> + ret = amd_pmf_set_dram_addr(dev);
> + if (ret)
> + return ret;
>
> /*
> * Start collecting the metrics data after a small delay
> @@ -284,17 +290,35 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> return 0;
> }
>
> +static int amd_pmf_suspend_handler(struct device *dev)
> +{
> + struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
> +
> + /*
> + * Free the buffer allocated for storing the metrics table
> + * information, as will have to allocate it freshly after
> + * resume.
> + */
> + kfree(pdev->buf);
This seems quite risky. You are freeing the memory here,
but the SET_DRAM_ADDR_HIGH and SET_DRAM_ADDR_LO registers
still point to it, so the hw may still access it.
IMHO it would be better to add a "bool alloc_buffer"
parameter to amd_pmf_set_dram_addr() and set that
to true on init and false on resume.
Also I guess that this DRAM buffer is used by the new
smartpc mode and specifically by the command send by
amd_pmf_invoke_cmd() work. In that case you really
need to make sure to cancel the work before
freeing the buffer and then re-submit the work
on resume.
Regards,
Hans
> +
> + return 0;
> +}
> +
> static int amd_pmf_resume_handler(struct device *dev)
> {
> struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
> + int ret;
>
> - if (pdev->buf)
> - amd_pmf_set_dram_addr(pdev);
> + if (pdev->buf) {
> + ret = amd_pmf_set_dram_addr(pdev);
> + if (ret)
> + return ret;
> + }
>
> return 0;
> }
>
> -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmf_pm, NULL, amd_pmf_resume_handler);
> +static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmf_pm, amd_pmf_suspend_handler, amd_pmf_resume_handler);
>
> static void amd_pmf_init_features(struct amd_pmf_dev *dev)
> {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index a24e34e42032..6a0e4c446dd3 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -421,6 +421,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
> int amd_pmf_get_power_source(void);
> int apmf_install_handler(struct amd_pmf_dev *pmf_dev);
> int apmf_os_power_slider_update(struct amd_pmf_dev *dev, u8 flag);
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev);
>
> /* SPS Layer */
> int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
