Re: [PATCH v7 3/4] driver core: platform: unify release path

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Jul 13, 2026 at 5:11 PM CEST, Bartosz Golaszewski wrote:
> @@ -599,6 +599,7 @@ static void platform_device_release(struct device *dev)
>  	struct platform_object *pa = container_of(dev, struct platform_object,
>  						  pdev.dev);
>  
> +	device_remove_software_node(dev);
>  	fwnode_handle_put(pa->pdev.dev.fwnode);

This technically changes the semantics of platform_device_set_fwnode():

Previously it took an additional reference count for the fwnode pointer passed
to it. But now, *iff* the fwnode is a software node it consumes the callers
reference count, which for obvious reasons isn't great.

Now, platform_device_set_fwnode() is unused now, so we could just get rid of it,
which also gets us rid of its asymmetric semantics.

However, couldn't we also just move the if (is_software_node(pdevinfo->fwnode))
conditional up here, such that we have:

	device_remove_software_node(dev);
	if (!is_software_node(dev->fwnode))
	    fwnode_handle_put(pa->pdev.dev.fwnode);

This way, the swnode special case would go away. Well, at least the "two
reference counts" special case.

Another special case I just noticed still remains, but is independent of this
change:

If platform_device_set_fwnode() or platform_device_set_of_node() is called for a
device that already has a swnode set, we only call fwnode_handle_put(), but
software_node_notify_remove() etc. isn't called.

Of course that never happens, but it is an inconsistency in the API. Now, it
seems that neither platform_device_set_fwnode() (which we should remove anyway),
nor platform_device_set_of_node() is ever called with a fwnode already set.

So, either we have to special case platform_device_set_of_node() too (for the
case that a swnode is set already), or just require that the function must only
be called when no node has been set, as all users already do; I'd go for the
latter.

If my proposal about moving the is_software_node() check into
platform_device_release() holds, it would probably be good to just add those
patches in a subsequent version, otherwise I'm happy to pull this in as is and
address the other stuff subsequently.

>  	kfree(pa->pdev.dev.platform_data);
>  	kfree(pa->pdev.mfd_cell);
> @@ -606,12 +607,6 @@ static void platform_device_release(struct device *dev)
>  	kfree(pa);
>  }
>  
> -static void platform_device_release_full(struct device *dev)
> -{
> -	device_remove_software_node(dev);
> -	platform_device_release(dev);
> -}
> -
>  /**
>   * platform_device_alloc - create a platform device
>   * @name: base name of the device we're adding
> @@ -933,6 +928,16 @@ struct platform_device *platform_device_register_full(const struct platform_devi
>  		pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
>  	}
>  
> +	/*
> +	 * If the primary firmware node is a software node and there's no
> +	 * secondary firmware node, the primary will be affected by the call
> +	 * to device_remove_software_node() in platform_device_release() and
> +	 * its reference count will be dropped by one. Take another reference
> +	 * here to make it have no effect.
> +	 */
> +	if (is_software_node(pdevinfo->fwnode))
> +		fwnode_handle_get(pdevinfo->fwnode);
> +
>  	ret = platform_device_add_resources(pdev, pdevinfo->res, pdevinfo->num_res);
>  	if (ret)
>  		goto err;
> @@ -945,8 +950,6 @@ struct platform_device *platform_device_register_full(const struct platform_devi
>  		ret = device_add_software_node(&pdev->dev, pdevinfo->swnode);
>  		if (ret)
>  			goto err;
> -
> -		pdev->dev.release = platform_device_release_full;
>  	} else if (pdevinfo->properties) {
>  		ret = device_create_managed_software_node(&pdev->dev,
>  							  pdevinfo->properties, NULL);
>
> -- 
> 2.47.3