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

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

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++;