In pcc_mbox_probe(), the PCCT table acquired via acpi_get_table() is only released in error paths but not in the success path. This leads to a permanent ACPI memory leak when the driver successfully initializes.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn --- Changes in v2: - Add tags of 'Fixes' and 'Cc' - Change goto target from out_put_pcct to e_nomem --- drivers/mailbox/pcc.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index f6714c233f5a..b5b4e3665593 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -763,19 +763,19 @@ static int pcc_mbox_probe(struct platform_device *pdev) GFP_KERNEL); if (!pcc_mbox_channels) { rc = -ENOMEM; - goto err; + goto e_nomem; }
chan_info = devm_kcalloc(dev, count, sizeof(*chan_info), GFP_KERNEL); if (!chan_info) { rc = -ENOMEM; - goto err; + goto e_nomem; }
pcc_mbox_ctrl = devm_kzalloc(dev, sizeof(*pcc_mbox_ctrl), GFP_KERNEL); if (!pcc_mbox_ctrl) { rc = -ENOMEM; - goto err; + goto e_nomem; }
/* Point to the first PCC subspace entry */ @@ -796,17 +796,17 @@ static int pcc_mbox_probe(struct platform_device *pdev) !pcc_mbox_ctrl->txdone_irq) { pr_err("Platform Interrupt flag must be set to 1"); rc = -EINVAL; - goto err; + goto e_nomem; }
if (pcc_mbox_ctrl->txdone_irq) { rc = pcc_parse_subspace_irq(pchan, pcct_entry); if (rc < 0) - goto err; + goto e_nomem; } rc = pcc_parse_subspace_db_reg(pchan, pcct_entry); if (rc < 0) - goto err; + goto e_nomem;
pcc_parse_subspace_shmem(pchan, pcct_entry);
@@ -827,9 +827,8 @@ static int pcc_mbox_probe(struct platform_device *pdev) rc = mbox_controller_register(pcc_mbox_ctrl); if (rc) pr_err("Err registering PCC as Mailbox controller: %d\n", rc); - else - return 0; -err: + +e_nomem: acpi_put_table(pcct_tbl); return rc; }
In pcc_mbox_probe(), the PCCT table acquired via acpi_get_table() is only released in error paths but not in the success path. This leads to a permanent ACPI memory leak when the driver successfully initializes.
* You may occasionally put more than 66 characters into text lines of such a change description. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
* See also once more: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
…> ---
Changes in v2:
- Add tags of 'Fixes' and 'Cc'
- Change goto target from out_put_pcct to e_nomem
…> +++ b/drivers/mailbox/pcc.c
@@ -763,19 +763,19 @@ static int pcc_mbox_probe(struct platform_device *pdev) GFP_KERNEL); if (!pcc_mbox_channels) { rc = -ENOMEM;
goto err;
goto e_nomem;
…
Why do you not move the assignment statement accordingly?
…> @@ -796,17 +796,17 @@ static int pcc_mbox_probe(struct platform_device *pdev)
!pcc_mbox_ctrl->txdone_irq) { pr_err("Platform Interrupt flag must be set to 1"); rc = -EINVAL;
goto err;
goto e_nomem;
…
You misunderstood one of my development suggestions.
…> @@ -827,9 +827,8 @@ static int pcc_mbox_probe(struct platform_device *pdev) …> - return 0;
-err:
+e_nomem: acpi_put_table(pcct_tbl); return rc; }
Would an other code variant be more reasonable?
e_nomem: rc = -ENOMEM; goto err;
Regards, Markus
In pcc_mbox_probe(), the PCCT table acquired via acpi_get_table() is only released in error paths but not in the success path. Fix a permanent ACPI memory leak when the driver successfully initializes. Add the goto label 'err_nomem'.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn --- Changes in v2: - Add tags of 'Fixes' and 'Cc' - Change goto target from out_put_pcct to e_nomem --- Changes in v3: - Add goto label err_nomem, keep the err label. - Update commit msg --- drivers/mailbox/pcc.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index f6714c233f5a..b5ce3a5d2e7a 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -761,22 +761,16 @@ static int pcc_mbox_probe(struct platform_device *pdev)
pcc_mbox_channels = devm_kcalloc(dev, count, sizeof(*pcc_mbox_channels), GFP_KERNEL); - if (!pcc_mbox_channels) { - rc = -ENOMEM; - goto err; - } + if (!pcc_mbox_channels) + goto err_nomem;
chan_info = devm_kcalloc(dev, count, sizeof(*chan_info), GFP_KERNEL); - if (!chan_info) { - rc = -ENOMEM; - goto err; - } + if (!chan_info) + goto err_nomem;
pcc_mbox_ctrl = devm_kzalloc(dev, sizeof(*pcc_mbox_ctrl), GFP_KERNEL); - if (!pcc_mbox_ctrl) { - rc = -ENOMEM; - goto err; - } + if (!pcc_mbox_ctrl) + goto err_nomem;
/* Point to the first PCC subspace entry */ pcct_entry = (struct acpi_subtable_header *) ( @@ -827,8 +821,11 @@ static int pcc_mbox_probe(struct platform_device *pdev) rc = mbox_controller_register(pcc_mbox_ctrl); if (rc) pr_err("Err registering PCC as Mailbox controller: %d\n", rc); - else - return 0; + goto err; + +err_nomem: + rc = -ENOMEM; + goto err; err: acpi_put_table(pcct_tbl); return rc;
In pcc_mbox_probe(), the PCCT table acquired via acpi_get_table() is only released in error paths but not in the success path. Fix a permanent ACPI memory leak when the driver successfully initializes. Add the goto label 'err_nomem'.
You may use an enumeration for your change description.
* Fix …
* Add … so that a bit of exception handling can be refined.
…> +++ b/drivers/mailbox/pcc.c …> @@ -827,8 +821,11 @@ static int pcc_mbox_probe(struct platform_device *pdev) …> +err_nomem:
- rc = -ENOMEM;
- goto err;
Please omit such a redundant statement at this proposed place.
err: acpi_put_table(pcct_tbl); return rc;
Would a label like “put_table” become helpful here?
Regards, Markus
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn --- Changes in v4: - Change goto target from err to put_table. - Remove goto tatget err_nomem - Update commit msg Changes in v3: - Add goto label err_nomem, keep the err label. - Update commit msg Changes in v2: - Add tags of 'Fixes' and 'Cc' - Change goto target from out_put_pcct to e_nomem --- drivers/mailbox/pcc.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index f6714c233f5a..d8420c87b615 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -763,19 +763,19 @@ static int pcc_mbox_probe(struct platform_device *pdev) GFP_KERNEL); if (!pcc_mbox_channels) { rc = -ENOMEM; - goto err; + goto put_table; }
chan_info = devm_kcalloc(dev, count, sizeof(*chan_info), GFP_KERNEL); if (!chan_info) { rc = -ENOMEM; - goto err; + goto put_table; }
pcc_mbox_ctrl = devm_kzalloc(dev, sizeof(*pcc_mbox_ctrl), GFP_KERNEL); if (!pcc_mbox_ctrl) { rc = -ENOMEM; - goto err; + goto put_table; }
/* Point to the first PCC subspace entry */ @@ -796,17 +796,17 @@ static int pcc_mbox_probe(struct platform_device *pdev) !pcc_mbox_ctrl->txdone_irq) { pr_err("Platform Interrupt flag must be set to 1"); rc = -EINVAL; - goto err; + goto put_table; }
if (pcc_mbox_ctrl->txdone_irq) { rc = pcc_parse_subspace_irq(pchan, pcct_entry); if (rc < 0) - goto err; + goto put_table; } rc = pcc_parse_subspace_db_reg(pchan, pcct_entry); if (rc < 0) - goto err; + goto put_table;
pcc_parse_subspace_shmem(pchan, pcct_entry);
@@ -827,9 +827,8 @@ static int pcc_mbox_probe(struct platform_device *pdev) rc = mbox_controller_register(pcc_mbox_ctrl); if (rc) pr_err("Err registering PCC as Mailbox controller: %d\n", rc); - else - return 0; -err: + +put_table: acpi_put_table(pcct_tbl); return rc; }
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
Will a desire grow for the usage of imperative mood also in such change descriptions?
See also once more: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
…> Changes in v4:
- Change goto target from err to put_table.
Thanks.
- Remove goto tatget err_nomem
…
Does this adjustment indicate questionable development difficulties?
Regards, Markus
On Tue, Aug 05, 2025 at 08:28:17AM +0200, Markus Elfring wrote:
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
Will a desire grow for the usage of imperative mood also in such change descriptions?
See also once more: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
…> Changes in v4:
- Change goto target from err to put_table.
Thanks.
- Remove goto tatget err_nomem
…
Does this adjustment indicate questionable development difficulties?
Regards, Markus
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.
thanks,
greg k-h's patch email bot
On Tue, Aug 05, 2025 at 11:48:29AM +0800, Zhen Ni wrote:
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
I don't see any point in this unnecessary churn by renaming the label as there is only one label here. That said, I am not too picky either. The patch can be as simple as dropping the "else return 0;" part.
Either way,
Reviewed-by: Sudeep Holla sudeep.holla@arm.com
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table().
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn --- Changes in v5: - Discard err label modification Changes in v4: - Change goto target from err to put_table. - Remove goto tatget err_nomem - Update commit msg Changes in v3: - Add goto label err_nomem, keep the err label. - Update commit msg Changes in v2: - Add tags of 'Fixes' and 'Cc' - Change goto target from out_put_pcct to e_nomem --- drivers/mailbox/pcc.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index f6714c233f5a..70d47f8759eb 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -827,8 +827,6 @@ static int pcc_mbox_probe(struct platform_device *pdev) rc = mbox_controller_register(pcc_mbox_ctrl); if (rc) pr_err("Err registering PCC as Mailbox controller: %d\n", rc); - else - return 0; err: acpi_put_table(pcct_tbl); return rc;
On Thu, Aug 07, 2025 at 11:28:31AM +0800, Zhen Ni wrote:
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table().
NACK as you need to copy all GAS register reference which otherwise would cause issue as reported to your v4. Sorry I do know I did provide the Reviewed-by tag as I clearly missed to see this copy of reference to the GAS registers in the PCCT which is being accessed later which makes it impossible to drop the table reference unless you copy them to local structures.
Hello,
This commit, now part of next-20250807[1], is causing kernel boot crash on AMD EPYC Milan platform.
commit c3f772c384c8ec0796a73c80f89a31ff862c1295 (HEAD) Author: Zhen Ni zhen.ni@easystack.cn Date: Tue Aug 5 11:48:29 2025 +0800
mailbox: pcc: Add missed acpi_put_table() to fix memory leak
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn Signed-off-by: Jassi Brar jassisinghbrar@gmail.com
Attaching both the dmesg and kernel config here.
If I go back 1 commit [i.e 75f1fbc9fd409a0c232dc78871ee7df186da9d57], things work fine.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tag/?h=n...
Reported-by: Srikanth Aithal sraithal@amd.com
On 8/5/2025 9:18 AM, Zhen Ni wrote:
Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn
Changes in v4:
- Change goto target from err to put_table.
- Remove goto tatget err_nomem
- Update commit msg
Changes in v3:
- Add goto label err_nomem, keep the err label.
- Update commit msg
Changes in v2:
- Add tags of 'Fixes' and 'Cc'
- Change goto target from out_put_pcct to e_nomem
drivers/mailbox/pcc.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index f6714c233f5a..d8420c87b615 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -763,19 +763,19 @@ static int pcc_mbox_probe(struct platform_device *pdev) GFP_KERNEL); if (!pcc_mbox_channels) { rc = -ENOMEM;
goto err;
goto put_table;chan_info = devm_kcalloc(dev, count, sizeof(*chan_info), GFP_KERNEL); if (!chan_info) { rc = -ENOMEM;
goto err;
goto put_table;pcc_mbox_ctrl = devm_kzalloc(dev, sizeof(*pcc_mbox_ctrl), GFP_KERNEL); if (!pcc_mbox_ctrl) { rc = -ENOMEM;
goto err;
goto put_table;/* Point to the first PCC subspace entry */ @@ -796,17 +796,17 @@ static int pcc_mbox_probe(struct platform_device *pdev) !pcc_mbox_ctrl->txdone_irq) { pr_err("Platform Interrupt flag must be set to 1"); rc = -EINVAL;
goto err;
goto put_table;if (pcc_mbox_ctrl->txdone_irq) { rc = pcc_parse_subspace_irq(pchan, pcct_entry); if (rc < 0)
goto err;
goto put_table;
goto err;
goto put_table;pcc_parse_subspace_shmem(pchan, pcct_entry); @@ -827,9 +827,8 @@ static int pcc_mbox_probe(struct platform_device *pdev) rc = mbox_controller_register(pcc_mbox_ctrl); if (rc) pr_err("Err registering PCC as Mailbox controller: %d\n", rc);
- else
return 0;-err:
+put_table: acpi_put_table(pcct_tbl); return rc; }
On Thu, Aug 07, 2025 at 07:10:35PM +0530, Aithal, Srikanth wrote:
Hello,
This commit, now part of next-20250807[1], is causing kernel boot crash on AMD EPYC Milan platform.
commit c3f772c384c8ec0796a73c80f89a31ff862c1295 (HEAD) Author: Zhen Ni zhen.ni@easystack.cn Date: Tue Aug 5 11:48:29 2025 +0800
mailbox: pcc: Add missed acpi_put_table() to fix memory leak Fixes a permanent ACPI memory leak in the success path by adding acpi_put_table(). Renaming generic 'err' label to 'put_table' for clarity. Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing intopcc_mbox_probe") Cc: stable@vger.kernel.org Signed-off-by: Zhen Ni zhen.ni@easystack.cn Signed-off-by: Jassi Brar jassisinghbrar@gmail.com
Attaching both the dmesg and kernel config here.
If I go back 1 commit [i.e 75f1fbc9fd409a0c232dc78871ee7df186da9d57], things work fine.
Thanks for the report. I analysed the report and concluded that we can't drop the reference to the table unless we may a copy of some GAS registers as we access them at runtime.
Zhen,
Sorry, I must withdraw my Reviewed-by.
On Thu, Aug 7, 2025 at 8:40 AM Aithal, Srikanth sraithal@amd.com wrote:
Hello,
This commit, now part of next-20250807[1], is causing kernel boot crash on AMD EPYC Milan platform.
commit c3f772c384c8ec0796a73c80f89a31ff862c1295 (HEAD) Author: Zhen Ni zhen.ni@easystack.cn
Zhen, while it looked reasonable to have a corresponding acpi_put_table() , I also see the documentation of acpi_get_table() talks about early vs late stage calls. I don't know enough about it to be sure that doesn't apply here. So I am going to drop the patch until it gets reviewed by acpi/pcc folks.
Thanks, Jassi
linux-stable-mirror@lists.linaro.org
-
Aithal, Srikanth -
Greg KH -
Jassi Brar -
Markus Elfring -
Sudeep Holla -
Zhen Ni