[PATCH] x86/amd/node: Release reserved config regions on init error

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH] x86/amd/node: Release reserved config regions on init error
@ 2026-09-15 16:22 yolezz
  2026-09-15 16:35 ` Mario Limonciello
  0 siblings, 1 reply; 10+ messages in thread
From: yolezz @ 2026-09-15 16:22 UTC (permalink / raw)
  To: mario.limonciello, yazen.ghannam; +Cc: x86, linux-kernel, yolezz

In amd_smn_init(), if pci_request_config_region_exclusive() fails or
if kzalloc_objs() fails to allocate memory for amd_roots, the already
reserved PCI config regions are left allocated.

Add amd_smn_release_config_regions() to clean up and release all
reserved PCI config space regions before returning an error code.

Signed-off-by: yolezz <yolezz.secret@gmail.com>
---
 arch/x86/kernel/amd_node.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index b7926ba3610a..6a96d888b5a7 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -239,6 +239,15 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
 	return root;
 }
 
+static void amd_smn_release_config_regions(u16 num_roots)
+{
+	struct pci_dev *root __free(pci_dev_put) = NULL;
+
+	/* Release the PCI config space for each root device. */
+	while (num_roots-- && (root = get_next_root(root)))
+		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
+}
+
 static bool enable_dfs;
 
 static int __init amd_smn_enable_dfs(char *str)
@@ -273,6 +282,7 @@ static int __init amd_smn_init(void)
 		 */
 		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
 			pci_err(root, "Failed to reserve config space\n");
+			amd_smn_release_config_regions(num_roots);
 			return -EEXIST;
 		}
 
@@ -286,8 +296,10 @@ static int __init amd_smn_init(void)
 
 	num_nodes = amd_num_nodes();
 	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
-	if (!amd_roots)
+	if (!amd_roots) {
+		amd_smn_release_config_regions(num_roots);
 		return -ENOMEM;
+	}
 
 	roots_per_node = num_roots / num_nodes;
 	if (!roots_per_node) {
-- 
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/amd/node: Release reserved config regions on init error
  2026-09-15 16:22 [PATCH] x86/amd/node: Release reserved config regions on init error yolezz
@ 2026-09-15 16:35 ` Mario Limonciello
  2026-09-15 17:23   ` [PATCH v2] " yolezz
                     ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Mario Limonciello @ 2026-09-15 16:35 UTC (permalink / raw)
  To: yolezz, yazen.ghannam; +Cc: x86, linux-kernel



On 9/15/26 11:22, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Add amd_smn_release_config_regions() to clean up and release all
> reserved PCI config space regions before returning an error code.
> 
> Signed-off-by: yolezz <yolezz.secret@gmail.com>
> ---
>   arch/x86/kernel/amd_node.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index b7926ba3610a..6a96d888b5a7 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -239,6 +239,15 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>   	return root;
>   }
>   
> +static void amd_smn_release_config_regions(u16 num_roots)
> +{
> +	struct pci_dev *root __free(pci_dev_put) = NULL;
> +
> +	/* Release the PCI config space for each root device. */
> +	while (num_roots-- && (root = get_next_root(root)))
> +		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
> +}
> +
>   static bool enable_dfs;
>   
>   static int __init amd_smn_enable_dfs(char *str)
> @@ -273,6 +282,7 @@ static int __init amd_smn_init(void)
>   		 */
>   		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
>   			pci_err(root, "Failed to reserve config space\n");
> +			amd_smn_release_config_regions(num_roots);
>   			return -EEXIST;
>   		}
>   
> @@ -286,8 +296,10 @@ static int __init amd_smn_init(void)
>   
>   	num_nodes = amd_num_nodes();
>   	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
> -	if (!amd_roots)
> +	if (!amd_roots) {
> +		amd_smn_release_config_regions(num_roots);
>   		return -ENOMEM;
> +	}
>   
>   	roots_per_node = num_roots / num_nodes;
>   	if (!roots_per_node) {

Given this is a cleanup on failure exit path, would it be better to use 
a __free() helper perhaps?  That would mean just needing to set it to 
NULL on the success path.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] x86/amd/node: Release reserved config regions on init error
  2026-09-15 16:35 ` Mario Limonciello
