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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 10, 2012 at 12:50:01PM -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>

2 more nitpicks:
- patch doesn't apply cleanly - can you please rebase against
  drm-intel-next-queued available at

  http://cgit.freedesktop.org/~danvet/drm-intel/

- please move the new gmbus_mutex to the other gmbus stuff in
  drm_i915_private and add a small comment to that it explains against
  concurrent use (from e.g. userspace) of the single gmbus controller.

Yours, Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_i2c.c |   23 +++++++++++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 559fb6f..4ed9fd9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -722,6 +722,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 d98cee6..42569b1 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -232,11 +232,15 @@ 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);
> +		goto out;
> +	}
>  
>  	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
>  
> @@ -320,7 +324,8 @@ done:
>  	 * start of the next xfer, till then let it sleep.
>  	 */
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
> -	return i;
> +	ret = i;
> +	goto out;
>  
>  timeout:
>  	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
> @@ -330,9 +335,13 @@ 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);
> +out:
> +	mutex_unlock(&dev_priv->gmbus_mutex);
> +	return ret;
>  }
>  
>  static u32 gmbus_func(struct i2c_adapter *adapter)
> @@ -379,6 +388,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.4
> 

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