On Sun, Nov 26, 2017 at 11:58:43AM +0000, Chris Wilson wrote:

Quoting Lukas Wunner (2017-11-26 11:49:19)

...

Hm, the race at hand would be solved by the intel_fbdev_sync() below, or am I missing something? Still wondering why it's necessary to leave the fbdev around...

The race is solved, but if we do free ifbdev, we can't dereference ifbdev prior to the sync; and we store the async cookie inside ifbdev. Bleugh. Catch 22.

Right. Oh dear god! We could move the cookie into dev_priv, the fbdev_suspend_work is also there, outside of struct intel_fbdev.

What we might do then is just pull the struct into dev_priv under ifdef FBDEV.

I vaguely remember something that dev_priv deliberately only contains a pointer to struct intel_fbdev, that it *was* embedded in dev_priv in the past but moved out for some reason.

...

However the "if (ifbdev->vma)" looks a bit fishy, ifbdev could be NULL (e.g. if BIOS fb was too small but intelfb_alloc() failed) so I think this might lead to a null pointer deref. Does it make a difference if we check for ifbdev versus ifbdev->vma? I also notice that you added a check for ifbdev->vma with 15727ed0d944 but Daniel later removed it with 88be58be886f.

We know that ifbdev is non-NULL and can't become NULL until fini. So after the sync point, we want to ask the question of whether the config was successful, for that I used to use ->fb which now replaced by ->vma.

Yes if the fbdev is kept around then obviously it's fine to deref it.

Thanks,

Lukas