Quoting Daniel Vetter (2017-07-26 07:02:44)
> lockdep complaints about a locking recursion for the i2c bus lock
> because both the sdvo ddc proxy bus and the gmbus nested within use
> the same locking class. It's not really a deadlock since we never nest
> the other way round, but it's annoying.
>
> Fix it by pulling the gmbus locking into the i2c lock_ops for both
> i2c_adapater and making sure that the ddc_proxy_xfer function is
> entirely lockless.
>
> Re-layouting the extracted function resulted in some whitespace
> cleanups, I figured we might as well keep them.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 0403e30dfabc..392d3d2d4bbf 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -451,23 +451,24 @@ static const char * const cmd_status_names[] = {
> "Scaling not supported"
> };
>
> -static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> - const void *args, int args_len)
> +static bool __intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> + const void *args, int args_len,
> + bool locked)
> {
> u8 *buf, status;
> struct i2c_msg *msgs;
> int i, ret = true;
>
> - /* Would be simpler to allocate both in one go ? */
> + /* Would be simpler to allocate both in one go ? */
> buf = kzalloc(args_len * 2 + 2, GFP_KERNEL);
> if (!buf)
> return false;
>
> msgs = kcalloc(args_len + 3, sizeof(*msgs), GFP_KERNEL);
> if (!msgs) {
> - kfree(buf);
> + kfree(buf);
> return false;
> - }
> + }
>
> intel_sdvo_debug_write(intel_sdvo, cmd, args, args_len);
>
> @@ -498,7 +499,10 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> msgs[i+2].len = 1;
> msgs[i+2].buf = &status;
>
> - ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3);
> + if (locked)
locked=true is called from the unlocked context, very confusing.
> + ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3);
> + else
> + ret = __i2c_transfer(intel_sdvo->i2c, msgs, i+3);
> if (ret < 0) {
> DRM_DEBUG_KMS("I2c transfer returned %d\n", ret);
> ret = false;
> @@ -516,6 +520,12 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> return ret;
> }
>
> +static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> + const void *args, int args_len)
> +{
> + return __intel_sdvo_write_cmd(intel_sdvo, cmd, args, args_len, true);
> +}
> +
> static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
> void *response, int response_len)
> {
> @@ -602,13 +612,13 @@ static int intel_sdvo_get_pixel_multiplier(const struct drm_display_mode *adjust
> return 4;
> }
>
> -static bool intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo,
> - u8 ddc_bus)
> +static bool __intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo,
> + u8 ddc_bus)
> {
> /* This must be the immediately preceding write before the i2c xfer */
> - return intel_sdvo_write_cmd(intel_sdvo,
> - SDVO_CMD_SET_CONTROL_BUS_SWITCH,
> - &ddc_bus, 1);
> + return __intel_sdvo_write_cmd(intel_sdvo,
> + SDVO_CMD_SET_CONTROL_BUS_SWITCH,
> + &ddc_bus, 1, false);
> }
>
> static bool intel_sdvo_set_value(struct intel_sdvo *intel_sdvo, u8 cmd, const void *data, int len)
> @@ -2934,7 +2944,7 @@ static int intel_sdvo_ddc_proxy_xfer(struct i2c_adapter *adapter,
> {
> struct intel_sdvo *sdvo = adapter->algo_data;
>
> - if (!intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus))
> + if (!__intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus))
> return -EIO;
>
> return sdvo->i2c->algo->master_xfer(sdvo->i2c, msgs, num);
> @@ -2951,6 +2961,39 @@ static const struct i2c_algorithm intel_sdvo_ddc_proxy = {
> .functionality = intel_sdvo_ddc_proxy_func
> };
>
> +static void proxy_lock_bus(struct i2c_adapter *adapter,
> + unsigned int flags)
> +{
> + struct intel_sdvo *sdvo = adapter->algo_data;
> + struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> + mutex_lock(&dev_priv->gmbus_mutex);
sdvo->i2c->lock_ops->lock_bus(sdvo->i2c, flags);
> +}
> +
> +static int proxy_trylock_bus(struct i2c_adapter *adapter,
> + unsigned int flags)
> +{
> + struct intel_sdvo *sdvo = adapter->algo_data;
> + struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> + return mutex_trylock(&dev_priv->gmbus_mutex);
sdvo->i2c->lock_ops->trylock_bus(sdvo->i2c, flags);
> +}
> +
> +static void proxy_unlock_bus(struct i2c_adapter *adapter,
> + unsigned int flags)
> +{
> + struct intel_sdvo *sdvo = adapter->algo_data;
> + struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> + mutex_unlock(&dev_priv->gmbus_mutex);
sdvo->i2c->lock_ops->unlock_bus(sdvo->i2c, flags);
On reflection, you only need to hook up gmbus.lock_ops since gmbus is a
different lockclass it will be allowed to nest.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
