Hi
On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi <hshi@xxxxxxxxxxxx> wrote:
> Remove the general drm_device_is_unplugged() checks, and move the unplugged
> flag handling logic into drm/udl. In general we want to keep driver-specific
> logic out of common drm code.
I like the idea of moving this hack into udl. However, I don't think
this patch is sufficient, see below for comments.
Btw., hotplug might happen on other buses as well. It just happens
that no-one tried pci/platform unplugging so far.. We used to have
some patches flying around to make it work properly (with enter/leave
markers), but no-one picked those up. But I like the idea of moving
this unplugged marker into UDL, to make clear if someone else needs
it, they better do it properly.
> Signed-off-by: Haixia Shi <hshi@xxxxxxxxxxxx>
> Reviewed-by: Stéphane Marchesin <marcheu@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_drv.c | 6 ------
> drivers/gpu/drm/drm_fops.c | 2 --
> drivers/gpu/drm/drm_gem.c | 3 ---
> drivers/gpu/drm/drm_ioctl.c | 3 ---
> drivers/gpu/drm/drm_vm.c | 3 ---
> drivers/gpu/drm/udl/udl_connector.c | 2 +-
> drivers/gpu/drm/udl/udl_drv.c | 1 +
> drivers/gpu/drm/udl/udl_drv.h | 3 +++
> drivers/gpu/drm/udl/udl_fb.c | 2 +-
> drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++
> include/drm/drmP.h | 14 --------------
> 11 files changed, 21 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..f93ee12 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>
> if (!minor) {
> return ERR_PTR(-ENODEV);
> - } else if (drm_device_is_unplugged(minor->dev)) {
> - drm_dev_unref(minor->dev);
> - return ERR_PTR(-ENODEV);
> }
You cannot just drop this without a replacement. You should add a
drm_driver.open callback to UDL which checks for this.
drm_minor_acquire() is only ever called from drm_stub_open(), and as
such only from drm_open().
Alternatively, you can provide fops.open from UDL and wrap drm_open().
>
> return minor;
> @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>
> mutex_lock(&drm_global_mutex);
> -
> - drm_device_set_unplugged(dev);
> -
You replace this by moving it into the caller of drm_unplug_dev().
Sounds good to me. There has never been a real reason to put it
underneath the global mutex, anyway.
> if (dev->open_count == 0) {
> drm_put_dev(dev);
> }
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 1ea8790..b4332d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
>
> if (!--dev->open_count) {
> retcode = drm_lastclose(dev);
> - if (drm_device_is_unplugged(dev))
> - drm_put_dev(dev);
Again, you cannot drop this without replacement! In this case, you
really should wrap fops.release() from UDL and call into drm_release()
before copying this unplug-logic.
> }
> mutex_unlock(&drm_global_mutex);
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e8c77e..c622e32 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> struct drm_vma_offset_node *node;
> int ret;
>
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
Rather than just dropping it, you better move it into udl_drm_gem_mmap().
> drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> vma->vm_pgoff,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8ce2a0c..f959074 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
>
> dev = file_priv->minor->dev;
>
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
You *must* provide a wrapper for fops.ioctl() in udl which does this check.
> is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>
> if (is_driver_ioctl) {
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index f90bd5f..3a68be4 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma)
> struct drm_device *dev = priv->minor->dev;
> int ret;
>
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
This looks fine to me.
> mutex_lock(&dev->struct_mutex);
> ret = drm_mmap_locked(filp, vma);
> mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index 4709b54..b168f2c 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
> static enum drm_connector_status
> udl_detect(struct drm_connector *connector, bool force)
> {
> - if (drm_device_is_unplugged(connector->dev))
> + if (udl_device_is_unplugged(connector->dev))
> return connector_status_disconnected;
> return connector_status_connected;
> }
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d5728ec..3cbafe7 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
> drm_connector_unplug_all(dev);
> udl_fbdev_unplug(dev);
> udl_drop_usb(dev);
> + udl_device_set_unplugged(dev);
> drm_unplug_dev(dev);
This looks good.
> }
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 4a064ef..61e117a 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -65,6 +65,7 @@ struct udl_device {
> atomic_t bytes_identical; /* saved effort with backbuffer comparison */
> atomic_t bytes_sent; /* to usb, after compression including overhead */
> atomic_t cpu_kcycles_used; /* transpired during pixel processing */
> + atomic_t unplugged; /* device has been unplugged or gone away */
> };
>
> struct udl_gem_object {
> @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
> int width, int height);
>
> int udl_drop_usb(struct drm_device *dev);
> +void udl_device_set_unplugged(struct drm_device *dev);
> +int udl_device_is_unplugged(struct drm_device *dev);
>
> #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */
> #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 200419d..df1d53e 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user)
> struct udl_device *udl = dev->dev_private;
>
> /* If the USB device is gone, we don't accept new opens */
> - if (drm_device_is_unplugged(udl->ddev))
> + if (udl_device_is_unplugged(udl->ddev))
> return -ENODEV;
>
> ufbdev->fb_count++;
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 33dbfb2..f6028a4 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev)
> kfree(udl);
> return 0;
> }
> +
> +void udl_device_set_unplugged(struct drm_device *dev)
> +{
> + struct udl_device *udl = dev->dev_private;
> + smp_wmb();
> + atomic_set(&udl->unplugged, 1);
> +}
> +
> +int udl_device_is_unplugged(struct drm_device *dev)
> +{
> + struct udl_device *udl = dev->dev_private;
> + int ret = atomic_read(&udl->unplugged);
> + smp_rmb();
> + return ret;
> +}
This looks fine to me.
Please make sure to wrap most of the file_operations callbacks in UDL
and protect them with the "unplugged" check. Yes, we all know that
this is racy, and always has been. However, it is still better than
nothing. So please don't just drop them without any explanation!
Thanks
David
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d7162cf..40c6099 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -748,7 +748,6 @@ struct drm_device {
> struct drm_minor *control; /**< Control node */
> struct drm_minor *primary; /**< Primary node */
> struct drm_minor *render; /**< Render node */
> - atomic_t unplugged; /**< Flag whether dev is dead */
> struct inode *anon_inode; /**< inode for private address-space */
> char *unique; /**< unique name of the device */
> /*@} */
> @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
> return ((dev->driver->driver_features & feature) ? 1 : 0);
> }
>
> -static inline void drm_device_set_unplugged(struct drm_device *dev)
> -{
> - smp_wmb();
> - atomic_set(&dev->unplugged, 1);
> -}
> -
> -static inline int drm_device_is_unplugged(struct drm_device *dev)
> -{
> - int ret = atomic_read(&dev->unplugged);
> - smp_rmb();
> - return ret;
> -}
> -
> static inline bool drm_is_render_client(const struct drm_file *file_priv)
> {
> return file_priv->minor->type == DRM_MINOR_RENDER;
> --
> 2.7.0.rc3.207.g0ac5344
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
