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

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