[Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner]

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