@ 2026-09-15 17:23   ` yolezz
  2026-09-15 17:32     ` Mario Limonciello
  2026-09-15 17:36   ` [PATCH v3] " yolezz
  2026-09-15 18:05   ` [PATCH v4] " yolezz
  2 siblings, 1 reply; 10+ messages in thread
From: yolezz @ 2026-09-15 17:23 UTC (permalink / raw)
  To: mario.limonciello, yazen.ghannam; +Cc: x86, linux-kernel, yolezz

In amd_smn_init(), if pci_request_config_region_exclusive() fails or
if kzalloc_objs() fails to allocate memory for amd_roots, the already
reserved PCI config regions are left allocated.

Use a __free() cleanup helper to automatically release all reserved PCI
config space regions on error exit paths.

Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
Signed-off-by: yolezz <yolezz.secret@gmail.com>
---
v2:
 - Use a __free() cleanup helper instead of manual error rollback, as suggested by Mario.

 arch/x86/kernel/amd_node.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index 6a96d888b5a7..20451f0ad8c7 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -239,15 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
 	return root;
 }
 
-static void amd_smn_release_config_regions(u16 num_roots)
+static void amd_smn_release_config_regions(u16 *num_roots)
 {
 	struct pci_dev *root __free(pci_dev_put) = NULL;
 
-	/* Release the PCI config space for each root device. */
-	while (num_roots-- && (root = get_next_root(root)))
+	if (!num_roots)
+		return;
+
+	while (*num_roots && (root = get_next_root(root))) {
 		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
+		(*num_roots)--;
+	}
 }
 
+DEFINE_FREE(amd_smn_release_config_regions, u16 *,
+	    amd_smn_release_config_regions(_T));
+
 static bool enable_dfs;
 
 static int __init amd_smn_enable_dfs(char *str)
@@ -259,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
 
 static int __init amd_smn_init(void)
 {
-	u16 count, num_roots, roots_per_node, node, num_nodes;
+	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
 	struct pci_dev *root __free(pci_dev_put) = NULL;
 
+	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
+
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
 		return 0;
 
@@ -270,7 +279,8 @@ static int __init amd_smn_init(void)
 	if (amd_roots)
 		return 0;
 
-	num_roots = 0;
+	config_regions = &num_roots;
+
 	while ((root = get_next_root(root))) {
 		pci_dbg(root, "Reserving PCI config space\n");
 
@@ -282,7 +292,6 @@ static int __init amd_smn_init(void)
 		 */
 		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
 			pci_err(root, "Failed to reserve config space\n");
-			amd_smn_release_config_regions(num_roots);
 			return -EEXIST;
 		}
 
@@ -297,7 +306,6 @@ static int __init amd_smn_init(void)
 	num_nodes = amd_num_nodes();
 	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
 	if (!amd_roots) {
-		amd_smn_release_config_regions(num_roots);
 		return -ENOMEM;
 	}
 
@@ -327,6 +335,7 @@ static int __init amd_smn_init(void)
 		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
 	}
 
+	config_regions = NULL;
 	return 0;
 }
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] x86/amd/node: Release reserved config regions on init error
  2026-09-15 17:23   ` [PATCH v2] " yolezz
@ 2026-09-15 17:32     ` Mario Limonciello
  0 siblings, 0 replies; 10+ messages in thread
From: Mario Limonciello @ 2026-09-15 17:32 UTC (permalink / raw)
  To: yolezz, yazen.ghannam; +Cc: x86, linux-kernel



