[PATCH] pmdomains: mtk-pm-domains: improve spinlock recursion fix in probe

[PATCH] pmdomains: mtk-pm-domains: improve spinlock recursion fix in probe

[Macpaul Lin]

Remove scpsys_get_legacy_regmap() and update usages with
of_find_node_with_property(). Use an explicit of_node_get(np) to ensure
correct node referencing against of_node_put() and ensuring it is called
in a safe context (i.e., not while holding devtree_lock).

If fwnode_count_parents() obtains parent nodes via
fwnode_for_each_parent_node() and this process requires device tree
operations, it may result in repeated acquisition of devtree_lock in
the same thread/context, leading to spinlock recursion errors.

Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 80561d27f2b2..f64f24d520dd 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -984,18 +984,6 @@ static void scpsys_domain_cleanup(struct scpsys *scpsys)
 	}
 }
 
-static struct device_node *scpsys_get_legacy_regmap(struct device_node *np, const char *pn)
-{
-	struct device_node *local_node;
-
-	for_each_child_of_node(np, local_node) {
-		if (of_property_present(local_node, pn))
-			return local_node;
-	}
-
-	return NULL;
-}
-
 static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *scpsys)
 {
 	const u8 bp_blocks[3] = {
@@ -1017,7 +1005,8 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 	 * this makes it then possible to allocate the array of bus_prot
 	 * regmaps and convert all to the new style handling.
 	 */
-	node = scpsys_get_legacy_regmap(np, "mediatek,infracfg");
+	of_node_get(np);
+	node = of_find_node_with_property(np, "mediatek,infracfg");
 	if (node) {
 		regmap[0] = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg");
 		of_node_put(node);
@@ -1030,7 +1019,8 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 		regmap[0] = NULL;
 	}
 
-	node = scpsys_get_legacy_regmap(np, "mediatek,smi");
+	of_node_get(np);
+	node = of_find_node_with_property(np, "mediatek,smi");
 	if (node) {
 		smi_np = of_parse_phandle(node, "mediatek,smi", 0);
 		of_node_put(node);
@@ -1048,7 +1038,8 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 		regmap[1] = NULL;
 	}
 
-	node = scpsys_get_legacy_regmap(np, "mediatek,infracfg-nao");
+	of_node_get(np);
+	node = of_find_node_with_property(np, "mediatek,infracfg-nao");
 	if (node) {
 		regmap[2] = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg-nao");
 		num_regmaps++;
-- 
2.45.2

Re: [PATCH] pmdomains: mtk-pm-domains: improve spinlock recursion fix in probe

[Louis-Alexis Eyraud]

Hi Macpaul,

On Tue, 2025-11-25 at 18:56 +0800, Macpaul Lin wrote:
> Remove scpsys_get_legacy_regmap() and update usages with
> of_find_node_with_property(). Use an explicit of_node_get(np) to
> ensure
> correct node referencing against of_node_put() and ensuring it is
> called
> in a safe context (i.e., not while holding devtree_lock).
> 
> If fwnode_count_parents() obtains parent nodes via
> fwnode_for_each_parent_node() and this process requires device tree
> operations, it may result in repeated acquisition of devtree_lock in
> the same thread/context, leading to spinlock recursion errors.
> 
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
>  drivers/pmdomain/mediatek/mtk-pm-domains.c | 21 ++++++--------------
> -
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 80561d27f2b2..f64f24d520dd 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -984,18 +984,6 @@ static void scpsys_domain_cleanup(struct scpsys
> *scpsys)
>  	}
>  }
>  
> -static struct device_node *scpsys_get_legacy_regmap(struct
> device_node *np, const char *pn)
> -{
> -	struct device_node *local_node;
> -
> -	for_each_child_of_node(np, local_node) {
> -		if (of_property_present(local_node, pn))
> -			return local_node;
> -	}
> -
> -	return NULL;
> -}
> -
>  static int scpsys_get_bus_protection_legacy(struct device *dev,
> struct scpsys *scpsys)
>  {
>  	const u8 bp_blocks[3] = {
> @@ -1017,7 +1005,8 @@ static int
> scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>  	 * this makes it then possible to allocate the array of
> bus_prot
>  	 * regmaps and convert all to the new style handling.
>  	 */
> -	node = scpsys_get_legacy_regmap(np, "mediatek,infracfg");
> +	of_node_get(np);
> +	node = of_find_node_with_property(np, "mediatek,infracfg");

with a kernel based on next-20251125 plus this patch, my Mediatek Genio
350, 510 and 1200 EVK boards booted OK.

About the patch itself, it looks that you try to balance the
of_node_put() done by of_find_node_with_property() on its `from`
parameter (np in this case): 
https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/of/base.c#L1066

Your patch description is a bit unclear about that and could be
reworded.

I'm not sure if it is the proper way to use the
of_find_node_with_property API, seems a bit hacky. Though, I found 
similar sequences for other of_find_* calls (like
of_find_compatible_node or of_find_node_by_name) in the `from is not
null` case but not all the times. Hope someone else can confirm if
using of_node_get before calling these API is OK.

Finally the patch also misses the Fixes tag since it is a follow up
one.

Regards,
Louis-Alexis

>  	if (node) {
>  		regmap[0] = syscon_regmap_lookup_by_phandle(node,
> "mediatek,infracfg");
>  		of_node_put(node);
> @@ -1030,7 +1019,8 @@ static int
> scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>  		regmap[0] = NULL;
>  	}
>  
> -	node = scpsys_get_legacy_regmap(np, "mediatek,smi");
> +	of_node_get(np);
> +	node = of_find_node_with_property(np, "mediatek,smi");
>  	if (node) {
>  		smi_np = of_parse_phandle(node, "mediatek,smi", 0);
>  		of_node_put(node);
> @@ -1048,7 +1038,8 @@ static int
> scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>  		regmap[1] = NULL;
>  	}
>  
> -	node = scpsys_get_legacy_regmap(np, "mediatek,infracfg-
> nao");
> +	of_node_get(np);
> +	node = of_find_node_with_property(np, "mediatek,infracfg-
> nao");
>  	if (node) {
>  		regmap[2] = syscon_regmap_lookup_by_phandle(node,
> "mediatek,infracfg-nao");
>  		num_regmaps++;

Re: [PATCH] pmdomains: mtk-pm-domains: improve spinlock recursion fix in probe

[Macpaul Lin (林智斌)]

On Wed, 2025-11-26 at 13:51 +0100, Louis-Alexis Eyraud wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi Macpaul,
> 
> On Tue, 2025-11-25 at 18:56 +0800, Macpaul Lin wrote:
> > Remove scpsys_get_legacy_regmap() and update usages with
> > of_find_node_with_property(). Use an explicit of_node_get(np) to
> > ensure
> > correct node referencing against of_node_put() and ensuring it is
> > called
> > in a safe context (i.e., not while holding devtree_lock).
> > 
> > If fwnode_count_parents() obtains parent nodes via
> > fwnode_for_each_parent_node() and this process requires device tree
> > operations, it may result in repeated acquisition of devtree_lock
> > in
> > the same thread/context, leading to spinlock recursion errors.
> > 
> > Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> > ---
> >  drivers/pmdomain/mediatek/mtk-pm-domains.c | 21 ++++++------------
> > --
> > -
> >  1 file changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > index 80561d27f2b2..f64f24d520dd 100644
> > --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > @@ -984,18 +984,6 @@ static void scpsys_domain_cleanup(struct
> > scpsys
> > *scpsys)
> >       }
> >  }
> > 
> > -static struct device_node *scpsys_get_legacy_regmap(struct
> > device_node *np, const char *pn)
> > -{
> > -     struct device_node *local_node;
> > -
> > -     for_each_child_of_node(np, local_node) {
> > -             if (of_property_present(local_node, pn))
> > -                     return local_node;
> > -     }
> > -
> > -     return NULL;
> > -}
> > -
> >  static int scpsys_get_bus_protection_legacy(struct device *dev,
> > struct scpsys *scpsys)
> >  {
> >       const u8 bp_blocks[3] = {
> > @@ -1017,7 +1005,8 @@ static int
> > scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys
> > *s
> >        * this makes it then possible to allocate the array of
> > bus_prot
> >        * regmaps and convert all to the new style handling.
> >        */
> > -     node = scpsys_get_legacy_regmap(np, "mediatek,infracfg");
> > +     of_node_get(np);
> > +     node = of_find_node_with_property(np, "mediatek,infracfg");
> 
> with a kernel based on next-20251125 plus this patch, my Mediatek
> Genio
> 350, 510 and 1200 EVK boards booted OK.
> 

Thanks for helping the verification work on all platforms.
Well, should I add a Tested-by: tag from you for the next patch?

> About the patch itself, it looks that you try to balance the
> of_node_put() done by of_find_node_with_property() on its `from`
> parameter (np in this case):
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/of/base.c*L1066__;Iw!!CTRNKA9wMg0ARbw!nIEKBNc582OkFUWke5JW66FKC78AzItgTwlefoTjKiyAR8kzJOCcF-hTBwWVw2loY4mG4lY5AKvvBpCNTbXDoHbl8liqB8sS$
> 
> Your patch description is a bit unclear about that and could be
> reworded.

I'll try to refine and see if I could express it more strait forward.

> 
> I'm not sure if it is the proper way to use the
> of_find_node_with_property API, seems a bit hacky. Though, I found
> similar sequences for other of_find_* calls (like
> of_find_compatible_node or of_find_node_by_name) in the `from is not
> null` case but not all the times. Hope someone else can confirm if
> using of_node_get before calling these API is OK.
> 

Yeah. I'm totally agree with you. That's why I didn't add a 'Fixes:'
tag since we already have Angelo's fix in tree and we might be able to
have more discussion on this 'hacky' implementation. To ensure if we
want to migrate to native of_* APIs or keep some legacy functions on
this driver.

> Finally the patch also misses the Fixes tag since it is a follow up
> one.
> 

Okay, I'll add Fixes: tag in the v2. Thanks for review.

> Regards,
> Louis-Alexis
> 
> 

[snip]

Best regards,
Macpaul Lin