Re: [PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Wednesday 06 May 2009 11:20:52 am Bjorn Helgaas wrote:
> On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> > Below is the outcome of the patch which fixes a fixed feature button
> > s2ram issue.
> > 
> > This does not work anymore since Bjoern's patches.
> > Comparing the with the HID of the acpi device looks ugly.
> > 
> > Shall I revive the fixed feature vs GPE button types?
> > ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?
> 
> I don't understand exactly how I broke this.
> 
> Your patch makes us remove the fixed event handler in .suspend()
> and install it again in .resume().  In 2.6.29, we didn't touch the
> handler during suspend/resume, and my patches didn't change that
> aspect of things.
> 
> Whatever the problem is, it feels like something that should be
> handled in the Linux/ACPI code, not in the driver.
> 
> Did you identify the patch that broke things?  My guess would be
> 373cfc360ec773b, but I took a look at it (and 46ec8598fde74b, on
> which it depends), and I don't see the problem yet.

What's happening here?  If I broke something, I want to figure it
out ASAP because it sounds like a regression from 2.6.29.  Is there
a bugzilla or more detailed bug report anywhere?

Bjorn

> > Can you think of anything nicer?
> > 
> > Thanks,
> > 
> >      Thomas
> > 
> > 
> > ACPI: button: Fix fixed feature buttons for s2disk for HW modifing event registers
> > 
> > This happened on a recent HP laptop. After s2disk the fixed feature power button
> > event handler is not functional because the BIOS has modified the fixed feature
> > register(s) when resuming/rebooting after s2disk.
> > Setting up some bits there again by unregistering and re-installing the fixed
> > feature event handler fixes the problem and the button is functional again after s2disk.
> > 
> > Comment from Rafael:
> > This may be a consequence of the fact that we initialize ACPI before reading
> > the hibernation image from disk.  The spec seems to assume that won't be done. 
> > Hence, I think we should handle this.
> > 
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 9195deb..80ee2bc 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -74,6 +74,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
> >  static int acpi_button_add(struct acpi_device *device);
> >  static int acpi_button_remove(struct acpi_device *device, int type);
> >  static int acpi_button_resume(struct acpi_device *device);
> > +static int acpi_button_suspend(struct acpi_device *device, pm_message_t state);
> >  static void acpi_button_notify(struct acpi_device *device, u32 event);
> >  static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
> >  static int acpi_button_state_open_fs(struct inode *inode, struct file *file);
> > @@ -85,6 +86,7 @@ static struct acpi_driver acpi_button_driver = {
> >  	.ops = {
> >  		.add = acpi_button_add,
> >  		.resume = acpi_button_resume,
> > +		.suspend = acpi_button_suspend,
> >  		.remove = acpi_button_remove,
> >  		.notify = acpi_button_notify,
> >  	},
> > @@ -281,8 +283,41 @@ static int acpi_button_resume(struct acpi_device *device)
> >  {
> >  	struct acpi_button *button = acpi_driver_data(device);
> >  
> > -	if (button->type == ACPI_BUTTON_TYPE_LID)
> > +	if (!button)
> > +		return -EINVAL;
> > +	switch (button->type) {
> > +	case ACPI_BUTTON_TYPE_LID:
> >  		return acpi_lid_send_state(device);
> > +	case ACPI_BUTTON_TYPE_SLEEPF:
> > +		return acpi_install_fixed_event_handler(
> > +					ACPI_EVENT_SLEEP_BUTTON,
> > +					acpi_button_notify_fixed, button);
> > +
> > +	case ACPI_BUTTON_TYPE_POWERF:
> > +		return acpi_install_fixed_event_handler(
> > +					ACPI_EVENT_POWER_BUTTON,
> > +					acpi_button_notify_fixed, button);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int acpi_button_suspend(struct acpi_device *device, pm_message_t state)
> > +{
> > +	struct acpi_button *button;
> > +	if (!device)
> > +		return -EINVAL;
> > +	button = acpi_driver_data(device);
> > +	if (!button)
> > +		return -EINVAL;
> > +	switch (button->type) {
> > +	case ACPI_BUTTON_TYPE_SLEEPF:
> > +		return acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> > +					       acpi_button_notify_fixed);
> > +
> > +	case ACPI_BUTTON_TYPE_POWERF:
> > +		return acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> > +					       acpi_button_notify_fixed);
> > +	}
> >  	return 0;
> >  }
> >  
> > 
> 
> 
>