Re: [PATCH v6 03/15] platform/x86/amd/pmf: Change return type of amd_pmf_set_dram_addr()

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

Hi Hans,

On 12/11/2023 2:10 PM, Hans de Goede wrote:
> 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@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  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.
> 

Ok. I have made this change.

> 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.

With some sanity tests, I don't think so this is required. pdev->buf
is only used to get the metrics table info. So, I am keeping only the
freeing part + alloc_buffer thing and not cancel/resubmit in the
suspend/resume.

If there are issues in the future because of not including
cancel/resubmit thing, can we work that as a bug fix later? Would that
be OK for you?

Thanks,
Shyam

> 
> 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);
>