Re: [PATCH 4/5] drm: add core support for unplugging a device

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 20, 2012 at 04:13:48PM +0000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Two parts to this, one is simple unplug from sysfs for the device node.
> 
> The second adds an unplugged state, if we have device opens, we
> just set the unplugged state and return, if we have no device
> opens we drop the drm device.
> 
> If after a lastclose we discover we are unplugged we then
> drop the drm device.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

I like the unplugged state to handle whole-device removal, but I think
this needs a few extensions:
- I think we should unconditionally unregister the drm dev minors. Simply
  checking dev->unplugged should do the trick because we can't just tear
  down the minor structs (we can only unregister the dev node).
- writing to dev->unplugged needs to be protected by the same mutex (i.e.
  drm_global_mutex) as the actual unregister step.
- I think we should add a simply drm_device_is_unplugged so that drivers
  can check the unplugged state in a race-free manner. Adding an smp_wmb()
  after writing to dev->unplugged and an smb_rmb() before reading it in
  that helper (plus ensuring that dev->unplugged is a separate word in
  drm_device) should do the trick without overhead.
- I think we wan't the drm core to return -ENODEV for any ioctls an open
  calls and return a SIGBUS on the fault handler. Currently you roll that
  yourself in udl, but I can't think of a use-case where we want to stick
  the interfaces around after a hotremove of the entire device.

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_fops.c |    2 ++
>  drivers/gpu/drm/drm_stub.c |   22 ++++++++++++++++++++++
>  include/drm/drmP.h         |    2 ++
>  3 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 6263b01..154cfac 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -579,6 +579,8 @@ int drm_release(struct inode *inode, struct file *filp)
>  			retcode = -EBUSY;
>  		} else
>  			retcode = drm_lastclose(dev);
> +		if (dev->unplugged)
> +			drm_put_dev(dev);
>  	}
>  	mutex_unlock(&drm_global_mutex);
>  
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 6d7b083..b04c92d 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -429,6 +429,11 @@ int drm_put_minor(struct drm_minor **minor_p)
>  	return 0;
>  }
>  
> +static void drm_unplug_minor(struct drm_minor *minor)
> +{
> +	drm_sysfs_device_remove(minor);
> +}
> +
>  /**
>   * Called via drm_exit() at module unload time or when pci device is
>   * unplugged.
> @@ -492,3 +497,20 @@ void drm_put_dev(struct drm_device *dev)
>  	kfree(dev);
>  }
>  EXPORT_SYMBOL(drm_put_dev);
> +
> +void drm_unplug_dev(struct drm_device *dev)
> +{
> +	/* for a USB device */
> +	if (drm_core_check_feature(dev, DRIVER_MODESET))
> +		drm_unplug_minor(dev->control);
> +	drm_unplug_minor(dev->primary);
> +
> +	dev->unplugged = true;
> +
> +	mutex_lock(&drm_global_mutex);
> +	if (dev->open_count == 0) {
> +		drm_put_dev(dev);
> +	}
> +	mutex_unlock(&drm_global_mutex);
> +}
> +EXPORT_SYMBOL(drm_unplug_dev);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 92f0981..8c11797 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1170,6 +1170,7 @@ struct drm_device {
>  	struct idr object_name_idr;
>  	/*@} */
>  	int switch_power_state;
> +	bool unplugged; /* device has been unplugged or gone away */
>  };
>  
>  #define DRM_SWITCH_POWER_ON 0
> @@ -1464,6 +1465,7 @@ extern void drm_master_put(struct drm_master **master);
>  
>  extern void drm_put_dev(struct drm_device *dev);
>  extern int drm_put_minor(struct drm_minor **minor);
> +extern void drm_unplug_dev(struct drm_device *dev);
>  extern unsigned int drm_debug;
>  
>  extern unsigned int drm_vblank_offdelay;
> -- 
> 1.7.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48