On 9/15/26 12:23, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Use a __free() cleanup helper to automatically release all reserved PCI
> config space regions on error exit paths.
> 
> Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
> Signed-off-by: yolezz <yolezz.secret@gmail.com>
> ---
> v2:
>   - Use a __free() cleanup helper instead of manual error rollback, as suggested by Mario.
> 
>   arch/x86/kernel/amd_node.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index 6a96d888b5a7..20451f0ad8c7 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -239,15 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>   	return root;
>   }
>   
> -static void amd_smn_release_config_regions(u16 num_roots)
> +static void amd_smn_release_config_regions(u16 *num_roots)
>   {
>   	struct pci_dev *root __free(pci_dev_put) = NULL;
>   
> -	/* Release the PCI config space for each root device. */
> -	while (num_roots-- && (root = get_next_root(root)))
> +	if (!num_roots)
> +		return;
> +
> +	while (*num_roots && (root = get_next_root(root))) {
>   		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
> +		(*num_roots)--;
> +	}
>   }
>   
> +DEFINE_FREE(amd_smn_release_config_regions, u16 *,
> +	    amd_smn_release_config_regions(_T));
> +
>   static bool enable_dfs;
>   
>   static int __init amd_smn_enable_dfs(char *str)
> @@ -259,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
>   
>   static int __init amd_smn_init(void)
>   {
> -	u16 count, num_roots, roots_per_node, node, num_nodes;
> +	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
>   	struct pci_dev *root __free(pci_dev_put) = NULL;
>   
> +	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
> +
>   	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
>   		return 0;
>   
> @@ -270,7 +279,8 @@ static int __init amd_smn_init(void)
>   	if (amd_roots)
>   		return 0;
>   
> -	num_roots = 0;
> +	config_regions = &num_roots;
> +
>   	while ((root = get_next_root(root))) {
>   		pci_dbg(root, "Reserving PCI config space\n");
>   
> @@ -282,7 +292,6 @@ static int __init amd_smn_init(void)
>   		 */
>   		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
>   			pci_err(root, "Failed to reserve config space\n");
> -			amd_smn_release_config_regions(num_roots);

Did you forget to "remove" your v1 from the tree?  You should squash 
your v1 and v2 together as though it's one patch, not as though your v1 
was accepted.

I would just do this as a v3.

>   			return -EEXIST;
>   		}
>   
> @@ -297,7 +306,6 @@ static int __init amd_smn_init(void)
>   	num_nodes = amd_num_nodes();
>   	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
>   	if (!amd_roots) {
> -		amd_smn_release_config_regions(num_roots);
>   		return -ENOMEM;
>   	}
>   
> @@ -327,6 +335,7 @@ static int __init amd_smn_init(void)
>   		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
>   	}
>   
> +	config_regions = NULL;
>   	return 0;
>   }
>   


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] x86/amd/node: Release reserved config regions on init error
  2026-09-15 16:35 ` Mario Limonciello
  2026-09-15 17:23   ` [PATCH v2] " yolezz
@ 2026-09-15 17:36   ` yolezz
  2026-09-15 17:42     ` Mario Limonciello
  2026-09-15 18:05   ` [PATCH v4] " yolezz
  2 siblings, 1 reply; 10+ messages in thread
From: yolezz @ 2026-09-15 17:36 UTC (permalink / raw)
  To: mario.limonciello, yazen.ghannam; +Cc: x86, linux-kernel, yolezz

In amd_smn_init(), if pci_request_config_region_exclusive() fails or
if kzalloc_objs() fails to allocate memory for amd_roots, the already
reserved PCI config regions are left allocated.

Use a __free() cleanup helper to automatically release all reserved PCI
config space regions on error exit paths.

Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
Signed-off-by: yolezz <yolezz.secret@gmail.com>
---
v3:
 - Squash changes into a single standalone patch to fix multi-patch confusion.

 arch/x86/kernel/amd_node.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index b7926ba3610a..20451f0ad8c7 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -239,6 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
 	return root;
 }
 
+static void amd_smn_release_config_regions(u16 *num_roots)
+{
+	struct pci_dev *root __free(pci_dev_put) = NULL;
+
+	if (!num_roots)
+		return;
+
+	while (*num_roots && (root = get_next_root(root))) {
+		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
+		(*num_roots)--;
+	}
+}
+
+DEFINE_FREE(amd_smn_release_config_regions, u16 *,
+	    amd_smn_release_config_regions(_T));
+
 static bool enable_dfs;
 
 static int __init amd_smn_enable_dfs(char *str)
@@ -250,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
 
 static int __init amd_smn_init(void)
 {
-	u16 count, num_roots, roots_per_node, node, num_nodes;
+	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
 	struct pci_dev *root __free(pci_dev_put) = NULL;
 
+	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
+
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
 		return 0;
 
@@ -261,7 +279,8 @@ static int __init amd_smn_init(void)
 	if (amd_roots)
 		return 0;
 
-	num_roots = 0;
+	config_regions = &num_roots;
+
 	while ((root = get_next_root(root))) {
 		pci_dbg(root, "Reserving PCI config space\n");
 
@@ -286,8 +305,9 @@ static int __init amd_smn_init(void)
 
 	num_nodes = amd_num_nodes();
 	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
-	if (!amd_roots)
+	if (!amd_roots) {
 		return -ENOMEM;
+	}
 
 	roots_per_node = num_roots / num_nodes;
 	if (!roots_per_node) {
@@ -315,6 +335,7 @@ static int __init amd_smn_init(void)
 		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
 	}
 
+	config_regions = NULL;
 	return 0;
 }
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] x86/amd/node: Release reserved config regions on init error
  2026-09-15 17:36   ` [PATCH v3] " yolezz
