[PATCH] drm/amdgpu: annotate a false positive recursive locking

[Christian König]

[SNIP]
>>>> What we should do instead is to make sure we have only a single lock for the complete hive instead.
>>>> [Dennis Li] If we use a single lock, users will must wait for all devices resuming successfully, but in fact their tasks are only running in device a. It is not friendly to users.
>>> Well that is intentional I would say. We can only restart submissions after all devices are resumed successfully, cause otherwise it can happen that a job on device A depends on memory on device B which isn't accessible.
>>> [Dennis Li] Yes, you are right. Driver have make sure that the shared resources(such as the shard memory) are ready before unlock the lock of adev one by one. But each device still has private hardware resources such as video  and display.
>> Yeah, but those are negligible and we have a team working on display support for XGMI. So this will sooner or later become a problem as well.
>>
>> I still think that a single rwsem for the whole hive is still the best option here.
>>
>> [Dennis Li] For your above concern, we can open a new thread to discuss it. If we make a decision to use a single after discussing, we can create another patch for it.
> That's a really good argument, but I still hesitate to merge this patch.
> How severe is the lockdep splat?
>
> At bare minimum we need a "/* TODO: ...." comment why we do this and how to remove the workaround again.
> [Dennis Li] It is not a workaround. According to design of lockdep, each rw_semaphore should has a separated lock_class_key. I have explained that init_rwsem has the above limitation, so we must correct it.

Yeah, but this explanation is not really sufficient. Again this is not 
an limitation of init_rwsem, but intentional behavior.

In other words in most cases lockdep should splat if you use rw 
semaphores like this.

That we avoid this by locking multiple amdgpu device always in the same 
order is rather questionable driver design.

>   Core network driver (net/core/dev.c) has the similar use case with us.

Just took a look at that, but this is completely different use case. The 
lockdep classes are grouped by driver type to note the difference in the 
network stack.

A quick search showed that no other device driver is doing this in the 
kernel, so I'm not sure if such a behavior wouldn't be rejected. Adding 
Daniel to comment on this as well.

Felix had some really good arguments to make that an urgent fix, so 
either we come up with some real rework of this or at least add a big 
"/* TODO....*/".

Regards,
Christian.

>
> Regards,
> Christian.
>