[Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output

[Daniel Vetter]

On Tue, Mar 12, 2019 at 11:13:03PM +0100, Ahmed S. Darwish wrote:
> Hi,
> 
> [[ CCing John for the trylock parts ]]
> 
> On Mon, Mar 11, 2019 at 11:33:15PM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 11.03.2019 20.23, skrev Daniel Vetter:
> > > On Mon, Mar 11, 2019 at 06:42:16PM +0100, Noralf Trønnes wrote:
> > >> This adds support for outputting kernel messages on panic().
> > >> A kernel message dumper is used to dump the log. The dumper iterates
> > >> over each DRM device and it's crtc's to find suitable framebuffers.
> > >>
> > >> All the other dumpers are run before this one except mtdoops.
> > >> Only atomic drivers are supported.
> > >>
> > >> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> > >
> > > Bunch of comments/ideas for you or Darwish below, whoever picks this up.
> >
> > Actually it would ne nice if Darwish could pick it up since he will do
> > it on i915 which will be useful to a much broader audience.
> > If not I'll respin when I'm done with the drm_fb_helper refactoring.
> >
> 
> Yup, I'll be more than happy to do this.. while preserving all of
> Noralf's authorship and copyright notices of course.
> 
> I guess it can be:
> 
>   - Handle the comments posted by Daniel and others (I'll post
>     some questions too)
> 
>   - Add the necessary i915 specific bits
> 
>   - Test, post v3/v4/../vn. Rinse and repeat. Keep it local at
>     dri-devel until getting the necessary S-o-Bs.
> 
>   - Post to wider audience (some feedback from distribution folks
>     would also be nice, before posting to lkml)
> 
> More comments below..
> 
> [...]
> 
> > >> +
> > >> +static void drm_panic_kmsg_dump(struct kmsg_dumper *dumper,
> > >> +				enum kmsg_dump_reason reason)
> > >> +{
> > >> +	class_for_each_device(drm_class, NULL, dumper, drm_panic_dev_iter);
> > >
> > > class_for_each_device uses klist, which only uses an irqsave spinlock. I
> > > think that's good enough. Comment to that effect would be good e.g.
> > >
> > > 	/* based on klist, which uses only a spin_lock_irqsave, which we
> > > 	 * assume still works */
> > >
> > > If we aim for perfect this should be a trylock still, maybe using our own
> > > device list.
> > >
> 
> I definitely agree here.
> 
> The lock may already be locked either by a stopped CPU, or by the
> very same CPU we execute panic() on (e.g. NMI panic() on the
> printing CPU).
> 
> This is why it's very common for example in serial consoles, which
> are usually careful about re-entrance and panic contexts, to do:
> 
>   xxxxxx_console_write(...) {
> 	if (oops_in_progress)
> 		locked = spin_trylock_irqsave(&port->lock, flags);
> 	else
> 		spin_lock_irqsave(&port->lock, flags);
>   }
> 
> I'm quite positive we should do the same for panic drm drivers.

Yeah Ideally all the locking in the drm path would be trylock only.

I wonder whether lockdep could help us validate this, with some "don't
allow anything except trylocks in this context". It's easy to audit the
core code with review, but drivers are much tougher. And often end up with
really deep callchains to get at the backing buffers.
> John?
> 
> > >> +}
> > >> +
> > >> +static struct kmsg_dumper drm_panic_kmsg_dumper = {
> > >> +	.dump = drm_panic_kmsg_dump,
> > >> +	.max_reason = KMSG_DUMP_PANIC,
> > >> +};
> > >> +
> > >> +static ssize_t drm_panic_file_panic_write(struct file *file,
> > >> +					  const char __user *user_buf,
> > >> +					  size_t count, loff_t *ppos)
> > >> +{
> > >> +	unsigned long long val;
> > >> +	char buf[24];
> > >> +	size_t size;
> > >> +	ssize_t ret;
> > >> +
> > >> +	size = min(sizeof(buf) - 1, count);
> > >> +	if (copy_from_user(buf, user_buf, size))
> > >> +		return -EFAULT;
> > >> +
> > >> +	buf[size] = '\0';
> > >> +	ret = kstrtoull(buf, 0, &val);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	drm_panic_kmsg_dumper.max_reason = KMSG_DUMP_OOPS;
> > >> +	wmb();
> > >> +
> > >> +	/* Do a real test with: echo c > /proc/sysrq-trigger */
> > >> +
> > >> +	if (val == 0) {
> > >> +		pr_info("Test panic screen using kmsg_dump(OOPS)\n");
> > >> +		kmsg_dump(KMSG_DUMP_OOPS);
> > >> +	} else if (val == 1) {
> > >> +		char *null_pointer = NULL;
> > >> +
> > >> +		pr_info("Test panic screen using NULL pointer dereference\n");
> > >> +		*null_pointer = 1;
> > >> +	} else {
> > >> +		return -EINVAL;
> > >> +	}
> > >
> > > This isn't quite what I had in mind, since it still kills the kernel (like
> > > sysrq-trigger).
> >
> > If val == 0, it doesn't kill the kernel, it only dumps the kernel log.
> > And it doesn't taint the kernel either.
> >
> > > Instead what I had in mind is to recreate the worst
> > > possible panic context as much as feasible (disabling interrupts should be
> > > a good start, maybe we can even do an nmi callback), and then call our
> > > panic implementation. That way we can test the panic handler in a
> > > non-destructive way (i.e. aside from last dmesg lines printed to the
> > > screen nothing bad happens to the kernel: No real panic, no oops, no
> > > tainting).
> >
> > The interrupt case I can do, nmi I have no idea.
> >
> 
> I agree too. Disabling interrupts + CONFIG_DEBUG_ATOMIC_SLEEP
> would be a nice non-destructive test-case emulation.

See above, if we can somehow emulate "all locks are held, only allow
trylock" with lockdep that would be great too.

Plus nmi context, once/if that somehow becomes relevant.

The thing that killed the old drm panic handling code definitely was that
we flat out couldnt' test it except with real oopses. And that's just
whack-a-mole and bug reporter frustration if you first have a few patch
iterations around "oh sry, it's still some oops in the panic handler, not
the first one that we're seeing" :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch