Re: [RFT][PATCH v1 1/4] ACPI: button: Eliminate the driver notify callback

[Wilczynski, Michal]

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>