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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On 12/11/2023 9:25 PM, Hans de Goede wrote:
> Hi Shyam,
> 
> On 12/11/23 16:15, Shyam Sundar S K wrote:
>> 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@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.
>>>
>>
>> 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?
> 
> If you are sure that it is safe to keep the work running
> after your suspend-handler has run and on resume to
> have it running before your resume handler has run
> then yes that is ok.
> 
> This seems unwise though, adding a sync kill of the
> work + requeue is only a couple of lines of code.
> 
> And what about accessing other subsystems like the
> AMD HID sensors, I guess the work item may do that
> too? That esp. seems unwise to do after your
> suspend-handler has run.
> 
> Even if you stop the work from your suspend handler
> you may need to add some dev-links to ensure
> that the PMF linux-device is suspended before
> e.g. the AMD HID sensors. That can be done in
> a follow up patch though.
> 

Sure, thank you. Will address them separately as a followup patch later.

Thanks,
Shyam

> Regards,
> 
> Hans
> 
> 
> 




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux