[Intel-gfx] [PATCH 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Sagar Arun Kamble]

On 1/22/2018 4:17 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-22 10:38:10)
>>
>> On 1/22/2018 3:46 PM, Chris Wilson wrote:
>>> Quoting Sagar Arun Kamble (2018-01-22 08:26:01)
>>>> +int intel_guc_log_relay_create(struct intel_guc *guc)
>>>> +{
>>>> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>>> +       struct rchan *guc_log_relay_chan;
>>>> +       size_t n_subbufs, subbuf_size;
>>>> +       int ret;
>>>> +
>>>> +       if (!i915_modparams.guc_log_level)
>>>> +               return 0;
>>>> +
>>>> +       GEM_BUG_ON(guc_log_has_relay(guc));
>>>> +
>>>>            /* Keep the size of sub buffers same as shared log buffer */
>>>> -       subbuf_size = guc->log.vma->obj->base.size;
>>>> +       subbuf_size = GUC_LOG_SIZE;
>>>>    
>>>>           /* Store up to 8 snapshots, which is large enough to buffer sufficient
>>>>            * boot time logs and provides enough leeway to User, in terms of
>>>> @@ -407,33 +442,31 @@ static int guc_log_runtime_create(struct intel_guc *guc)
>>>>                   DRM_ERROR("Couldn't create relay chan for GuC logging\n");
>>>>    
>>>>                   ret = -ENOMEM;
>>>> -               goto err_vaddr;
>>>> +               goto err;
>>>>           }
>>>>    
>>>>           GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
>>>>           guc->log.runtime.relay_chan = guc_log_relay_chan;
>>>>    
>>>> -       INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
>>>>           return 0;
>>>>    
>>>> -err_vaddr:
>>>> -       i915_gem_object_unpin_map(guc->log.vma->obj);
>>>> -       guc->log.runtime.buf_addr = NULL;
>>>> +err:
>>>> +       /* logging will be off */
>>>> +       i915_modparams.guc_log_level = 0;
>>>>           return ret;
>>>>    }
>>>>    
>>>> -static void guc_log_runtime_destroy(struct intel_guc *guc)
>>>> +void intel_guc_log_relay_destroy(struct intel_guc *guc)
>>>>    {
>>> Now exposed to multiple users, we need to document what the locking
>>> requirements are here. Or add some local locking.
>> Do you mean locking between relay_create and relay_destroy?
> We need a lock around guc->log.runtime.relay_chan as the destroy path is
> not ostensibly serialised between multiple potential callers. Ordinarily
> I would have said that serialisation for create/destroy/access of
> relay_chan was guaranteed by init/fini ordering,
I was also initially thinking that init/fini ordering should take care 
of synchronization.
Checked further and I see that relay_open and relay_destroy are 
synchronized by relay_channels_mutex
and internally if needed through debugfs inode synchronization. So I 
feel we need not add new lock.
>   but that's no longer
> clear (based on a 5min read of the patch).
>
> The most important question is "can relay_destroy be called while some
> user still has access to the relay_chan?"
>
>>>    Looks like at the
>>> moment, _create is using struct_mutex,
>> relay_create and relay_destroy are now to be done outside of struct_mutex.
>> I will add this documentation to the functions.
> (lockdep_assert_held :)
> -Chris

-- 
Thanks,
Sagar