* [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups
@ 2024-02-17 1:41 Mario Limonciello
2024-02-17 1:41 ` [PATCH v2 1/2] platform/x86/amd/pmf: Add debugging message for missing policy data Mario Limonciello
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Mario Limonciello @ 2024-02-17 1:41 UTC (permalink / raw)
To: Shyam-sundar.S-k, hdegoede
Cc: platform-driver-x86, linux-kernel, Mario Limonciello
While debugging the suspend issue for amd-pmf the initial bisect result
pointed at red herrings of cleanup flow problems for
amd_pmf_init_smart_pc(). The actual issue wasn't in this code, but still
a lot of memory is allocated and not immediately released if any of the
error branches are taken.
This series cleans that up so that every step is cleaned up. I believe
this actually fixes driver bugs that "could" occur if a BIOS advertisd
Smart PC as well as ITS auto or CNQF but didn't include a policy in the
BIOS.
Mario Limonciello (2):
platform/x86/amd/pmf: Add debugging message for missing policy data
platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc()
drivers/platform/x86/amd/pmf/tee-if.c | 69 +++++++++++++++++----------
1 file changed, 43 insertions(+), 26 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 1/2] platform/x86/amd/pmf: Add debugging message for missing policy data 2024-02-17 1:41 [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Mario Limonciello @ 2024-02-17 1:41 ` Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() Mario Limonciello 2024-02-19 12:38 ` [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Hans de Goede 2 siblings, 0 replies; 5+ messages in thread From: Mario Limonciello @ 2024-02-17 1:41 UTC (permalink / raw) To: Shyam-sundar.S-k, hdegoede Cc: platform-driver-x86, linux-kernel, Mario Limonciello If a machine advertises Smart PC support but is missing policy data show a debugging message to help clarify why Smart PC wasn't enabled. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/platform/x86/amd/pmf/tee-if.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c index 8b7e3f87702e..1359ab340f7c 100644 --- a/drivers/platform/x86/amd/pmf/tee-if.c +++ b/drivers/platform/x86/amd/pmf/tee-if.c @@ -252,8 +252,10 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) cookie = readl(dev->policy_buf + POLICY_COOKIE_OFFSET); length = readl(dev->policy_buf + POLICY_COOKIE_LEN); - if (cookie != POLICY_SIGN_COOKIE || !length) + if (cookie != POLICY_SIGN_COOKIE || !length) { + dev_dbg(dev->dev, "cookie doesn't match\n"); return -EINVAL; + } /* Update the actual length */ dev->policy_sz = length + 512; -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() 2024-02-17 1:41 [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 1/2] platform/x86/amd/pmf: Add debugging message for missing policy data Mario Limonciello @ 2024-02-17 1:41 ` Mario Limonciello 2024-02-18 14:58 ` Hans de Goede 2024-02-19 12:38 ` [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Hans de Goede 2 siblings, 1 reply; 5+ messages in thread From: Mario Limonciello @ 2024-02-17 1:41 UTC (permalink / raw) To: Shyam-sundar.S-k, hdegoede Cc: platform-driver-x86, linux-kernel, Mario Limonciello amd_pmf_init_smart_pc() calls out to amd_pmf_get_bios_buffer() but the error handling flow doesn't clean everything up allocated memory. As amd_pmf_get_bios_buffer() is only called by amd_pmf_init_smart_pc(), fold it into the function and add labels to clean up any step that can fail along the way. Explicitly set everything allocated to NULL as there are other features that may access some of the same variables. Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v1->v2: * Use a single label * Move all into amd_pmf_deinit_smart_pc() * Set to NULL explicitly --- drivers/platform/x86/amd/pmf/tee-if.c | 65 ++++++++++++++++----------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c index 1359ab340f7c..4f74de680654 100644 --- a/drivers/platform/x86/amd/pmf/tee-if.c +++ b/drivers/platform/x86/amd/pmf/tee-if.c @@ -338,25 +338,6 @@ static void amd_pmf_remove_pb(struct amd_pmf_dev *dev) {} static void amd_pmf_hex_dump_pb(struct amd_pmf_dev *dev) {} #endif -static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev) -{ - dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); - if (!dev->policy_buf) - return -ENOMEM; - - dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz); - if (!dev->policy_base) - return -ENOMEM; - - memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz); - - amd_pmf_hex_dump_pb(dev); - if (pb_side_load) - amd_pmf_open_pb(dev, dev->dbgfs_dir); - - return amd_pmf_start_policy_engine(dev); -} - static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data) { return ver->impl_id == TEE_IMPL_ID_AMDTEE; @@ -455,22 +436,56 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) return ret; INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); - amd_pmf_set_dram_addr(dev, true); - amd_pmf_get_bios_buffer(dev); + + ret = amd_pmf_set_dram_addr(dev, true); + if (ret) + goto error; + + dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz); + if (!dev->policy_base) { + ret = -ENOMEM; + goto error; + } + + dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); + if (!dev->policy_buf) { + ret = -ENOMEM; + goto error; + } + + memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz); + + amd_pmf_hex_dump_pb(dev); + if (pb_side_load) + amd_pmf_open_pb(dev, dev->dbgfs_dir); + dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); if (!dev->prev_data) - return -ENOMEM; + goto error; - return dev->smart_pc_enabled; + ret = amd_pmf_start_policy_engine(dev); + if (ret) + goto error; + + return 0; + +error: + amd_pmf_deinit_smart_pc(dev); + + return ret; } void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev) { - if (pb_side_load) + if (pb_side_load && dev->esbin) amd_pmf_remove_pb(dev); + cancel_delayed_work_sync(&dev->pb_work); kfree(dev->prev_data); + dev->prev_data = NULL; kfree(dev->policy_buf); - cancel_delayed_work_sync(&dev->pb_work); + dev->policy_buf = NULL; + kfree(dev->buf); + dev->buf = NULL; amd_pmf_tee_deinit(dev); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() 2024-02-17 1:41 ` [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() Mario Limonciello @ 2024-02-18 14:58 ` Hans de Goede 0 siblings, 0 replies; 5+ messages in thread From: Hans de Goede @ 2024-02-18 14:58 UTC (permalink / raw) To: Mario Limonciello, Shyam-sundar.S-k; +Cc: platform-driver-x86, linux-kernel Hi Mario, Thank you for the quick v2. On 2/17/24 02:41, Mario Limonciello wrote: > amd_pmf_init_smart_pc() calls out to amd_pmf_get_bios_buffer() but > the error handling flow doesn't clean everything up allocated > memory. > > As amd_pmf_get_bios_buffer() is only called by amd_pmf_init_smart_pc(), > fold it into the function and add labels to clean up any step that > can fail along the way. Explicitly set everything allocated to NULL as > there are other features that may access some of the same variables. > > Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v1->v2: > * Use a single label > * Move all into amd_pmf_deinit_smart_pc() > * Set to NULL explicitly > --- > drivers/platform/x86/amd/pmf/tee-if.c | 65 ++++++++++++++++----------- > 1 file changed, 40 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c > index 1359ab340f7c..4f74de680654 100644 > --- a/drivers/platform/x86/amd/pmf/tee-if.c > +++ b/drivers/platform/x86/amd/pmf/tee-if.c > @@ -338,25 +338,6 @@ static void amd_pmf_remove_pb(struct amd_pmf_dev *dev) {} > static void amd_pmf_hex_dump_pb(struct amd_pmf_dev *dev) {} > #endif > > -static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev) > -{ > - dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); > - if (!dev->policy_buf) > - return -ENOMEM; > - > - dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz); > - if (!dev->policy_base) > - return -ENOMEM; > - > - memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz); > - > - amd_pmf_hex_dump_pb(dev); > - if (pb_side_load) > - amd_pmf_open_pb(dev, dev->dbgfs_dir); > - > - return amd_pmf_start_policy_engine(dev); > -} > - > static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data) > { > return ver->impl_id == TEE_IMPL_ID_AMDTEE; > @@ -455,22 +436,56 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) > return ret; > > INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); > - amd_pmf_set_dram_addr(dev, true); > - amd_pmf_get_bios_buffer(dev); > + > + ret = amd_pmf_set_dram_addr(dev, true); > + if (ret) > + goto error; > + > + dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz); > + if (!dev->policy_base) { > + ret = -ENOMEM; > + goto error; > + } > + > + dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); > + if (!dev->policy_buf) { > + ret = -ENOMEM; > + goto error; > + } > + > + memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz); > + > + amd_pmf_hex_dump_pb(dev); > + if (pb_side_load) > + amd_pmf_open_pb(dev, dev->dbgfs_dir); There is a small race here where the debugfs file can be used to load another policy while amd_pmf_start_policy_engine() is running. Leading to both the policy-buffer getting modified underneath the first thread running amd_pmf_start_policy_engine() and to 2 threads possibly running amd_pmf_start_policy_engine() at once. This is a pre-existing problem. Can you post a follow-up patch (on top of this series) to move the: if (pb_side_load) amd_pmf_open_pb(dev, dev->dbgfs_dir); To below the amd_pmf_start_policy_engine() call ? That avoids the possibility for this race. I don't expect this to be a problem in real life, but it would be good to close the race alltogether. Since this is not a problem with this patch: Reviewed-by: Hans de Goede <hdegoede@redhat.com> for this patch as-is. Regards, Hans > + > dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); > if (!dev->prev_data) > - return -ENOMEM; > + goto error; > > - return dev->smart_pc_enabled; > + ret = amd_pmf_start_policy_engine(dev); > + if (ret) > + goto error; > + > + return 0; > + > +error: > + amd_pmf_deinit_smart_pc(dev); > + > + return ret; > } > > void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev) > { > - if (pb_side_load) > + if (pb_side_load && dev->esbin) > amd_pmf_remove_pb(dev); > > + cancel_delayed_work_sync(&dev->pb_work); > kfree(dev->prev_data); > + dev->prev_data = NULL; > kfree(dev->policy_buf); > - cancel_delayed_work_sync(&dev->pb_work); > + dev->policy_buf = NULL; > + kfree(dev->buf); > + dev->buf = NULL; > amd_pmf_tee_deinit(dev); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups 2024-02-17 1:41 [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 1/2] platform/x86/amd/pmf: Add debugging message for missing policy data Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() Mario Limonciello @ 2024-02-19 12:38 ` Hans de Goede 2 siblings, 0 replies; 5+ messages in thread From: Hans de Goede @ 2024-02-19 12:38 UTC (permalink / raw) To: Mario Limonciello, Shyam-sundar.S-k; +Cc: platform-driver-x86, linux-kernel Hi, On 2/17/24 02:41, Mario Limonciello wrote: > While debugging the suspend issue for amd-pmf the initial bisect result > pointed at red herrings of cleanup flow problems for > amd_pmf_init_smart_pc(). The actual issue wasn't in this code, but still > a lot of memory is allocated and not immediately released if any of the > error branches are taken. > > This series cleans that up so that every step is cleaned up. I believe > this actually fixes driver bugs that "could" occur if a BIOS advertisd > Smart PC as well as ITS auto or CNQF but didn't include a policy in the > BIOS. > > Mario Limonciello (2): > platform/x86/amd/pmf: Add debugging message for missing policy data > platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() Thank you for your patch-series, I've applied this series to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. I will include this patch in my next fixes pull-req to Linus for the current kernel development cycle. Regards, Hans ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-19 12:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-17 1:41 [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 1/2] platform/x86/amd/pmf: Add debugging message for missing policy data Mario Limonciello 2024-02-17 1:41 ` [PATCH v2 2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc() Mario Limonciello 2024-02-18 14:58 ` Hans de Goede 2024-02-19 12:38 ` [PATCH v2 0/2] AMD PMF Smart PC error handling cleanups Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).