Re: [Linux-fbdev-devel] [PATCH] atyfb: Fix HP OmniBook 500 reboot hang

[Ville S. <sy...@sc...>]

On Tue, May 26, 2009 at 01:49:18PM -0700, Andrew Morton wrote:
> On Tue, 26 May 2009 02:05:14 +0300
> Ville Syrjala <sy...@sc...> 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ä
sy...@sc...
http://www.sci.fi/~syrjala/