| Message ID | 20231204101548.1458499-4-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce PMF Smart PC Solution Builder Feature | expand |
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. 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);
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); >
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@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? 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
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@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? > > 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 > > >
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); + + 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);