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.
Regards,
Hans
