[PATCH] atyfb: Fix HP OmniBook 500 reboot hang

[PATCH] atyfb: Fix HP OmniBook 500 reboot hang

[Ville Syrjala]

Apparently HP OmniBook 500's BIOS doesn't like the way atyfb reprograms
the hardware. The BIOS will simply hang after a reboot. Fix the problem
by restoring the hardware to it's original state on reboot.

Signed-off-by: Ville Syrjala <syrjala@sci.fi>
---
 drivers/video/aty/atyfb.h      |    2 +
 drivers/video/aty/atyfb_base.c |   69 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/video/aty/atyfb.h b/drivers/video/aty/atyfb.h
index 7691e73..0369653 100644
--- a/drivers/video/aty/atyfb.h
+++ b/drivers/video/aty/atyfb.h
@@ -187,6 +187,8 @@ struct atyfb_par {
 	int mtrr_reg;
 #endif
 	u32 mem_cntl;
+	struct crtc saved_crtc;
+	union aty_pll saved_pll;
 };
 
     /*
diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
index 1207c20..305f7c8 100644
--- a/drivers/video/aty/atyfb_base.c
+++ b/drivers/video/aty/atyfb_base.c
@@ -66,6 +66,7 @@
 #include <linux/spinlock.h>
 #include <linux/wait.h>
 #include <linux/backlight.h>
+#include <linux/reboot.h>
 
 #include <asm/io.h>
 #include <linux/uaccess.h>
@@ -249,8 +250,6 @@ static int aty_init(struct fb_info *info);
 static int store_video_par(char *videopar, unsigned char m64_num);
 #endif
 
-static struct crtc saved_crtc;
-static union aty_pll saved_pll;
 static void aty_get_crtc(const struct atyfb_par *par, struct crtc *crtc);
 
 static void aty_set_crtc(const struct atyfb_par *par, const struct crtc *crtc);
@@ -261,6 +260,8 @@ static void set_off_pitch(struct atyfb_par *par, const struct fb_info *info);
 static int read_aty_sense(const struct atyfb_par *par);
 #endif
 
+static DEFINE_MUTEX(reboot_lock);
+static struct fb_info *reboot_info;
 
     /*
      *  Interface used by the world
@@ -2390,9 +2391,9 @@ static int __devinit aty_init(struct fb_info *info)
 #endif /* CONFIG_FB_ATY_CT */
 
 	/* save previous video mode */
-	aty_get_crtc(par, &saved_crtc);
+	aty_get_crtc(par, &par->saved_crtc);
 	if(par->pll_ops->get_pll)
-		par->pll_ops->get_pll(info, &saved_pll);
+		par->pll_ops->get_pll(info, &par->saved_pll);
 
 	par->mem_cntl = aty_ld_le32(MEM_CNTL, par);
 	gtb_memsize = M64_HAS(GTB_DSP);
@@ -2667,8 +2668,8 @@ static int __devinit aty_init(struct fb_info *info)
 
 aty_init_exit:
 	/* restore video mode */