@ 2026-09-15 17:42     ` Mario Limonciello
  0 siblings, 0 replies; 10+ messages in thread
From: Mario Limonciello @ 2026-09-15 17:42 UTC (permalink / raw)
  To: yolezz, yazen.ghannam; +Cc: x86, linux-kernel



On 9/15/26 12:36, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Use a __free() cleanup helper to automatically release all reserved PCI
> config space regions on error exit paths.
> 
> Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
> Signed-off-by: yolezz <yolezz.secret@gmail.com>
> ---
> v3:
>   - Squash changes into a single standalone patch to fix multi-patch confusion.
> 
>   arch/x86/kernel/amd_node.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index b7926ba3610a..20451f0ad8c7 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -239,6 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>   	return root;
>   }
>   
> +static void amd_smn_release_config_regions(u16 *num_roots)
> +{
> +	struct pci_dev *root __free(pci_dev_put) = NULL;
> +
> +	if (!num_roots)
> +		return;
> +
> +	while (*num_roots && (root = get_next_root(root))) {
> +		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
> +		(*num_roots)--;
> +	}
> +}
> +
> +DEFINE_FREE(amd_smn_release_config_regions, u16 *,
> +	    amd_smn_release_config_regions(_T));
> +
>   static bool enable_dfs;
>   
>   static int __init amd_smn_enable_dfs(char *str)
> @@ -250,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
>   
>   static int __init amd_smn_init(void)
>   {
> -	u16 count, num_roots, roots_per_node, node, num_nodes;
> +	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
>   	struct pci_dev *root __free(pci_dev_put) = NULL;
>   
> +	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
> +
>   	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
>   		return 0;
>   
> @@ -261,7 +279,8 @@ static int __init amd_smn_init(void)
>   	if (amd_roots)
>   		return 0;
>   
> -	num_roots = 0;
> +	config_regions = &num_roots;
> +
>   	while ((root = get_next_root(root))) {
>   		pci_dbg(root, "Reserving PCI config space\n");
>   
> @@ -286,8 +305,9 @@ static int __init amd_smn_init(void)
>   
>   	num_nodes = amd_num_nodes();
>   	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
> -	if (!amd_roots)
> +	if (!amd_roots) {
>   		return -ENOMEM;
> +	}

Unnecessary braces

>   
>   	roots_per_node = num_roots / num_nodes;
>   	if (!roots_per_node) {
> @@ -315,6 +335,7 @@ static int __init amd_smn_init(void)
>   		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
>   	}
>   
> +	config_regions = NULL;
>   	return 0;
>   }
>   


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4] x86/amd/node: Release reserved config regions on init error
  2026-09-15 16:35 ` Mario Limonciello
  2026-09-15 17:23   ` [PATCH v2] " yolezz
  2026-09-15 17:36   ` [PATCH v3] " yolezz
