| Message ID | 1455306648-31583-1-git-send-email-hshi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
David - is the v3 patch acceptable to you? Thanks. On Fri, Feb 12, 2016 at 11:50 AM, Haixia Shi <hshi@chromium.org> wrote: > v1: 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. > > v2: Based on discussion with Stephane and David, drop most of the unplugged > flag handling logic in drm/udl except for udl_detect() and udl_fb_open(). > The intention is to treat the device removal as a connector-unplug, and kep > the UDL device fully functional. > > v3: Based on feedback from David, entirely drop the "unplugged" flag and > all > related code. There is no need to check for the unplugged flag as the > existing > udl_usb_disconnect() behavior already ensures the controller is removed, > and > all code paths that uses the usb-device are not reachable after unplug. > > When a UDL monitor is unplugged, we need to still treat it as a fully > functional device which just happens to have its connector unplugged. > This allows user-space to properly deallocate fbs and dumb buffers > before closing the device. > > This drops the "unplugged" flag hack, which puts the device in a > non-functional state after USB unplug and rejects most operations on > the device such as ioctls with error -ENODEV. > > Signed-off-by: Haixia Shi <hshi@chromium.org> > Reviewed-by: Stéphane Marchesin <marcheu@chromium.org> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > 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_fb.c | 6 ------ > include/drm/drmP.h | 14 -------------- > 8 files changed, 39 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); > } > > 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); > - > 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); > } > 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; > - > 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; > - > 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; > - > 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..a6d5e21 100644 > --- a/drivers/gpu/drm/udl/udl_connector.c > +++ b/drivers/gpu/drm/udl/udl_connector.c > @@ -96,8 +96,6 @@ 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)) > - return connector_status_disconnected; > return connector_status_connected; > } > > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index 200419d..29aca6c 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, > const struct fb_image *image) > static int udl_fb_open(struct fb_info *info, int user) > { > struct udl_fbdev *ufbdev = info->par; > - struct drm_device *dev = ufbdev->ufb.base.dev; > - 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)) > - return -ENODEV; > > ufbdev->fb_count++; > > 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 > >
Hi On Fri, Feb 12, 2016 at 8:50 PM, Haixia Shi <hshi@chromium.org> wrote: > v1: 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. > > v2: Based on discussion with Stephane and David, drop most of the unplugged > flag handling logic in drm/udl except for udl_detect() and udl_fb_open(). > The intention is to treat the device removal as a connector-unplug, and kep > the UDL device fully functional. > > v3: Based on feedback from David, entirely drop the "unplugged" flag and all > related code. There is no need to check for the unplugged flag as the existing > udl_usb_disconnect() behavior already ensures the controller is removed, and > all code paths that uses the usb-device are not reachable after unplug. > > When a UDL monitor is unplugged, we need to still treat it as a fully > functional device which just happens to have its connector unplugged. > This allows user-space to properly deallocate fbs and dumb buffers > before closing the device. > > This drops the "unplugged" flag hack, which puts the device in a > non-functional state after USB unplug and rejects most operations on > the device such as ioctls with error -ENODEV. > > Signed-off-by: Haixia Shi <hshi@chromium.org> > Reviewed-by: Stéphane Marchesin <marcheu@chromium.org> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > 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_fb.c | 6 ------ > include/drm/drmP.h | 14 -------------- > 8 files changed, 39 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); > } > > 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); > - > 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); Who frees the udl device now? What code-path is responsible to call drm_dev_unregister() for udl devices? This hunk looks bogus to me. Please elaborate. > } > 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; > - > 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; > - > 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; > - > 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..a6d5e21 100644 > --- a/drivers/gpu/drm/udl/udl_connector.c > +++ b/drivers/gpu/drm/udl/udl_connector.c > @@ -96,8 +96,6 @@ 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)) > - return connector_status_disconnected; > return connector_status_connected; Who marks a connector as "disconnect" once the device is removed? > } > > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index 200419d..29aca6c 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) > static int udl_fb_open(struct fb_info *info, int user) > { > struct udl_fbdev *ufbdev = info->par; > - struct drm_device *dev = ufbdev->ufb.base.dev; > - 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)) > - return -ENODEV; > > ufbdev->fb_count++; > > 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; Did you actually test this patch? How is it supposed to work? Please explain _exactly_ in your commit message what the new semantics are. I highly doubt that this patch does what you want. Also, please run kmemleak on your tests to verify that all memory is actually released. Generally, I like the approach. However, I think you still need a state that marks a device as dropped. I'd do something like this: 1) In udl_usb_disconnect(), set udl->udev to NULL, locked by mode_config.mutex. This should be the _first_ thing you do there. This serves as barrier against ->get_modes(), which is the only path that uses "udl->udev". 2) In udl_detect(), return 'connector_status_disconnected' if "udl->udev" is NULL. 3) In udl_get_edid() add "if (WARN_ON(!udl->udev)) return NULL;" 4) Hook into f_ops.release() and call into drm_release(). But afterwards you have to call drm_put_dev() if open_count is 0 now. Thanks David
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); } 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); - 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); } 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; - 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; - 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; - 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..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ 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)) - return connector_status_disconnected; return connector_status_connected; } diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par; - struct drm_device *dev = ufbdev->ufb.base.dev; - 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)) - return -ENODEV; ufbdev->fb_count++; 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;