Breakage in "track dev_mapping in more robust and flexible way"

[Ilija Hadzic]

On Fri, 26 Oct 2012, Thomas Hellstrom wrote:

> Hi,
>
> On 10/25/2012 11:27 PM, Ilija Hadzic wrote:
>> 
>> Can you give the attached patch a whirl and let me know if it fixes the 
>> problem?
>> 
>> As I indicated in my previous note, vmwgfx should be the only affected 
>> driver because it looks at dev_mapping in the open hook (others do it when 
>> they create an object, so they are not affected).
>> 
>> I'll probably revise it (and I'll have some general questions about 
>> drm_open syscall) before officially send the patch, but I wanted to get 
>> something quickly to you to check if it fixes your problem. I hope that 
>> your vmwgfx test environment is such that you can reproduce the original
>> problem.
>> 
>> thanks,
>> 
>> -- Ilija
>
> Yes, it appears like this patch fixes the problem. It'd be good to have it in 
> 3.7 (drm-fixes) with a cc to stable.
>

OK great. Thanks for testing. Before I send out an "official" patch, I 
have a few questions for those who have been around longer and can 
possibly reflect better than me on the history of drm_open syscall.

Currently, before touching dev->dev_mapping field we grab dev->struct 
mutex. This has been introduced by Dave Airlie a long time ago in 
a2c0a97b784f837300f7b0869c82ab712c600952. I tried to preserve that in all 
patches where I touched dev_open, but looking at the code I don't think 
the mutex is necessary. Namely, drm_open is only set in drm_open, and 
concurrent openers are protected with drm_global_mutex. Other places 
(drivers) where dev->dev_mapping is accessed is read-only and dev_mapping 
is written at first open when there are no file descriptors around to 
issue any other call. Also, it doesn't look to me that any driver locks 
dev->struct_mutex before accessing dev->dev_mapping anyway. So I am 
thinking of dropping the mutex completely, but I would like to hear a 
second thought.

The other issue, I noticed is that of the drm_setup() call fails, the 
open_count counter would remain incremented and I think we need to restore 
it back (or if I am missing something, would someone please enlighten me). 
This was also in the kernel all this time (and I have not noticed until 
now), so I "smuggled" that fix in the patch that I sent you. However, 
wonder if I should cut the separate patch for open_count fix.

Actually, I think that I should cut three patches: one to drop the mutex, 
one to fix the open_count and one to fix your problem with dev_mapping and 
that probably all three should CC stable. Before I do that, I'd like to 
hear opinions of others.

thanks,

Ilija