| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200211142711.GE2363188@phenom.ffwll.local>
Date: Tue, 11 Feb 2020 15:27:11 +0100
From: Daniel Vetter <daniel@...ll.ch>
To: Gerd Hoffmann <kraxel@...hat.com>
Cc: dri-devel@...ts.freedesktop.org, olvaffe@...il.com,
gurchetansingh@...omium.org, David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
"open list:VIRTIO GPU DRIVER"
<virtualization@...ts.linux-foundation.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] drm/virtio: add drm_driver.release callback.
On Tue, Feb 11, 2020 at 02:58:04PM +0100, Gerd Hoffmann wrote:
> Split virtio_gpu_deinit(), move the drm shutdown and release to
> virtio_gpu_release(). Drop vqs_ready variable, instead use
> drm_dev_{enter,exit,unplug} to avoid touching hardware after
> device removal. Tidy up here and there.
>
> v4: add changelog.
> v3: use drm_dev_*().
>
> Signed-off-by: Gerd Hoffmann <kraxel@...hat.com>
Looks reasonable I think.
Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
I didn't review whether you need more drm_dev_enter/exit pairs, virtio is
a bit more complex for that and I have no idea how exactly it works. Maybe
for these more complex drivers we need a drm_dev_assert_entered() or so
that uses the right srcu lockdep annotations to make sure we do this
right. Sprinkling that check into a few low-level hw functions (touching
registers or whatever) should catch most issues.
-Daniel
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 3 ++-
> drivers/gpu/drm/virtio/virtgpu_display.c | 1 -
> drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++++-
> drivers/gpu/drm/virtio/virtgpu_kms.c | 7 ++++--
> drivers/gpu/drm/virtio/virtgpu_vq.c | 27 +++++++++++++-----------
> 5 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 7fd8361e1c9e..af9403e1cf78 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -32,6 +32,7 @@
> #include <linux/virtio_gpu.h>
>
> #include <drm/drm_atomic.h>
> +#include <drm/drm_drv.h>
> #include <drm/drm_encoder.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_gem.h>
> @@ -177,7 +178,6 @@ struct virtio_gpu_device {
> struct virtio_gpu_queue ctrlq;
> struct virtio_gpu_queue cursorq;
> struct kmem_cache *vbufs;
> - bool vqs_ready;
>
> bool disable_notify;
> bool pending_notify;
> @@ -219,6 +219,7 @@ extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
> /* virtio_kms.c */
> int virtio_gpu_init(struct drm_device *dev);
> void virtio_gpu_deinit(struct drm_device *dev);
> +void virtio_gpu_release(struct drm_device *dev);
> int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file);
> void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file *file);
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 7b0f0643bb2d..af953db4a0c9 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -368,6 +368,5 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
>
> for (i = 0 ; i < vgdev->num_scanouts; ++i)
> kfree(vgdev->outputs[i].edid);
> - drm_atomic_helper_shutdown(vgdev->ddev);
> drm_mode_config_cleanup(vgdev->ddev);
> }
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 8cf27af3ad53..ab4bed78e656 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -31,6 +31,7 @@
> #include <linux/pci.h>
>
> #include <drm/drm.h>
> +#include <drm/drm_atomic_helper.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_file.h>
>
> @@ -135,7 +136,8 @@ static void virtio_gpu_remove(struct virtio_device *vdev)
> {
> struct drm_device *dev = vdev->priv;
>
> - drm_dev_unregister(dev);
> + drm_dev_unplug(dev);
> + drm_atomic_helper_shutdown(dev);
> virtio_gpu_deinit(dev);
> drm_dev_put(dev);
> }
> @@ -214,4 +216,6 @@ static struct drm_driver driver = {
> .major = DRIVER_MAJOR,
> .minor = DRIVER_MINOR,
> .patchlevel = DRIVER_PATCHLEVEL,
> +
> + .release = virtio_gpu_release,
> };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index c1086df49816..4009c2f97d08 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -199,7 +199,6 @@ int virtio_gpu_init(struct drm_device *dev)
> virtio_gpu_modeset_init(vgdev);
>
> virtio_device_ready(vgdev->vdev);
> - vgdev->vqs_ready = true;
>
> if (num_capsets)
> virtio_gpu_get_capsets(vgdev, num_capsets);
> @@ -234,12 +233,16 @@ void virtio_gpu_deinit(struct drm_device *dev)
> struct virtio_gpu_device *vgdev = dev->dev_private;
>
> flush_work(&vgdev->obj_free_work);
> - vgdev->vqs_ready = false;
> flush_work(&vgdev->ctrlq.dequeue_work);
> flush_work(&vgdev->cursorq.dequeue_work);
> flush_work(&vgdev->config_changed_work);
> vgdev->vdev->config->reset(vgdev->vdev);
> vgdev->vdev->config->del_vqs(vgdev->vdev);
> +}
> +
> +void virtio_gpu_release(struct drm_device *dev)
> +{
> + struct virtio_gpu_device *vgdev = dev->dev_private;
>
> virtio_gpu_modeset_fini(vgdev);
> virtio_gpu_free_vbufs(vgdev);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index a682c2fcbe9a..cfe9c54f87a3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -330,7 +330,14 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> {
> struct virtqueue *vq = vgdev->ctrlq.vq;
> bool notify = false;
> - int ret;
> + int ret, idx;
> +
> + if (!drm_dev_enter(vgdev->ddev, &idx)) {
> + if (fence && vbuf->objs)
> + virtio_gpu_array_unlock_resv(vbuf->objs);
> + free_vbuf(vgdev, vbuf);
> + return;
> + }
>
> if (vgdev->has_indirect)
> elemcnt = 1;
> @@ -338,14 +345,6 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> again:
> spin_lock(&vgdev->ctrlq.qlock);
>
> - if (!vgdev->vqs_ready) {
> - spin_unlock(&vgdev->ctrlq.qlock);
> -
> - if (fence && vbuf->objs)
> - virtio_gpu_array_unlock_resv(vbuf->objs);
> - return;
> - }
> -
> if (vq->num_free < elemcnt) {
> spin_unlock(&vgdev->ctrlq.qlock);
> wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
> @@ -379,6 +378,7 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> else
> virtqueue_notify(vq);
> }
> + drm_dev_exit(idx);
> }
>
> static void virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
> @@ -460,12 +460,13 @@ static void virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
> {
> struct virtqueue *vq = vgdev->cursorq.vq;
> struct scatterlist *sgs[1], ccmd;
> + int idx, ret, outcnt;
> bool notify;
> - int ret;
> - int outcnt;
>
> - if (!vgdev->vqs_ready)
> + if (!drm_dev_enter(vgdev->ddev, &idx)) {
> + free_vbuf(vgdev, vbuf);
> return;
> + }
>
> sg_init_one(&ccmd, vbuf->buf, vbuf->size);
> sgs[0] = &ccmd;
> @@ -490,6 +491,8 @@ static void virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
>
> if (notify)
> virtqueue_notify(vq);
> +
> + drm_dev_exit(idx);
> }
>
> /* just create gem objects for userspace and long lived objects,
> --
> 2.18.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists