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