Re: [PATCH] spi: Fix spi device unregister flow

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

+Cc Lukas

On Tue, Apr 27, 2021 at 2:56 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> When an SPI device is unregistered, the spi->controller->cleanup() is
> called in the device's release callback. That's wrong for a couple of
> reasons:
>
> 1. spi_dev_put() can be called before spi_add_device() is called. And
>    it's spi_add_device() that calls spi_setup(). This will cause clean()
>    to get called without the spi device ever being setup.
>
> 2. There's no guarantee that the controller's driver would be present by
>    the time the spi device's release function gets called.
>
> 3. It also causes "sleeping in atomic context" stack dump[1] when device
>    link deletion code does a put_device() on the spi device.
>
> Fix these issues by simply moving the cleanup from the device release
> callback to the actual spi_unregister_device() function.
>
> [1] - https://lore.kernel.org/lkml/CAHp75Vc=FCGcUyS0v6fnxme2YJ+qD+Y-hQDQLa2JhWNON9VmsQ@xxxxxxxxxxxxxx/
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
>  drivers/spi/spi.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b08efe88ccd6..7d0d89172a1d 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -47,10 +47,6 @@ static void spidev_release(struct device *dev)
>  {
>         struct spi_device       *spi = to_spi_device(dev);
>
> -       /* spi controllers may cleanup for released devices */
> -       if (spi->controller->cleanup)
> -               spi->controller->cleanup(spi);
> -
>         spi_controller_put(spi->controller);
>         kfree(spi->driver_override);
>         kfree(spi);
> @@ -558,6 +554,12 @@ static int spi_dev_check(struct device *dev, void *data)
>         return 0;
>  }
>
> +static void spi_cleanup(struct spi_device *spi)
> +{
> +       if (spi->controller->cleanup)
> +               spi->controller->cleanup(spi);
> +}
> +
>  /**
>   * spi_add_device - Add spi_device allocated with spi_alloc_device
>   * @spi: spi_device to register
> @@ -622,11 +624,13 @@ int spi_add_device(struct spi_device *spi)
>
>         /* Device may be bound to an active driver when this returns */
>         status = device_add(&spi->dev);
> -       if (status < 0)
> +       if (status < 0) {
>                 dev_err(dev, "can't add %s, status %d\n",
>                                 dev_name(&spi->dev), status);
> -       else
> +               spi_cleanup(spi);
> +       } else {
>                 dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
> +       }
>
>  done:
>         mutex_unlock(&spi_add_lock);
> @@ -713,6 +717,8 @@ void spi_unregister_device(struct spi_device *spi)
>         if (!spi)
>                 return;
>
> +       spi_cleanup(spi);
> +
>         if (spi->dev.of_node) {
>                 of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
>                 of_node_put(spi->dev.of_node);
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>


-- 
With Best Regards,
Andy Shevchenko