Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

[Rob Herring <robh@kernel.org>]

On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> Formatting strings using "%pOF" while holding devtree_lock causes a
> deadlock.  Lockdep reports:
> 
>     of_get_parent from of_fwnode_get_parent+0x18/0x24
>     ^^^^^^^^^^^^^

I'm wondering if we really need the lock in there. We never unset or 
change the parent. It gets detached, but we're not checking for that. 
The node could get freed, but the lock is not for that, refcounts are.

>     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28

count parents? Huh? Isn't it always 1? 

>     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
>     fwnode_full_name_string from device_node_string+0x1a0/0x404
>     device_node_string from pointer+0x3c0/0x534
>     pointer from vsnprintf+0x248/0x36c
>     vsnprintf from vprintk_store+0x130/0x3b4
> 
> Fix this by making the locking cover only the parts that really need it.
> 
> Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/of/dynamic.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index e311d406b1705306..eae45a1c673ee05f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  
>  	__of_changeset_entry_dump(ce);
>  
> -	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	switch (ce->action) {
>  	case OF_RECONFIG_ATTACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_attach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);

I think you could just move the spinlock into __of_attach_node(). The 
only other caller looks like this.

>  		break;
>  	case OF_RECONFIG_DETACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_detach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		break;
>  	case OF_RECONFIG_ADD_PROPERTY:
>  		/* If the property is in deadprops then it must be removed */
> @@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_add_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: add_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  		}
>  		break;
>  	case OF_RECONFIG_REMOVE_PROPERTY:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_remove_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: remove_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_update_property(ce->np, ce->prop, &old_prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: update_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  	default:
>  		ret = -EINVAL;
>  	}
> -	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	if (ret)
>  		return ret;
> -- 
> 2.34.1
>