Re: [PATCHv4 1/6] spi: add ancillary device support

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Wed, Jun 09, 2021 at 05:12:30PM +0200, Sebastian Reichel wrote:
> Introduce support for ancillary devices, similar to existing
> implementation for I2C. This is useful for devices having
> multiple chip-selects, for example some microcontrollers
> provide a normal SPI interface and a flashing SPI interface.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
>  drivers/spi/spi.c       | 139 +++++++++++++++++++++++++++++++---------
>  include/linux/spi/spi.h |   2 +
>  2 files changed, 109 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index ba425b9c7700..7fdf224262b1 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -558,49 +558,23 @@ static int spi_dev_check(struct device *dev, void *data)
>  	return 0;
>  }
>  
> -/**
> - * spi_add_device - Add spi_device allocated with spi_alloc_device
> - * @spi: spi_device to register
> - *
> - * Companion function to spi_alloc_device.  Devices allocated with
> - * spi_alloc_device can be added onto the spi bus with this function.
> - *
> - * Return: 0 on success; negative errno on failure
> - */
> -int spi_add_device(struct spi_device *spi)
> +static int __spi_add_device(struct spi_device *spi)
>  {
>  	struct spi_controller *ctlr = spi->controller;
>  	struct device *dev = ctlr->dev.parent;
>  	int status;
>  
> -	/* Chipselects are numbered 0..max; validate. */
> -	if (spi->chip_select >= ctlr->num_chipselect) {
> -		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
> -			ctlr->num_chipselect);
> -		return -EINVAL;
> -	}
> -
> -	/* Set the bus ID string */
> -	spi_dev_set_name(spi);
> -
> -	/* We need to make sure there's no other device with this
> -	 * chipselect **BEFORE** we call setup(), else we'll trash
> -	 * its configuration.  Lock against concurrent add() calls.
> -	 */
> -	mutex_lock(&spi_add_lock);
> -
>  	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
>  	if (status) {
>  		dev_err(dev, "chipselect %d already in use\n",
>  				spi->chip_select);
> -		goto done;
> +		return status;
>  	}
>  
>  	/* Controller may unregister concurrently */
>  	if (IS_ENABLED(CONFIG_SPI_DYNAMIC) &&
>  	    !device_is_registered(&ctlr->dev)) {
> -		status = -ENODEV;
> -		goto done;
> +		return -ENODEV;
>  	}
>  
>  	/* Descriptors take precedence */
> @@ -617,7 +591,7 @@ int spi_add_device(struct spi_device *spi)
>  	if (status < 0) {
>  		dev_err(dev, "can't setup %s, status %d\n",
>  				dev_name(&spi->dev), status);
> -		goto done;
> +		return status;
>  	}
>  
>  	/* Device may be bound to an active driver when this returns */
> @@ -628,12 +602,64 @@ int spi_add_device(struct spi_device *spi)
>  	else
>  		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
>  
> -done:
> +	return status;
> +}
> +
> +/**
> + * spi_add_device - Add spi_device allocated with spi_alloc_device
> + * @spi: spi_device to register
> + *
> + * Companion function to spi_alloc_device.  Devices allocated with
> + * spi_alloc_device can be added onto the spi bus with this function.
> + *
> + * Return: 0 on success; negative errno on failure
> + */
> +int spi_add_device(struct spi_device *spi)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +	struct device *dev = ctlr->dev.parent;
> +	int status;
> +
> +	/* Chipselects are numbered 0..max; validate. */
> +	if (spi->chip_select >= ctlr->num_chipselect) {
> +		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
> +			ctlr->num_chipselect);
> +		return -EINVAL;
> +	}
> +
> +	/* Set the bus ID string */
> +	spi_dev_set_name(spi);
> +
> +	/* We need to make sure there's no other device with this
> +	 * chipselect **BEFORE** we call setup(), else we'll trash
> +	 * its configuration.  Lock against concurrent add() calls.
> +	 */
> +	mutex_lock(&spi_add_lock);
> +	status = __spi_add_device(spi);
>  	mutex_unlock(&spi_add_lock);
>  	return status;
>  }
>  EXPORT_SYMBOL_GPL(spi_add_device);
>  
> +static int spi_add_device_locked(struct spi_device *spi)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +	struct device *dev = ctlr->dev.parent;
> +
> +	/* Chipselects are numbered 0..max; validate. */
> +	if (spi->chip_select >= ctlr->num_chipselect) {
> +		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
> +			ctlr->num_chipselect);
> +		return -EINVAL;
> +	}
> +
> +	/* Set the bus ID string */
> +	spi_dev_set_name(spi);
> +
> +	WARN_ON(!mutex_is_locked(&spi_add_lock));

So you just rebooted a machine that has panic-on-warn set.  Not nice.

If this really can happen, test for it and recover, do not reboot
devices.

If this really can never happen, why are you testing for it?

thanks,

greg k-h