Re: [PATCH] drm/i915: Fix race condition in accessing GMBUS

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 09, 2012 at 05:14:57PM -0500, Yufeng Shen wrote:
> GMBUS has several ports and each has it's own corresponding
> I2C adpater. When multiple I2C adapters call gmbus_xfer() at
> the same time there is a race condition in using the underlying
> GMBUS controller. Fixing this by adding a mutex lock when calling
> gmbus_xfer().
> 
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_i2c.c |   21 ++++++++++++++++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9689ca3..d0f826e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -733,6 +733,8 @@ typedef struct drm_i915_private {
>  	u8 corr;
>  	spinlock_t *mchdev_lock;
>  
> +	struct mutex gmbus_mutex;
> +
>  	enum no_fbc_reason no_fbc_reason;
>  
>  	struct drm_mm_node *compressed_fb;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d30cccc..24fafdc 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -233,11 +233,16 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  					       struct intel_gmbus,
>  					       adapter);
>  	struct drm_i915_private *dev_priv = adapter->algo_data;
> -	int i, reg_offset;
> +	int i, reg_offset, ret;
>  
> -	if (bus->force_bit)
> -		return intel_i2c_quirk_xfer(dev_priv,
> +	mutex_lock(&dev_priv->gmbus_mutex);
> +
> +	if (bus->force_bit) {
> +		ret = intel_i2c_quirk_xfer(dev_priv,
>  					    bus->force_bit, msgs, num);
> +		mutex_unlock(&dev_priv->gmbus_mutex);
> +		return ret;

The usual code pattern is to have

out:
	mutex_unlock(mutex)

	return ret;

add the end of the function and

	ret = retval;
	goto out;

on all exit paths. This way it's clearer to see that all locks (or other
resources) get properly unlocked and unref'ed. Can you please adjust your
patch accordingly?

Otherwise the patch looks good.

Thanks, Daniel

> +	}
>  
>  	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
>  
> @@ -321,6 +326,7 @@ done:
>  	 * start of the next xfer, till then let it sleep.
>  	 */
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
> +	mutex_unlock(&dev_priv->gmbus_mutex);
>  	return i;
>  
>  timeout:
> @@ -331,9 +337,12 @@ timeout:
>  	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
>  	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
>  	if (!bus->force_bit)
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +	else
> +		ret = intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
>  
> -	return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
> +	mutex_unlock(&dev_priv->gmbus_mutex);
> +	return ret;
>  }
>  
>  static u32 gmbus_func(struct i2c_adapter *adapter)
> @@ -380,6 +389,8 @@ int intel_setup_gmbus(struct drm_device *dev)
>  	if (dev_priv->gmbus == NULL)
>  		return -ENOMEM;
>  
> +	mutex_init(&dev_priv->gmbus_mutex);
> +
>  	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
>  		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>  
> -- 
> 1.7.3.1
> 
> _______________________________________________
> 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