Re: [PATCH 2/2] software node: Fix device_add_software_node()

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 05:30:12PM +0300, Heikki Krogerus wrote:
> The function device_add_software_node() was meant to
> register the node supplied to it, but only if that node
> wasn't already registered. Right now the function attempts
> to always register the node. That will cause a failure with
> nodes that are already registered.
> 
> Fixing that by incrementing the reference count of the nodes
> that have already been registered, and only registering the
> new nodes. Also, clarifying the behaviour in the function
> documentation.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Tested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

(On Intel Galileo Gen 2 with some custom patches to convert gpio-dwapb et al.
  to use swnodes. Those patches a subject to further submission.)

Thanks!

> Fixes: e68d0119e328 ("software node: Introduce device_add_software_node()")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
>  drivers/base/swnode.c    | 26 +++++++++++++++++---------
>  include/linux/property.h |  2 +-
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 74db8c971db74..fa3719ef80e4d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -1005,25 +1005,33 @@ EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
>  /**
>   * device_add_software_node - Assign software node to a device
>   * @dev: The device the software node is meant for.
> - * @swnode: The software node.
> + * @node: The software node.
>   *
> - * This function will register @swnode and make it the secondary firmware node
> - * pointer of @dev. If @dev has no primary node, then @swnode will become the primary
> - * node.
> + * This function will make @node the secondary firmware node pointer of @dev. If
> + * @dev has no primary node, then @node will become the primary node. The
> + * function will register @node automatically if it wasn't already registered.
>   */
> -int device_add_software_node(struct device *dev, const struct software_node *swnode)
> +int device_add_software_node(struct device *dev, const struct software_node *node)
>  {
> +	struct swnode *swnode;
>  	int ret;
>  
>  	/* Only one software node per device. */
>  	if (dev_to_swnode(dev))
>  		return -EBUSY;
>  
> -	ret = software_node_register(swnode);
> -	if (ret)
> -		return ret;
> +	swnode = software_node_to_swnode(node);
> +	if (swnode) {
> +		kobject_get(&swnode->kobj);
> +	} else {
> +		ret = software_node_register(node);
> +		if (ret)
> +			return ret;
> +
> +		swnode = software_node_to_swnode(node);
> +	}
>  
> -	set_secondary_fwnode(dev, software_node_fwnode(swnode));
> +	set_secondary_fwnode(dev, &swnode->fwnode);
>  
>  	return 0;
>  }
> diff --git a/include/linux/property.h b/include/linux/property.h
> index dafccfce02624..dd4687b562393 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -488,7 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent);
>  void fwnode_remove_software_node(struct fwnode_handle *fwnode);
>  
> -int device_add_software_node(struct device *dev, const struct software_node *swnode);
> +int device_add_software_node(struct device *dev, const struct software_node *node);
>  void device_remove_software_node(struct device *dev);
>  
>  int device_create_managed_software_node(struct device *dev,
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko