[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 8:57 PM, Sagar Arun Kamble wrote:
>
>
> 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.
Sorry. after some more thinking, I am now convinced that lock will be 
needed as guc_log_has_relay was accessing
it without any locking. Will share new rev. Thanks for the review.
>>   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