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@xxxxxxx>
> >
> > 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;
> > }
> >
> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
