Re: [PATCH v4 20/35] platform/x86/eeepc-laptop: Move handler installing logic to driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 1 Jun 2023, Michal Wilczynski wrote:

> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
> 
> Call acpi_device_install_event_handler() at the end of .add() callback.
> Call acpi_device_remove_event_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify callback to match with
> what's required by acpi_device_install_event_handler().
> 
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 62b71e8e3567..bd6ada963d88 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -1204,12 +1204,15 @@ static void eeepc_input_notify(struct eeepc_laptop *eeepc, int event)
>  		pr_info("Unknown key %x pressed\n", event);
>  }
>  
> -static void eeepc_acpi_notify(struct acpi_device *device, u32 event)
> +static void eeepc_acpi_notify(acpi_handle handle, u32 event, void *data)
>  {
> -	struct eeepc_laptop *eeepc = acpi_driver_data(device);
>  	int old_brightness, new_brightness;
> +	struct acpi_device *device = data;
> +	struct eeepc_laptop *eeepc;
>  	u16 count;
>  
> +	eeepc = acpi_driver_data(device);
> +
>  	if (event > ACPI_MAX_SYS_NOTIFY)
>  		return;
>  	count = eeepc->event_count[event % 128]++;
> @@ -1423,6 +1426,11 @@ static int eeepc_acpi_add(struct acpi_device *device)
>  		goto fail_rfkill;
>  
>  	eeepc_device_present = true;
> +	result = acpi_device_install_event_handler(device, ACPI_ALL_NOTIFY,
> +						   eeepc_acpi_notify);
> +	if (result)
> +		goto fail_rfkill;

This too is incorrectly rolled back. You shouldn't just copy the previous 
goto label like this but think what _more_ should be rolled back if that 
preceeding call succeeded. Usually, the remove function gives you good 
hint on what additional things should be rolled back. In here it looks 
like eeepc_rfkill_exit() would be needed too.

> +
>  	return 0;
>  
>  fail_rfkill:
> @@ -1444,6 +1452,8 @@ static void eeepc_acpi_remove(struct acpi_device *device)
>  {
>  	struct eeepc_laptop *eeepc = acpi_driver_data(device);
>  
> +	acpi_device_remove_event_handler(device, ACPI_ALL_NOTIFY,
> +					 eeepc_acpi_notify);
>  	eeepc_backlight_exit(eeepc);
>  	eeepc_rfkill_exit(eeepc);
>  	eeepc_input_exit(eeepc);


-- 
 i.