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

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

[Yufeng Shen]

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;
+	}
 
 	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

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

[Daniel Vetter]

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

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

[Yufeng Shen]

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 |   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

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

[Ben Widawsky]

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>

I do not see the race. All the i2c transfers should be protected
correctly by the i2c core, or else I think we haven't registered our
device properly. Could you give an example of when/how this can happen?

Ben

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

[Daniel Vetter]

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

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

[Yufeng Shen]

Hi Ben,

So I2C core  does protect multiple access to one adapter, but I2C core
does not protect the case where multiple adapters share the same underlying
device. GMBUS has 7 different pin pairs and each pair is registered as an I2C
adapter. I2C core can serialize the access to one pin pair, say that
the LVDS port
, or the DDC port. But when there are I2C transactions on both LVDS
and DDC ports,
since they share the same GMBUS registers, there will be race condition.
Does this make sense to you ?

--- Yufeng Shen

On Mon, Feb 13, 2012 at 4:04 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> 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>
>
> I do not see the race. All the i2c transfers should be protected
> correctly by the i2c core, or else I think we haven't registered our
> device properly. Could you give an example of when/how this can happen?
>
> Ben

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

[y]

From: Yufeng Shen <miletus@chromium.org>

Moved gmbus_mutex below intel_gmbus and added comments.
Rebased to drm-intel-next-queued.

----------------------------------------------------------------

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  |    8 ++++++++
 drivers/gpu/drm/i915/intel_i2c.c |   24 +++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 563d24e..40027de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,6 +313,14 @@ typedef struct drm_i915_private {
 		u32 reg0;
 	} *gmbus;
 
+	/** Multiple intel_gmbus i2c adapters are actually sharing the same
+	underlying GMBUS controller. i2c core protects concurrent use of
+	the same i2c adapter but is not aware of the concurrent use of the
+	underlying GMBUS controller. gmbus_mutex is used to prevent the race
+	condition from the perspective of GMBUS controller.
+	*/
+	struct mutex gmbus_mutex;
+
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
 	uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30cccc..fc75d71 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,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;
 
@@ -321,7 +325,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",
@@ -331,9 +336,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;
-
-	return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+		ret = -ENOMEM;
+	else
+		ret = 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)
@@ -380,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.1

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

[Yufeng Shen]

Moved gmbus_mutex below intel_gmbus and added comments.
Rebased to drm-intel-next-queued.

----------------------------------------------------------------

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  |    8 ++++++++
 drivers/gpu/drm/i915/intel_i2c.c |   24 +++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 563d24e..40027de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,6 +313,14 @@ typedef struct drm_i915_private {
 		u32 reg0;
 	} *gmbus;
 
+	/** Multiple intel_gmbus i2c adapters are actually sharing the same
+	underlying GMBUS controller. i2c core protects concurrent use of
+	the same i2c adapter but is not aware of the concurrent use of the
+	underlying GMBUS controller. gmbus_mutex is used to prevent the race
+	condition from the perspective of GMBUS controller.
+	*/
+	struct mutex gmbus_mutex;
+
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
 	uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30cccc..fc75d71 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,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;
 
@@ -321,7 +325,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",
@@ -331,9 +336,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;
-
-	return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+		ret = -ENOMEM;
+	else
+		ret = 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)
@@ -380,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.1

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

[Daniel Vetter]

On Mon, Feb 13, 2012 at 05:36:54PM -0500, Yufeng Shen wrote:
> Moved gmbus_mutex below intel_gmbus and added comments.
> Rebased to drm-intel-next-queued.
> 
> ----------------------------------------------------------------
> 
> 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>

Queued for -next (with some slight modifications in the comments), thanks
for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48