dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: "Ahmed S. Darwish" <darwish.07@gmail.com>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: John Ogness <john.ogness@linutronix.de>,
	daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 1/3] drm: Add support for panic message output
Date: Wed, 13 Mar 2019 05:05:05 +0100	[thread overview]
Message-ID: <20190313040505.GB10747@darwi-home-pc> (raw)
In-Reply-To: <b841870f-70bf-4e1f-17b4-31d2d57fce69@tronnes.org>

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@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.
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.

thanks!

--
darwi
http://darwish.chasingpointers.com
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2019-03-13  4:05 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-11 17:42 [PATCH v2 0/3] drm: Add panic handling Noralf Trønnes
2019-03-11 17:42 ` [PATCH v2 1/3] drm: Add support for panic message output Noralf Trønnes
2019-03-11 19:23   ` Daniel Vetter
2019-03-11 19:29     ` Daniel Vetter
2019-03-11 22:40       ` Noralf Trønnes
2019-03-12  9:53         ` Daniel Vetter
2019-03-12  9:59           ` Daniel Vetter
2019-03-11 22:33     ` Noralf Trønnes
2019-03-12 10:58       ` Daniel Vetter
2019-03-12 13:29         ` Noralf Trønnes
2019-03-13  3:53         ` Ahmed S. Darwish
2019-03-12 22:13       ` Ahmed S. Darwish
2019-03-13  7:49         ` John Ogness
2019-03-13  8:37           ` Daniel Vetter
2019-03-14  2:51             ` Ahmed S. Darwish
2019-03-14  9:32               ` Daniel Vetter
2019-03-14  9:43                 ` John Ogness
2019-03-14  9:52                   ` John Ogness
2019-03-15 10:56                     ` Daniel Vetter
2019-03-13  8:35         ` Daniel Vetter
2019-03-14  4:45           ` Ahmed S. Darwish
2019-03-14  9:35             ` Daniel Vetter
2019-03-13 10:24         ` Noralf Trønnes
2019-03-13  4:05       ` Ahmed S. Darwish [this message]
2019-03-11 19:55   ` Sam Ravnborg
2019-03-12 10:47   ` Michel Dänzer
2019-03-12 16:17     ` [Intel-gfx] " Ville Syrjälä
2019-03-12 17:15       ` Noralf Trønnes
2019-03-12 17:25         ` Ville Syrjälä
2019-03-12 17:37           ` Noralf Trønnes
2019-03-12 17:44             ` Noralf Trønnes
2019-03-12 18:02             ` [Intel-gfx] " Ville Syrjälä
2019-03-13  8:29               ` Christian König
2019-03-13  8:43               ` [Intel-gfx] " Daniel Vetter
2019-03-13  9:35         ` Michel Dänzer
2019-03-13 13:31           ` [Intel-gfx] " Ville Syrjälä
2019-03-13 13:37             ` Christian König
2019-03-13 15:38               ` Michel Dänzer
2019-03-13 15:54                 ` [Intel-gfx] " Christian König
2019-03-13 16:16                   ` Kazlauskas, Nicholas
2019-03-13 17:30                     ` Koenig, Christian
2019-03-13 17:33                     ` Michel Dänzer
2019-03-13 17:41                       ` Kazlauskas, Nicholas
2019-03-14  9:50                         ` Daniel Vetter
2019-03-14 12:44                           ` Kazlauskas, Nicholas
2019-03-15 10:58                             ` [Intel-gfx] " Daniel Vetter
2019-03-13 17:52                       ` Koenig, Christian
2019-03-14  9:40                   ` [Intel-gfx] " Daniel Vetter
2019-03-11 17:42 ` [PATCH v2 2/3] drm/cma-helper: Add support for panic screen Noralf Trønnes
2019-03-11 17:42 ` [PATCH v2 3/3] drm/vc4: Support " Noralf Trønnes
2019-03-11 18:53 ` [PATCH v2 0/3] drm: Add panic handling Daniel Vetter
2019-03-17 23:06 ` Ahmed S. Darwish
2019-03-25  8:42   ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190313040505.GB10747@darwi-home-pc \
    --to=darwish.07@gmail.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=john.ogness@linutronix.de \
    --cc=noralf@tronnes.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).