Re: [PATCH] spi: virtio: move teardown to .remove()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] spi: virtio: virtqueue kick before DRIVER_OK in
  virtio_spi_restore() [1]
--

commit 30a7d01f20f6ab641d59ca6ce3ddf0209bd0408f
Author: Francesco Valla <francesco@valla.it>

spi: virtio: move teardown to .remove()

This commit replaces devm-managed controller registration and virtqueue
cleanup with an explicit .remove() callback so the virtio device is reset
before the virtio core checks its status during teardown.

> diff --git a/drivers/spi/spi-virtio.c b/drivers/spi/spi-virtio.c
> index 26f9711c99b39..2eeb79ff5aa26 100644
> --- a/drivers/spi/spi-virtio.c
> +++ b/drivers/spi/spi-virtio.c
[ ... ]
> @@ -347,21 +345,26 @@ static int virtio_spi_probe(struct virtio_device *vdev)
>  	if (ret)
>  		return dev_err_probe(&vdev->dev, ret, "Cannot setup virtqueues\n");
>  
> -	/* Register cleanup for virtqueues using devm */
> -	ret = devm_add_action_or_reset(&vdev->dev, virtio_spi_del_vq, vdev);
> -	if (ret)
> -		return dev_err_probe(&vdev->dev, ret, "Cannot register virtqueue cleanup\n");
> -
>  	virtio_device_ready(vdev);
>  
> -	/* Use devm version to register controller */
> -	ret = devm_spi_register_controller(&vdev->dev, ctrl);
> -	if (ret)
> +	ret = spi_register_controller(ctrl);
> +	if (ret) {
> +		virtio_spi_del_vq(vdev);
>  		return dev_err_probe(&vdev->dev, ret, "Cannot register controller\n");
> +	}
>  
>  	return 0;
>  }
>  
> +static void virtio_spi_remove(struct virtio_device *vdev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
> +
> +	spi_unregister_controller(ctrl);
> +
> +	virtio_spi_del_vq(vdev);
> +}
> +
>  static int virtio_spi_freeze(struct device *dev)
>  {
>  	struct spi_controller *ctrl = dev_get_drvdata(dev);

[Severity: High]
This problem wasn't introduced by this patch, but can virtio_spi_restore()
kick the virtqueue before VIRTIO_CONFIG_S_DRIVER_OK is set [1]?

drivers/spi/spi-virtio.c:virtio_spi_restore() {
	...
	ret = virtio_spi_find_vqs(vdev->priv);
	if (ret) {
		dev_err(dev, "problem starting vqueue (%d)\n", ret);
		return ret;
	}

	ret = spi_controller_resume(ctrl);
	...
}

Unlike virtio_spi_probe(), which calls virtio_device_ready() before
exposing the controller, virtio_spi_restore() calls
spi_controller_resume() without marking the device ready first:

CPU0                              CPU1 (kworker)
virtio_spi_restore()
  virtio_spi_find_vqs()
  spi_controller_resume()
                                  virtio_spi_transfer_one()
                                    virtqueue_kick()  <-- before DRIVER_OK
  virtio_device_ready()

Unblocking the SPI message pump wakes the kworker to process queued
transfers and kick the virtqueue before the resume path returns to set
DRIVER_OK, violating the virtio specification and risking dropped SPI
transfers or a broken device state on resume. Calling
virtio_device_ready(vdev) before spi_controller_resume(ctrl) would avoid
this.

[1] https://sashiko.dev/#/bug/linux-82cf02b8-4add-4c6f-b6da-ae25c86bd2bf

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-spi_virtio_remove-v1-1-fda5f0996290@valla.it?part=1