-	aty_set_crtc(par, &saved_crtc);
-	par->pll_ops->set_pll(info, &saved_pll);
+	aty_set_crtc(par, &par->saved_crtc);
+	par->pll_ops->set_pll(info, &par->saved_pll);
 
 #ifdef CONFIG_MTRR
 	if (par->mtrr_reg >= 0) {
@@ -3502,6 +3503,11 @@ static int __devinit atyfb_pci_probe(struct pci_dev *pdev, const struct pci_devi
 	par->mmap_map[1].prot_flag = _PAGE_E;
 #endif /* __sparc__ */
 
+	mutex_lock(&reboot_lock);
+	if (!reboot_info)
+		reboot_info = info;
+	mutex_unlock(&reboot_lock);
+
 	return 0;
 
 err_release_io:
@@ -3613,9 +3619,14 @@ static void __devexit atyfb_remove(struct fb_info *info)
 {
 	struct atyfb_par *par = (struct atyfb_par *) info->par;
 
+	mutex_lock(&reboot_lock);
+	if (reboot_info == info)
+		reboot_info = NULL;
+	mutex_unlock(&reboot_lock);
+
 	/* restore video mode */
-	aty_set_crtc(par, &saved_crtc);
-	par->pll_ops->set_pll(info, &saved_pll);
+	aty_set_crtc(par, &par->saved_crtc);
+	par->pll_ops->set_pll(info, &par->saved_pll);
 
 	unregister_framebuffer(info);
 
@@ -3808,6 +3819,39 @@ static int __init atyfb_setup(char *options)
 }
 #endif  /*  MODULE  */
 
+static int atyfb_reboot_notify(struct notifier_block *nb,
+			       unsigned long code, void *unused)
+{
+	struct atyfb_par *par;
+
+	if (code != SYS_RESTART)
+		return NOTIFY_DONE;
+
+	mutex_lock(&reboot_lock);
+
+	if (!reboot_info)
+		goto out;
+
+	par = reboot_info->par;
+
+	/*
+	 * HP OmniBook 500's BIOS doesn't like the state of the
+	 * hardware after atyfb has been used. Restore the hardware
+	 * to the original state to allow succesful reboots.
+	 */
+	aty_set_crtc(par, &par->saved_crtc);
+	par->pll_ops->set_pll(reboot_info, &par->saved_pll);
+
+ out:
+	mutex_unlock(&reboot_lock);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block atyfb_reboot_notifier = {
+	.notifier_call = atyfb_reboot_notify,
+};
+
 static int __init atyfb_init(void)
 {
     int err1 = 1, err2 = 1;
@@ -3826,11 +3870,18 @@ static int __init atyfb_init(void)
     err2 = atyfb_atari_probe();
 #endif
 
-    return (err1 && err2) ? -ENODEV : 0;
+    if (err1 && err2)
+	return -ENODEV;
+
+    register_reboot_notifier(&atyfb_reboot_notifier);
+
+    return 0;
 }
 
 static void __exit atyfb_exit(void)
 {
+	unregister_reboot_notifier(&atyfb_reboot_notifier);
+
 #ifdef CONFIG_PCI
 	pci_unregister_driver(&atyfb_driver);
 #endif
-- 
1.6.0.6


------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp asthey present alongside digital heavyweights like Barbarian
Group, R/GA, & Big Spaceship. http://www.creativitycat.com 

Re: [PATCH] atyfb: Fix HP OmniBook 500 reboot hang

[Andrew Morton]

On Tue, 26 May 2009 02:05:14 +0300
Ville Syrjala <syrjala@sci.fi> wrote:

> Apparently HP OmniBook 500's BIOS doesn't like the way atyfb reprograms
> the hardware. The BIOS will simply hang after a reboot. Fix the problem
> by restoring the hardware to it's original state on reboot.
> 
>
> ...
>
> @@ -3502,6 +3503,11 @@ static int __devinit atyfb_pci_probe(struct pci_dev *pdev, const struct pci_devi
>  	par->mmap_map[1].prot_flag = _PAGE_E;
>  #endif /* __sparc__ */
>  
> +	mutex_lock(&reboot_lock);
> +	if (!reboot_info)
> +		reboot_info = info;
> +	mutex_unlock(&reboot_lock);

This looks risky to me.  We save away a pointer to a structure which
was created by framebuffer_alloc().  What guarantee is there that this
memory is still valid when the reboot happens later on?


>  	return 0;
>  
>  err_release_io:
> @@ -3613,9 +3619,14 @@ static void __devexit atyfb_remove(struct fb_info *info)
>  {
>  	struct atyfb_par *par = (struct atyfb_par *) info->par;
>  
> +	mutex_lock(&reboot_lock);
> +	if (reboot_info == info)
> +		reboot_info = NULL;
> +	mutex_unlock(&reboot_lock);
> +
>  	/* restore video mode */
> -	aty_set_crtc(par, &saved_crtc);
> -	par->pll_ops->set_pll(info, &saved_pll);
> +	aty_set_crtc(par, &par->saved_crtc);
> +	par->pll_ops->set_pll(info, &par->saved_pll);
>  
>  	unregister_framebuffer(info);
>  
> @@ -3808,6 +3819,39 @@ static int __init atyfb_setup(char *options)
>  }
>  #endif  /*  MODULE  */
>  
> +static int atyfb_reboot_notify(struct notifier_block *nb,
> +			       unsigned long code, void *unused)
> +{
> +	struct atyfb_par *par;
> +
> +	if (code != SYS_RESTART)
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&reboot_lock);
> +
> +	if (!reboot_info)
> +		goto out;
> +
> +	par = reboot_info->par;
> +
> +	/*
> +	 * HP OmniBook 500's BIOS doesn't like the state of the
> +	 * hardware after atyfb has been used. Restore the hardware
> +	 * to the original state to allow succesful reboots.

"successful" ;)

> +	 */
> +	aty_set_crtc(par, &par->saved_crtc);
> +	par->pll_ops->set_pll(reboot_info, &par->saved_pll);
> +
> + out:
> +	mutex_unlock(&reboot_lock);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block atyfb_reboot_notifier = {
> +	.notifier_call = atyfb_reboot_notify,
> +};
> +
>  static int __init atyfb_init(void)
>  {
>      int err1 = 1, err2 = 1;
> @@ -3826,11 +3870,18 @@ static int __init atyfb_init(void)
>      err2 = atyfb_atari_probe();
>  #endif
>  
> -    return (err1 && err2) ? -ENODEV : 0;
> +    if (err1 && err2)
> +	return -ENODEV;
> +
> +    register_reboot_notifier(&atyfb_reboot_notifier);
> +
> +    return 0;
>  }

ick.  Please feel free to repair the indenting in atyfb_init().

>  static void __exit atyfb_exit(void)
>  {
> +	unregister_reboot_notifier(&atyfb_reboot_notifier);
> +
>  #ifdef CONFIG_PCI
>  	pci_unregister_driver(&atyfb_driver);
>  #endif

So we do the restoration for all supported devices on all machines,
even though it's only known to be needed on one card on one machine.

Hopefully that's safe, but a more cautious approach would use a
whitelist of some form.  I don't have enough experience with these
things to be able to judge the risk.

------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT 
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp as they present alongside digital heavyweights like Barbarian 
Group, R/GA, & Big Spaceship. http://p.sf.net/sfu/creativitycat-com 

Re: [PATCH] atyfb: Fix HP OmniBook 500 reboot hang

[Ville Syrjälä]

On Tue, May 26, 2009 at 01:49:18PM -0700, Andrew Morton wrote:
> On Tue, 26 May 2009 02:05:14 +0300
> Ville Syrjala <syrjala@sci.fi> wrote:
> 
> > Apparently HP OmniBook 500's BIOS doesn't like the way atyfb reprograms
> > the hardware. The BIOS will simply hang after a reboot. Fix the problem
> > by restoring the hardware to it's original state on reboot.
> > 
> >
> > ...
> >
> > @@ -3502,6 +3503,11 @@ static int __devinit atyfb_pci_probe(struct pci_dev *pdev, const struct pci_devi
> >  	par->mmap_map[1].prot_flag = _PAGE_E;
> >  #endif /* __sparc__ */
> >  
> > +	mutex_lock(&reboot_lock);
> > +	if (!reboot_info)
> > +		reboot_info = info;
> > +	mutex_unlock(&reboot_lock);
> 
> This looks risky to me.  We save away a pointer to a structure which
> was created by framebuffer_alloc().  What guarantee is there that this
> memory is still valid when the reboot happens later on?

atyfb_remove() will clear the pointer before freeing the memory. The
mutex is supposed to make sure that the structure won't be freed while 
the reboot notifier is executing.

Hmm. I suppose I might have to grab the fb_info lock there too to
protect against other fb activity happening at the same time.

I also noticed that I managed to misplace reboot_info pointer clearing
a bit. It should really be in atyfb_pci_remove() instead of
atyfb_remove() since it's set in atyfb_pci_probe().

> >  	return 0;
> >  
> >  err_release_io:
> > @@ -3613,9 +3619,14 @@ static void __devexit atyfb_remove(struct fb_info *info)
> >  {
> >  	struct atyfb_par *par = (struct atyfb_par *) info->par;
> >  
> > +	mutex_lock(&reboot_lock);
> > +	if (reboot_info == info)
> > +		reboot_info = NULL;
> > +	mutex_unlock(&reboot_lock);
> > +
> >  	/* restore video mode */
> > -	aty_set_crtc(par, &saved_crtc);
> > -	par->pll_ops->set_pll(info, &saved_pll);
> > +	aty_set_crtc(par, &par->saved_crtc);
> > +	par->pll_ops->set_pll(info, &par->saved_pll);
> >  
> >  	unregister_framebuffer(info);
> >  
> > @@ -3808,6 +3819,39 @@ static int __init atyfb_setup(char *options)
> >  }
> >  #endif  /*  MODULE  */
> >  
> > +static int atyfb_reboot_notify(struct notifier_block *nb,
> > +			       unsigned long code, void *unused)
> > +{
> > +	struct atyfb_par *par;
> > +
> > +	if (code != SYS_RESTART)
> > +		return NOTIFY_DONE;
> > +
> > +	mutex_lock(&reboot_lock);
> > +
> > +	if (!reboot_info)
> > +		goto out;
> > +
> > +	par = reboot_info->par;
> > +
> > +	/*
> > +	 * HP OmniBook 500's BIOS doesn't like the state of the
> > +	 * hardware after atyfb has been used. Restore the hardware
> > +	 * to the original state to allow succesful reboots.
> 
> "successful" ;)

Right.

> > +	 */
> > +	aty_set_crtc(par, &par->saved_crtc);
> > +	par->pll_ops->set_pll(reboot_info, &par->saved_pll);
> > +
> > + out:
> > +	mutex_unlock(&reboot_lock);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block atyfb_reboot_notifier = {
> > +	.notifier_call = atyfb_reboot_notify,
> > +};
> > +
> >  static int __init atyfb_init(void)
> >  {
> >      int err1 = 1, err2 = 1;
> > @@ -3826,11 +3870,18 @@ static int __init atyfb_init(void)
> >      err2 = atyfb_atari_probe();
> >  #endif
> >  
> > -    return (err1 && err2) ? -ENODEV : 0;
> > +    if (err1 && err2)
> > +	return -ENODEV;
> > +
> > +    register_reboot_notifier(&atyfb_reboot_notifier);
> > +
> > +    return 0;
> >  }
> 
> ick.  Please feel free to repair the indenting in atyfb_init().

The indentation is broken in other parts of the driver too. I'll make
a separate cosmetics patch to clean it all up.

> >  static void __exit atyfb_exit(void)
> >  {
> > +	unregister_reboot_notifier(&atyfb_reboot_notifier);
> > +
> >  #ifdef CONFIG_PCI
> >  	pci_unregister_driver(&atyfb_driver);
> >  #endif
> 
> So we do the restoration for all supported devices on all machines,
> even though it's only known to be needed on one card on one machine.
> 
> Hopefully that's safe, but a more cautious approach would use a
> whitelist of some form.  I don't have enough experience with these
> things to be able to judge the risk.

It should be safe in theory :) If the restoration breaks on some system
then the probe error handling and remove handling are also broken since
they do the same stuff. But to be honest I didn't test it on other
systems. I could do a DMI match but I'm not sure the extra complexity
is actually warranted. I'll give it a spin on a few other systems in
any case.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT 
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp as they present alongside digital heavyweights like Barbarian 
Group, R/GA, & Big Spaceship. http://p.sf.net/sfu/creativitycat-com