Skip to contentSkip to conversation

drm/fb: drop panic handling

RFC PATCH In review Dave Airlie 3 participants 4 messages Jul 9, 2015 – Aug 20, 2015

Subsystems (2)
lore
Share & tools
lore

Dave Airlie

author maintainer
#0 lore raw

RFC PATCH 1/1 drm/fb: drop panic handling

From: Dave Airlie <[email protected]>

This really doesn't seem to have much chance of working anymore,

esp for irq context, qxl at least tries to talk to the hw, and waits for irqs, and fails.

with runtime pm and other stuff I think we should just bail on this for now.

---
 drivers/gpu/drm/drm_fb_helper.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +0 -26
index cac4229..eaf652b 100644
@@ -429,24 +429,6 @@ static bool drm_fb_helper_force_kernel_mode(void)
 	return error;
 }
 
-static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
-			void *panic_str)
-{
-	/*
-	 * It's a waste of time and effort to switch back to text console
-	 * if the kernel should reboot before panic messages can be seen.
-	 */
-	if (panic_timeout < 0)
-		return 0;
-
-	pr_err("panic occurred, switching back to text console\n");
-	return drm_fb_helper_force_kernel_mode();
-}
-
-static struct notifier_block paniced = {
-	.notifier_call = drm_fb_helper_panic,
-};
-
 static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -672,9 +654,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
-			pr_info("drm: unregistered panic notifier\n");
-			atomic_notifier_chain_unregister(&panic_notifier_list,
-							 &paniced);
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 		}
 	}
@@ -1109,12 +1088,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 			info->node, info->fix.id);
 
-	/* Switch back to kernel console on panic */
-	/* multi card linked list maybe */
 	if (list_empty(&kernel_fb_helper_list)) {
-		dev_info(fb_helper->dev->dev, "registered panic notifier\n");
-		atomic_notifier_chain_register(&panic_notifier_list,
-					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 
Signature · 6 lines
2.4.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Reviewed-by on v1 1/1
#1 lore raw
Quoted 11 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This really doesn't seem to have much chance of working anymore,

esp for irq context, qxl at least tries to talk to the hw, and waits for irqs, and fails.

with runtime pm and other stuff I think we should just bail on this for now.

Yeah concurred that this has become hopeless. Also this would allow us to drop an pretension in i915 to still support this which means we can stop checking drm_can_sleep in our wait_for macros. Which has papered over some pretty serious bugs.

There's one more though, we can get into the fbdev callbacks through a pretty impressive chain:

panic() -> bust_spinlock() -> console_unblank() -> pass the trylock ->

	c->unblank() -> unblank_screen() (now in vt/vt.c) ->
		vc_sw->con_blank() -> fbcon_blank()

To make this really complete I think we also need to sprinkle

	if (oops_in_progress)
		return;

over all the fbdev entry points we have in drm_fbdev_helper.c plus all the ones in drivers which have their own (qxl, udl, i915 are the ones I know of).

Cheers, Daniel

Quoted 63 lines
---
 drivers/gpu/drm/drm_fb_helper.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +0 -26
index cac4229..eaf652b 100644
@@ -429,24 +429,6 @@ static bool drm_fb_helper_force_kernel_mode(void)
 	return error;
 }
 
-static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
-			void *panic_str)
-{
-	/*
-	 * It's a waste of time and effort to switch back to text console
-	 * if the kernel should reboot before panic messages can be seen.
-	 */
-	if (panic_timeout < 0)
-		return 0;
-
-	pr_err("panic occurred, switching back to text console\n");
-	return drm_fb_helper_force_kernel_mode();
-}
-
-static struct notifier_block paniced = {
-	.notifier_call = drm_fb_helper_panic,
-};
-
 static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -672,9 +654,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
-			pr_info("drm: unregistered panic notifier\n");
-			atomic_notifier_chain_unregister(&panic_notifier_list,
-							 &paniced);
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 		}
 	}
@@ -1109,12 +1088,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 			info->node, info->fix.id);
 