@ 2026-09-15 18:05   ` yolezz
  2026-09-15 19:07     ` Mario Limonciello
                       ` (2 more replies)
  2 siblings, 3 replies; 10+ messages in thread
From: yolezz @ 2026-09-15 18:05 UTC (permalink / raw)
  To: mario.limonciello, yazen.ghannam; +Cc: x86, linux-kernel, yolezz

In amd_smn_init(), if pci_request_config_region_exclusive() fails or
if kzalloc_objs() fails to allocate memory for amd_roots, the already
reserved PCI config regions are left allocated.

Use a __free() cleanup helper to automatically release all reserved PCI
config space regions on error exit paths.

Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
Signed-off-by: yolezz <yolezz.secret@gmail.com>
---
v4:
 - Remove unnecessary braces around the kzalloc_objs() error check (as suggested by Yazen Ghannam).

 arch/x86/kernel/amd_node.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index b7926ba3610a..a1db03d3777b 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -239,6 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
 	return root;
 }
 
+static void amd_smn_release_config_regions(u16 *num_roots)
+{
+	struct pci_dev *root __free(pci_dev_put) = NULL;
+
+	if (!num_roots)
+		return;
+
+	while (*num_roots && (root = get_next_root(root))) {
+		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
+		(*num_roots)--;
+	}
+}
+
+DEFINE_FREE(amd_smn_release_config_regions, u16 *,
+	    amd_smn_release_config_regions(_T));
+
 static bool enable_dfs;
 
 static int __init amd_smn_enable_dfs(char *str)
@@ -250,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
 
 static int __init amd_smn_init(void)
 {
-	u16 count, num_roots, roots_per_node, node, num_nodes;
+	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
 	struct pci_dev *root __free(pci_dev_put) = NULL;
 
+	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
+
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
 		return 0;
 
@@ -261,7 +279,8 @@ static int __init amd_smn_init(void)
 	if (amd_roots)
 		return 0;
 
-	num_roots = 0;
+	config_regions = &num_roots;
+
 	while ((root = get_next_root(root))) {
 		pci_dbg(root, "Reserving PCI config space\n");
 
@@ -315,6 +334,7 @@ static int __init amd_smn_init(void)
 		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
 	}
 
+	config_regions = NULL;
 	return 0;
 }
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] x86/amd/node: Release reserved config regions on init error
  2026-09-15 18:05   ` [PATCH v4] " yolezz
@ 2026-09-15 19:07     ` Mario Limonciello
  2026-09-15 20:54     ` Borislav Petkov
  2026-09-15 22:20     ` Yazen Ghannam
  2 siblings, 0 replies; 10+ messages in thread
From: Mario Limonciello @ 2026-09-15 19:07 UTC (permalink / raw)
  To: yolezz, yazen.ghannam; +Cc: x86, linux-kernel



On 9/15/26 13:05, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Use a __free() cleanup helper to automatically release all reserved PCI
> config space regions on error exit paths.
> 
> Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
> Signed-off-by: yolezz <yolezz.secret@gmail.com>
> ---
> v4:
>   - Remove unnecessary braces around the kzalloc_objs() error check (as suggested by Yazen Ghannam).
I have no more concerns, thanks.

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>>
>   arch/x86/kernel/amd_node.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index b7926ba3610a..a1db03d3777b 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -239,6 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>   	return root;
>   }
>   
> +static void amd_smn_release_config_regions(u16 *num_roots)
> +{
> +	struct pci_dev *root __free(pci_dev_put) = NULL;
> +
> +	if (!num_roots)
> +		return;
> +
> +	while (*num_roots && (root = get_next_root(root))) {
> +		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
> +		(*num_roots)--;
> +	}
> +}
> +
> +DEFINE_FREE(amd_smn_release_config_regions, u16 *,
> +	    amd_smn_release_config_regions(_T));
> +
>   static bool enable_dfs;
>   
>   static int __init amd_smn_enable_dfs(char *str)
> @@ -250,9 +266,11 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
>   
>   static int __init amd_smn_init(void)
>   {
> -	u16 count, num_roots, roots_per_node, node, num_nodes;
> +	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
>   	struct pci_dev *root __free(pci_dev_put) = NULL;
>   
> +	u16 *config_regions __free(amd_smn_release_config_regions) = NULL;
> +
>   	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
>   		return 0;
>   
> @@ -261,7 +279,8 @@ static int __init amd_smn_init(void)
>   	if (amd_roots)
>   		return 0;
>   
> -	num_roots = 0;
> +	config_regions = &num_roots;
> +
>   	while ((root = get_next_root(root))) {
>   		pci_dbg(root, "Reserving PCI config space\n");
>   
> @@ -315,6 +334,7 @@ static int __init amd_smn_init(void)
>   		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
>   	}
>   
> +	config_regions = NULL;
>   	return 0;
>   }
>   


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] x86/amd/node: Release reserved config regions on init error
  2026-09-15 18:05   ` [PATCH v4] " yolezz
  2026-09-15 19:07     ` Mario Limonciello
@ 2026-09-15 20:54     ` Borislav Petkov
  2026-09-15 22:20     ` Yazen Ghannam
  2 siblings, 0 replies; 10+ messages in thread
From: Borislav Petkov @ 2026-09-15 20:54 UTC (permalink / raw)
  To: yolezz; +Cc: mario.limonciello, yazen.ghannam, x86, linux-kernel

I do have concerns:

On Tue, Sep 15, 2026 at 08:05:43PM +0200, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Use a __free() cleanup helper to automatically release all reserved PCI
> config space regions on error exit paths.
> 
> Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
> Signed-off-by: yolezz <yolezz.secret@gmail.com>

You need to use your legal name when you sign off on and author patches.

>  arch/x86/kernel/amd_node.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index b7926ba3610a..a1db03d3777b 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -239,6 +239,22 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>  	return root;
>  }
>  
> +static void amd_smn_release_config_regions(u16 *num_roots)
> +{
> +	struct pci_dev *root __free(pci_dev_put) = NULL;
> +
> +	if (!num_roots)
> +		return;
> +
> +	while (*num_roots && (root = get_next_root(root))) {
> +		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
> +		(*num_roots)--;
> +	}
> +}
> +
> +DEFINE_FREE(amd_smn_release_config_regions, u16 *,
> +	    amd_smn_release_config_regions(_T));
> +

The fact that you have to wrangle a solution like this just so that you can
use those fancy __free() gunk should both y'all perhaps give you a hint that
not everything is a nail.

The proper fix is to unwind any setup the function has done in reverse order
by jumping to error labels each time it encounters an error. The good old
design pattern that has been used in the kernel for decades.

This way you don't need a separate release function along with a dummy
config_regions crap which is just ugly.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] x86/amd/node: Release reserved config regions on init error
  2026-09-15 18:05   ` [PATCH v4] " yolezz
  2026-09-15 19:07     ` Mario Limonciello
  2026-09-15 20:54     ` Borislav Petkov
@ 2026-09-15 22:20     ` Yazen Ghannam
  2 siblings, 0 replies; 10+ messages in thread
From: Yazen Ghannam @ 2026-09-15 22:20 UTC (permalink / raw)
  To: yolezz; +Cc: mario.limonciello, x86, linux-kernel

On Tue, Sep 15, 2026 at 08:05:43PM +0200, yolezz wrote:
> In amd_smn_init(), if pci_request_config_region_exclusive() fails or
> if kzalloc_objs() fails to allocate memory for amd_roots, the already
> reserved PCI config regions are left allocated.
> 
> Use a __free() cleanup helper to automatically release all reserved PCI
> config space regions on error exit paths.
> 

What's wrong with leaving these reserved?

> Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
> Signed-off-by: yolezz <yolezz.secret@gmail.com>
> ---
> v4:
>  - Remove unnecessary braces around the kzalloc_objs() error check (as suggested by Yazen Ghannam).
> 

I didn't suggest this.

Thanks,
Yazen

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-09-15 22:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-15 16:22 [PATCH] x86/amd/node: Release reserved config regions on init error yolezz
2026-09-15 16:35 ` Mario Limonciello
2026-09-15 17:23   ` [PATCH v2] " yolezz
2026-09-15 17:32     ` Mario Limonciello
2026-09-15 17:36   ` [PATCH v3] " yolezz
2026-09-15 17:42     ` Mario Limonciello
2026-09-15 18:05   ` [PATCH v4] " yolezz
2026-09-15 19:07     ` Mario Limonciello
2026-09-15 20:54     ` Borislav Petkov
2026-09-15 22:20     ` Yazen Ghannam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®