On 6/4/2023 5:19 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Rework the ACPI button driver to install notify handlers or fixed
> event handlers for the devices it binds to by itself, reduce the
> indentation level in its notify handler routine and drop its
> notify callback.
>
> This will allow acpi_device_install_notify_handler() and
> acpi_device_remove_notify_handler() to be simplified going forward
> and it will allow the driver to use different notify handlers for the
> lid and for the power and sleep buttons.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> drivers/acpi/button.c | 140 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 96 insertions(+), 44 deletions(-)
>
> Index: linux-pm/drivers/acpi/button.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/button.c
> +++ linux-pm/drivers/acpi/button.c
> @@ -135,7 +135,6 @@ static const struct dmi_system_id dmi_li
>
> static int acpi_button_add(struct acpi_device *device);
> static void acpi_button_remove(struct acpi_device *device);
> -static void acpi_button_notify(struct acpi_device *device, u32 event);
>
> #ifdef CONFIG_PM_SLEEP
> static int acpi_button_suspend(struct device *dev);
> @@ -153,7 +152,6 @@ static struct acpi_driver acpi_button_dr
> .ops = {
> .add = acpi_button_add,
> .remove = acpi_button_remove,
> - .notify = acpi_button_notify,
> },
> .drv.pm = &acpi_button_pm,
> };
> @@ -409,45 +407,55 @@ static void acpi_lid_initialize_state(st
> button->lid_state_initialized = true;
> }
>
> -static void acpi_button_notify(struct acpi_device *device, u32 event)
> +static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_button *button = acpi_driver_data(device);
> + struct acpi_device *device = data;
> + struct acpi_button *button;
> struct input_dev *input;
> + int keycode;
>
> - switch (event) {
> - case ACPI_FIXED_HARDWARE_EVENT:
> - event = ACPI_BUTTON_NOTIFY_STATUS;
> - fallthrough;
> - case ACPI_BUTTON_NOTIFY_STATUS:
> - input = button->input;
> - if (button->type == ACPI_BUTTON_TYPE_LID) {
> - if (button->lid_state_initialized)
> - acpi_lid_update_state(device, true);
> - } else {
> - int keycode;
> -
> - acpi_pm_wakeup_event(&device->dev);
> - if (button->suspended)
> - break;
> -
> - keycode = test_bit(KEY_SLEEP, input->keybit) ?
> - KEY_SLEEP : KEY_POWER;
> - input_report_key(input, keycode, 1);
> - input_sync(input);
> - input_report_key(input, keycode, 0);
> - input_sync(input);
> -
> - acpi_bus_generate_netlink_event(
> - device->pnp.device_class,
> - dev_name(&device->dev),
> - event, ++button->pushed);
> - }
> - break;
> - default:
> + if (event != ACPI_BUTTON_NOTIFY_STATUS) {
> acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> event);
> - break;
> + return;
> + }
> +
> + button = acpi_driver_data(device);
> +
> + if (button->type == ACPI_BUTTON_TYPE_LID) {
> + if (button->lid_state_initialized)
> + acpi_lid_update_state(device, true);
> +
> + return;
> }
> +
> + acpi_pm_wakeup_event(&device->dev);
> +
> + if (button->suspended)
> + return;
> +
> + input = button->input;
> + keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER;
> +
> + input_report_key(input, keycode, 1);
> + input_sync(input);
> + input_report_key(input, keycode, 0);
> + input_sync(input);
> +
> + acpi_bus_generate_netlink_event(device->pnp.device_class,
> + dev_name(&device->dev),
> + event, ++button->pushed);
> +}
> +
> +static void acpi_button_notify_run(void *data)
> +{
> + acpi_button_notify(NULL, ACPI_BUTTON_NOTIFY_STATUS, data);
> +}
> +
> +static u32 acpi_button_event(void *data)
> +{
> + acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
> + return ACPI_INTERRUPT_HANDLED;
> }
>
> #ifdef CONFIG_PM_SLEEP
> @@ -492,8 +500,9 @@ static int acpi_button_add(struct acpi_d
> struct acpi_button *button;
> struct input_dev *input;
> const char *hid = acpi_device_hid(device);
> + acpi_status status;
> char *name, *class;
> - int error;
> + int error = 0;
>
> if (!strcmp(hid, ACPI_BUTTON_HID_LID) &&
> lid_init_state == ACPI_BUTTON_LID_INIT_DISABLED)
> @@ -535,12 +544,15 @@ static int acpi_button_add(struct acpi_d
> } else {
> pr_info("Unsupported hid [%s]\n", hid);
> error = -ENODEV;
...
> - goto err_free_input;
> }
>
> - error = acpi_button_add_fs(device);
> - if (error)
> - goto err_free_input;
> + if (!error)
> + error = acpi_button_add_fs(device);
> +
> + if (error) {
> + input_free_device(input);
> + goto err_free_button;
> + }
This logic is correct, just a bit weird to read and it's saving just one call to input_free_device().
>
> snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid);
>
> @@ -568,6 +580,30 @@ static int acpi_button_add(struct acpi_d
> error = input_register_device(input);
> if (error)
> goto err_remove_fs;
> +
> + switch (device->device_type) {
> + case ACPI_BUS_TYPE_POWER_BUTTON:
> + status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> + acpi_button_event,
> + device);
> + break;
> + case ACPI_BUS_TYPE_SLEEP_BUTTON:
> + status = acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> + acpi_button_event,
> + device);
> + break;
> + default:
> + status = acpi_install_notify_handler(device->handle,
> + ACPI_DEVICE_NOTIFY,
> + acpi_button_notify,
> + device);
> + break;
> + }
> + if (ACPI_FAILURE(status)) {
> + error = -ENODEV;
> + goto err_input_unregister;
> + }
> +
> if (button->type == ACPI_BUTTON_TYPE_LID) {
> /*
> * This assumes there's only one lid device, or if there are
> @@ -580,11 +616,11 @@ static int acpi_button_add(struct acpi_d
> pr_info("%s [%s]\n", name, acpi_device_bid(device));
> return 0;
>
> - err_remove_fs:
> +err_input_unregister:
> + input_unregister_device(input);
> +err_remove_fs:
> acpi_button_remove_fs(device);
> - err_free_input:
> - input_free_device(input);
> - err_free_button:
> +err_free_button:
> kfree(button);
> return error;
> }
> @@ -593,6 +629,22 @@ static void acpi_button_remove(struct ac
> {
> struct acpi_button *button = acpi_driver_data(device);
>
> + switch (device->device_type) {
> + case ACPI_BUS_TYPE_POWER_BUTTON:
> + acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> + acpi_button_event);
> + break;
> + case ACPI_BUS_TYPE_SLEEP_BUTTON:
> + acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> + acpi_button_event);
> + break;
> + default:
> + acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> + acpi_button_notify);
> + break;
> + }
> + acpi_os_wait_events_complete();
> +
> acpi_button_remove_fs(device);
> input_unregister_device(button->input);
> kfree(button);
>
>
Reviewed-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
