Re: [PATCH 2/2] drm/i915: use mutex_lock_interruptible for debugfs files

[Ben Widawsky <ben@bwidawsk.net>]

On Thu,  9 Aug 2012 15:07:02 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> It's no fun if your shell hangs when the driver has gone on vacation
> and you want to know why ...
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 31aa4b8..8acb8e9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -794,10 +794,14 @@ i915_error_state_write(struct file *filp,
>  	struct seq_file *m = filp->private_data;
>  	struct i915_error_state_file_priv *error_priv = m->private;
>  	struct drm_device *dev = error_priv->dev;
> +	int ret;
>  
>  	DRM_DEBUG_DRIVER("Resetting error state\n");
>  
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	i915_destroy_error_state(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -1467,8 +1471,12 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
>  		   swizzle_string(dev_priv->mm.bit_6_swizzle_x));
>  	seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
> @@ -1669,7 +1677,7 @@ i915_ring_stop_write(struct file *filp,
>  	struct drm_device *dev = filp->private_data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	char buf[20];
> -	int val = 0;
> +	int val = 0, ret;
>  
>  	if (cnt > 0) {
>  		if (cnt > sizeof(buf) - 1)
> @@ -1684,7 +1692,10 @@ i915_ring_stop_write(struct file *filp,
>  
>  	DRM_DEBUG_DRIVER("Stopping rings 0x%08x\n", val);
>  
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	dev_priv->stop_rings = val;
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -1861,12 +1872,15 @@ i915_cache_sharing_read(struct file *filp,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	char buf[80];
>  	u32 snpcr;
> -	int len;
> +	int len, ret;
>  
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> -	mutex_lock(&dev_priv->dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
>  
> @@ -1976,6 +1990,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  {
>  	struct drm_device *dev = inode->i_private;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
> @@ -1987,7 +2002,10 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  	 * hanging here is probably a minor inconvenience not to be seen my
>  	 * almost every user.
>  	 */
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	gen6_gt_force_wake_put(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  


If you feel like changing this intentionally non interruptible lock, you
should kill the comment about it too.



-- 
Ben Widawsky, Intel Open Source Technology Center