[Nouveau] [PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP

[Daniel Vetter]

On Fri, Jul 15, 2016 at 01:10:51PM +0200, Peter Wu wrote:
> On Wed, Jul 13, 2016 at 04:57:19PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 13, 2016 at 02:40:50PM +0200, Peter Wu wrote:
> > > On Wed, Jul 13, 2016 at 11:54:49AM +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> > > > > The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> > > > > while nouveau was runtime suspended, a deadlock would occur due to
> > > > > nouveau_fbcon_set_suspend also trying to obtain console_lock().
> > > > > 
> > > > > Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> > > > > i915 code (which was done for performance reasons though).
> > > > > 
> > > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > Signed-off-by: Peter Wu <peter at lekensteyn.nl>
> > > > > ---
> > > > > Tested on top of v4.7-rc5, the deadlock is gone.
> > > > 
> > > > If we bother with this, it should imo be moved into the drm_fb_helper.c
> > > > function drm_fb_helper_set_suspend(). But this also smells like some kind
> > > > of bad duct-tape. I think Lukas is working on some other rpm vs. fbdev
> > > > deadlocks, maybe we could fix them all with one proper fix? I've made some
> > > > comments on Lukas' last patch series.
> > > 
> > > This patch is only needed for drivers that use console_lock (for
> > > drm_fb_helper_set_suspend) in their runtime resume functions.
> > > Lukas posted fixes for runtime PM reference leaks, those are different
> > > from this deadlock (see
> > > https://lists.freedesktop.org/archives/dri-devel/2016-July/113005.html
> > > for a backtrace for this issue).
> > > 
> > > The deadlock could also be avoided if the device backing the fbcon is
> > > somehow runtime-resumed outside the lock, but that feels like a larger
> > > hack that does not seem easy.
> > > 
> > > The i915 patch was done to reduce resume time (due to console_lock
> > > contention), that feature seems useful to all other drivers too even if
> > > the deadlock is fixed in a different way.
> > 
> > I might have imagined something, but I thought Lukas is already working on
> > some rpm vs. vga_switcheroo inversions. But looking again this was on the
> > audio side.
> > 
> > I think the proper solution for fbcon would be for the fbdev emulation to
> > also hold a runtime pm references when the console is in use.
> 
> nouveau does this by adding a fb_open and fb_release function that calls
> pm_runtime_get and pm_runtime_put respectively (and is the only driver
> doing this). So that is why it causes the console_lock issue for
> nouveau, but not for others.
> 
> > This should already happen correctly for vblank, the more tricky part
> > is fbdev mmap and fbcon:
> > 
> > - I have no idea, but there should be a way to intercept fbdev userspace
> >   mmaps and make sure that as long as an app has the fbdev mmapped we
> >   don't runtime suspend. No one really should be doing that (at least for
> >   normal setups), hence this shouldn't result in unsafe.
> 
> mmap normally needs a fd right? When the chardev /dev/fbX is opened, the
> fb_open function will be called. So nouveau should not have this issue
> with mmap/read/write to a fb while the device is suspended.
> 
> (This RPM thing was added in f231976c2e89 ("drm/nouveau/fbcon: take
> runpm reference when userspace has an open fd"), maybe it is not a bad
> idea for other drivers with RPM support either.)

Yes, looks like nouveau implements this correctly already. I guess we
could ponder whether we should lift this into the shared fbdev emulation,
using some fbdev_rpm_get/put functions in dev->mode_config.helper_private.
We can't just do the rpm stuff directly because:
- gross midlayer violation
- with the "pile of devices" approach arm chips employ it's unclear what
  exactly can't be runtime suspended when fbdev is still in use.

But as long as no one else cares I'd go meh.

> > - For fbcon, we could suspend it in the dpms off callbacks (maybe with a
> >   timer), and resume it only when enabling fbcon again - fbcon needs to
> >   redraw anyway on dpms on.
> 
> Would this guarantee that the fb is suspended before/during suspend (of
> the graphics device) and resumed somewhere during/after resume?
> 
> Suspending the fb also has the effect that reads/writes to /dev/fbN
> fail, maybe that is a bit too restricted since the framebuffer is just
> accessible until the device is suspended.
> 
> (Hmm, fb_read/fb_write does not seem to do any locking...)

Hm, annoying. That pretty much means runtime pm breaks fbcon. I guess in
practice no one will notice, since it generally should keep working for as
long as the output is on (if we ignore stuff like manual refresh panels
for a bit here ...).

> >   Another solution for fbcon might be to untangle the suspend/resume stuff
> >   and protect it by something else than console_lock. But that means
> >   fixing up fbcon locking horror shows.
> 
> console_lock seems needed for some code down the call stack, removing it
> risks some blow ups.
> 
> Some archaeology: this locking problem was introduced with 054430e773c9
> ("fbcon: fix locking harder"). In the past fb_set_suspend also took the
> fb_info lock but that was removed in 9e769ff3f585 ("fb: avoid possible
> deadlock caused by fb_set_suspend").

Yup, this is the horror show I mean and which I think we'd need to fix
here as the underlying issue. Let me elaborate:

fbdev has a notifier chain to essentially implement dynamic function
lookup. It's used for a bunch of things, and those functions are indexed
with a set of #defines:

- fbcon setup/signalling. This is essentially fbdev->fbcon calls. There's
  various different locking contexts/rules for the different callbacks.

- iirc it also goes the other way, where fbcon calls into the notifier to
  update stuff

- a few aux functions like backlight control and other random bits.

The reason for this seems to be that it allows you to load fbcon and fbdev
drivers in any order, and you'll still end up with an fbcon on every fbdev
driver. The problem is that the notifier itself has it's own mutex, which
means through that mutex it'll interfer every calling context with every
other calling context, resulting in all these deadlocks.

Thus far the approach was to shuffle random bits around, but I think that
really doesn't work well any more. There's 2 real fixes:

- Nuke the fbdev notifier, at least for the fbcon interaction. This means
  we'll make fbcon a compile-time selection, and you can't decide any more
  at runtime (by loading or not loading the module) whether you want
  fbcon. Since distros all built-in fbcon anyway I don't think anyway
  cares about that, since embedded folks disable everything they don't
  need anyway. This would be the cleanest solution I think.

- Split up the fb notifier into the different calling contexts. This would
  definitely help for e.g. the backlight stuff (another area), but I'm not
  100% sure it would work in your case here.

Once we have that we've essentially undone the damage in 054430e773c9 and
we could try to implement some proper locking (i.e. restore the fb_info
lock) again.

I won't have time for this myself, but I can definitely promise to review
patches and help push them through - this has been annoying me (and lots
of other people) since a long time.
-Daniel

> 
> Peter
> 
> > > My current plan is to move stuff out of the lock and allow (just)
> > > resuming the console to be delayed.  Some drivers (nouveau,
> > > radeon/amdgpu, i915) do unnecessary stuff under the console lock:
> > > 
> > >  - nouveau: I *think* that cleraing/setting FBINFO_HWACCEL_DISABLED
> > >    (nouveau_fbcon_accel_restore) is safe outside the lock as the fb is
> > >    already suspended before clearing/after setting the flag.
> > >  - radeon: since the console is suspended, I don't think that that all
> > >    of the code is radeon_resume_kms is really needed.
> > >  - amdgpu: same as radeon. Btw, console_lock is leaked on an error path.
> > >  - i915: I think that clearing the fb memory can be done outside the
> > >    lock too as the console is suspended.
> > > 
> > > Please correct me if my assumptions are flawed.
> > 
> > Yeah, fixing this independent issues should definitely help, irrespective
> > of how we fix fb_set_suspend.
> > 
> > > > Besides this, when fixing a deadlock pls provide more details about the
> > > > precise callchain and the locks involved in the deadlock. If you
> > > > discovered this using lockdep, then just add the entire lockdep splat to
> > > > the commit message. Otherwise there's lots of guesswork involved here.
> > > > -Daniel
> > > 
> > > There was no lockdep splat, it was triggered via the ioctl in the commit
> > > message. I'll include the verbose trace from the previous mail in the
> > > next proposed patch to reduce hunting though.
> > 
> > Sounds good too.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch