Re: [PATCH v4 1/2] spi: add new_device/delete_device sysfs interface

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Mon, May 11, 2026 at 10:40:01AM +0000, Vishwaroop A wrote:

> +static ssize_t
> +new_device_store(struct device *dev, struct device_attribute *attr,
> +		 const char *buf, size_t count)
> +{

> +	/* Controller is dying; clean up if __unregister hasn't already */
> +	if (device_is_registered(&spi->dev))
> +		spi_unregister_device(spi);

This runs outside of userspace_clients_lock so there's a possibility
that if the controller is in the middle of unregistering (which is
likely since that's the case we're handling!) it might be running

	device_for_each_child(&ctlr->dev, NULL, __unregister);

which would result in a double free.

> @@ -3633,8 +3813,35 @@ void spi_unregister_controller(struct spi_controller *ctlr)
>  	if (IS_ENABLED(CONFIG_SPI_DYNAMIC))
>  		mutex_lock(&ctlr->add_lock);
>  
> +	/*
> +	 * Mark dead and drain userspace_clients before __unregister,
> +	 * since spi_unregister_device() doesn't do list_del() itself.
> +	 */
> +#if IS_ENABLED(CONFIG_SPI_DYNAMIC)
> +	mutex_lock(&ctlr->userspace_clients_lock);
> +	ctlr->dead = true;
> +	while (!list_empty(&ctlr->userspace_clients)) {
> +		struct spi_device *spi;
> +
> +		spi = list_first_entry(&ctlr->userspace_clients,
> +				       struct spi_device,
> +				       userspace_node);
> +		list_del(&spi->userspace_node);
> +		spi_unregister_device(spi);
> +	}
> +	mutex_unlock(&ctlr->userspace_clients_lock);

The spi_unregister_controller() side does things with the lock.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]