Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Cornelia Huck <cohuck@redhat.com>
To:     Jason Wang <jasowang@redhat.com>, jasowang@redhat.com,
        mst@redhat.com, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org
Cc:     sgarzare@redhat.com, eperezma@redhat.com, lulu@redhat.com,
        tglx@linutronix.de, peterz@infradead.org, paulmck@kernel.org,
        maz@kernel.org, pasic@linux.ibm.com
Subject: Re: [PATCH V3 7/9] virtio: allow to unbreak virtqueue
In-Reply-To: <20220425024418.8415-8-jasowang@redhat.com>
Organization: Red Hat GmbH
References: <20220425024418.8415-1-jasowang@redhat.com>
 <20220425024418.8415-8-jasowang@redhat.com>
User-Agent: Notmuch/0.34 (https://notmuchmail.org)
Date:   Mon, 25 Apr 2022 14:44:06 +0200
Message-ID: <87v8ux72p5.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Mon, Apr 25 2022, Jason Wang <jasowang@redhat.com> wrote:

> This patch allows the virtio_break_device() to accept a boolean value
> then we can unbreak the virtqueue.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/char/virtio_console.c              | 2 +-
>  drivers/crypto/virtio/virtio_crypto_core.c | 2 +-
>  drivers/s390/virtio/virtio_ccw.c           | 4 ++--
>  drivers/virtio/virtio_pci_common.c         | 2 +-
>  drivers/virtio/virtio_ring.c               | 4 ++--
>  include/linux/virtio.h                     | 2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e3c430539a17..afede977f7b3 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1958,7 +1958,7 @@ static void virtcons_remove(struct virtio_device *vdev)
>  	spin_unlock_irq(&pdrvdata_lock);
>  
>  	/* Device is going away, exit any polling for buffers */
> -	virtio_break_device(vdev);
> +	virtio_break_device(vdev, true);
>  	if (use_multiport(portdev))
>  		flush_work(&portdev->control_work);
>  	else
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
> index c6f482db0bc0..fd17f3f2e958 100644
> --- a/drivers/crypto/virtio/virtio_crypto_core.c
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -215,7 +215,7 @@ static int virtcrypto_update_status(struct virtio_crypto *vcrypto)
>  		dev_warn(&vcrypto->vdev->dev,
>  				"Unknown status bits: 0x%x\n", status);
>  
> -		virtio_break_device(vcrypto->vdev);
> +		virtio_break_device(vcrypto->vdev, true);
>  		return -EPERM;
>  	}
>  
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index c19f07a82d62..9a963f5af5b5 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1211,7 +1211,7 @@ static void virtio_ccw_remove(struct ccw_device *cdev)
>  
>  	if (vcdev && cdev->online) {
>  		if (vcdev->device_lost)
> -			virtio_break_device(&vcdev->vdev);
> +			virtio_break_device(&vcdev->vdev, true);
>  		unregister_virtio_device(&vcdev->vdev);
>  		spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
>  		dev_set_drvdata(&cdev->dev, NULL);
> @@ -1228,7 +1228,7 @@ static int virtio_ccw_offline(struct ccw_device *cdev)
>  	if (!vcdev)
>  		return 0;
>  	if (vcdev->device_lost)
> -		virtio_break_device(&vcdev->vdev);
> +		virtio_break_device(&vcdev->vdev, true);
>  	unregister_virtio_device(&vcdev->vdev);
>  	spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
>  	dev_set_drvdata(&cdev->dev, NULL);
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index d724f676608b..39a711ddff30 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -583,7 +583,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	 * layers can abort any ongoing operation.
>  	 */
>  	if (!pci_device_is_present(pci_dev))
> -		virtio_break_device(&vp_dev->vdev);
> +		virtio_break_device(&vp_dev->vdev, true);
>  
>  	pci_disable_sriov(pci_dev);
>  
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cfb028ca238e..6da13495a70c 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2382,7 +2382,7 @@ EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>   * This should prevent the device from being used, allowing drivers to
>   * recover.  You may need to grab appropriate locks to flush.
>   */
> -void virtio_break_device(struct virtio_device *dev)
> +void virtio_break_device(struct virtio_device *dev, bool broken)

I think we need to be careful to say when it is safe to unset 'broken'.

The current callers set all queues to broken in case of surprise removal
(ccw, pci), removal (console), or the device behaving badly
(crypto). There's also code setting individual queues to broken. We do
not want to undo any of these, unless the device has gone through a
reset in the meanwhile. Maybe add:

"It is only safe to call this function to *remove* the broken flag for a
device that is (re)transitioning to becoming usable; calling it that way
during normal usage may have unpredictable consequences."

(Not sure how to word this; especially if we consider future usage of
queue reset.)