-	/* Switch back to kernel console on panic */
-	/* multi card linked list maybe */
 	if (list_empty(&kernel_fb_helper_list)) {
-		dev_info(fb_helper->dev->dev, "registered panic notifier\n");
-		atomic_notifier_chain_register(&panic_notifier_list,
-					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 
Signature · 6 lines
2.4.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw
Quoted 11 lines · Daniel Vetter
Quoted 10 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This really doesn't seem to have much chance of working anymore,

esp for irq context, qxl at least tries to talk to the hw, and waits for irqs, and fails.

with runtime pm and other stuff I think we should just bail on this for now.

Quoted 8 lines
Quoted 1 line

Yeah concurred that this has become hopeless. Also this would allow us to drop an pretension in i915 to still support this which means we can stop checking drm_can_sleep in our wait_for macros. Which has papered over some pretty serious bugs.

Well I applied this to drm-misc.

Quoted 7 lines

There's one more though, we can get into the fbdev callbacks through a pretty impressive chain:

panic() -> bust_spinlock() -> console_unblank() -> pass the trylock ->

	c->unblank() -> unblank_screen() (now in vt/vt.c) ->
		vc_sw->con_blank() -> fbcon_blank()
Quoted 8 lines

To make this really complete I think we also need to sprinkle

	if (oops_in_progress)
		return;

over all the fbdev entry points we have in drm_fbdev_helper.c plus all the ones in drivers which have their own (qxl, udl, i915 are the ones I know of).

I'll do a patch for this. Just realized that we have some cargo-culted checks already, but they don't work everywhere so mostly about unifying everything.
-Daniel

Quoted 71 lines

Cheers, Daniel

Quoted 63 lines
---
 drivers/gpu/drm/drm_fb_helper.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +0 -26
index cac4229..eaf652b 100644
@@ -429,24 +429,6 @@ static bool drm_fb_helper_force_kernel_mode(void)
 	return error;
 }
 
-static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
-			void *panic_str)
-{
-	/*
-	 * It's a waste of time and effort to switch back to text console
-	 * if the kernel should reboot before panic messages can be seen.
-	 */
-	if (panic_timeout < 0)
-		return 0;
-
-	pr_err("panic occurred, switching back to text console\n");
-	return drm_fb_helper_force_kernel_mode();
-}
-
-static struct notifier_block paniced = {
-	.notifier_call = drm_fb_helper_panic,
-};
-
 static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -672,9 +654,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
-			pr_info("drm: unregistered panic notifier\n");
-			atomic_notifier_chain_unregister(&panic_notifier_list,
-							 &paniced);
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 		}
 	}
@@ -1109,12 +1088,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 			info->node, info->fix.id);
 
-	/* Switch back to kernel console on panic */
-	/* multi card linked list maybe */
 	if (list_empty(&kernel_fb_helper_list)) {
-		dev_info(fb_helper->dev->dev, "registered panic notifier\n");
-		atomic_notifier_chain_register(&panic_notifier_list,
-					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 
Signature · 6 lines
2.4.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#3 lore raw
Quoted 94 lines · Daniel Vetter
Quoted 11 lines · Daniel Vetter
Quoted 10 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This really doesn't seem to have much chance of working anymore,

esp for irq context, qxl at least tries to talk to the hw, and waits for irqs, and fails.

with runtime pm and other stuff I think we should just bail on this for now.

Quoted 8 lines
Quoted 1 line

Yeah concurred that this has become hopeless. Also this would allow us to drop an pretension in i915 to still support this which means we can stop checking drm_can_sleep in our wait_for macros. Which has papered over some pretty serious bugs.

Well I applied this to drm-misc.

Quoted 7 lines

There's one more though, we can get into the fbdev callbacks through a pretty impressive chain:

panic() -> bust_spinlock() -> console_unblank() -> pass the trylock ->

	c->unblank() -> unblank_screen() (now in vt/vt.c) ->
		vc_sw->con_blank() -> fbcon_blank()
Quoted 8 lines

To make this really complete I think we also need to sprinkle

	if (oops_in_progress)
		return;

over all the fbdev entry points we have in drm_fbdev_helper.c plus all the ones in drivers which have their own (qxl, udl, i915 are the ones I know of).

I'll do a patch for this. Just realized that we have some cargo-culted checks already, but they don't work everywhere so mostly about unifying everything.
-Daniel

Quoted 50 lines

Cheers, Daniel

Quoted 47 lines
---
 drivers/gpu/drm/drm_fb_helper.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +0 -21
index cac4229..eaf652b 100644
@@ -429,24 +429,6 @@ static bool drm_fb_helper_force_kernel_mode(void)
 	return error;
 }
 
-static int drm_fb_helper_panic(struct notifier_block *n, unsigned long ununsed,
-			void *panic_str)
-{
-	/*
-	 * It's a waste of time and effort to switch back to text console
-	 * if the kernel should reboot before panic messages can be seen.
-	 */
-	if (panic_timeout < 0)
-		return 0;
-
-	pr_err("panic occurred, switching back to text console\n");
-	return drm_fb_helper_force_kernel_mode();
-}
-
-static struct notifier_block paniced = {
-	.notifier_call = drm_fb_helper_panic,
-};
-
 static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -672,9 +654,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
-			pr_info("drm: unregistered panic notifier\n");
-			atomic_notifier_chain_unregister(&panic_notifier_list,
-							 &paniced);
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 		}
 	}
@@ -1109,12 +1088,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 			info->node, info->fix.id);
 
Quoted 8 lines
Quoted 8 lines

drivers/gpu/drm/drm_fb_helper.c static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, · line 1093

Quoted 8 lines
drivers/gpu/drm/drm_fb_helper.c +0 -5
-	/* Switch back to kernel console on panic */
-	/* multi card linked list maybe */
 	if (list_empty(&kernel_fb_helper_list)) {
-		dev_info(fb_helper->dev->dev, "registered panic notifier\n");
-		atomic_notifier_chain_register(&panic_notifier_list,
-					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}

I missed this; Daniel just told me about it tonight. It's too bad we have to drop it; I think PPC has had it since forever, so losing it from KMS is too bad. Hopefully we can bring it back somehow with David's new kernel console stuff at some point...

Jesse

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel