Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8d043ad3-e117-7bc6-d18e-6b7f17bf4de7@intel.com>
Date:   Fri, 9 Jun 2023 19:44:57 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [RFT][PATCH v1 1/4] ACPI: button: Eliminate the driver notify
 callback
Content-Language: en-US
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>
CC:     LKML <linux-kernel@vger.kernel.org>
References: <1847933.atdPhlSkOF@kreacher> <2176493.Icojqenx9y@kreacher>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <2176493.Icojqenx9y@kreacher>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?di9iQ3NtSllzelZReXRJdTNITVNBc0o5TTVrMCtWN0Q5UEdwYVhPeUtYUGp4?=
 =?utf-8?B?Q1E2ZFVPTGM5ekw1TEp1WGxSRVcwZmUzZU1RT1UvZE00KzJ3Vk9HT2dzS1dB?=
 =?utf-8?B?aGhXeE1Oa0UzUmdaU0VZakQ4T0gyckcvWWdXWm4ycVZvWFlPWG91LzhDS1FH?=
 =?utf-8?B?YTNFL1BFdEh5QUh4c3NlQjFucWUyZVFXcUUwYktDMmRDdWZUMHArUS8ybUNI?=
 =?utf-8?B?amQralRaNnJxUUlzN1o0WFJrR0FoQmZCRHlTZHBOWTNFZldUWnhTeDAyUUp0?=
 =?utf-8?B?dGprR2RyaXN6QzNCOVJIR3FWa09ydVBZLzl3SVhVWjVkSFpFQmhacktZMjFq?=
 =?utf-8?B?eDQvYTdzNEpzbllLbEZmdFZNUVEzVnRia3dqbzZ0MVNGL253a3ZqNWtMcXV6?=
 =?utf-8?B?QXhlYWdFWUc0ZE1WdWR5bldoUzVLc3BnYjFTS0tXTW5jeEFvQzAwSTRZOEVo?=
 =?utf-8?B?SHBlMVRDdm5Kd0puSTc5RHptSWtuRTVXM3JyM3ZKYTVNak5QbTQ4YTRyaldW?=
 =?utf-8?B?cU4vSjhuUVQrb2l3L09TeWUwRXd0NUlCNnN6K1lBTW5HVE9yYTlOLzFsaFUr?=
 =?utf-8?B?YUIzcjhpKzgxUjJ4amVYb2tZdUltTmRoa0oyU3ZxN1VNdjJrT0dwNnhoTVp6?=
 =?utf-8?B?eGJ1bW9Sc1JkczBHV3JaQWI1UGJaa3E0ajhqUkZDR0JQK0toV2JkSlBDZ3RG?=
 =?utf-8?B?eDQ2S09BZCtkQ2tjUm9PM3BOMWFMeHFlcGdodVd1NllDeXBmZGIxTndFaDF5?=
 =?utf-8?B?YmI3T3JsR0FMZmV4VUZ4MFRPcjJqdUQrc1B2TjduTDdBWWNuNmtzSHZkSzZ0?=
 =?utf-8?B?R3NXZ2RSdVNvZ3F5ZStucnB1cmhHZVVlQml0ZHBPME5CT2pKL0dkbHZKd3ha?=
 =?utf-8?B?RWNYOEhlcHlERUFWRUNBSUJjeVJZVjBTVmp4MFBvcklvbDdreWV1cFM2UGNL?=
 =?utf-8?B?ZDE3UFE3Rnlha0E2aGh2Y3Vjd2tFYjI4QVI1MVQrQ0pLRTNTcFR1ay9LS29L?=
 =?utf-8?B?OFg2RjREL1NTMlowL0dpd0dINGdMRGZJUmc2eVRZZTA0TU9TRDlWakk1dDlK?=
 =?utf-8?B?SC9BMkRQT1FIYWdSQUhpZ0dWSXlKK2FyckxHSzJicldTWWNpUCtSc25hZy9a?=
 =?utf-8?B?OGxUdG8xRlkwNitWemVPZWhTeENjZGFVUVFBdU5xU3NjQk1lcUZiSlB1cmhl?=
 =?utf-8?B?Y2NiRXFoeU5tWmp5L0dxQ05DeThQWlNST0xZb0Zmd29ITFVGUVNmQ0w5OWVy?=
 =?utf-8?B?SmtIWW1WRUcvUmN0ZHE5NWZWSTNyRi9lbkxRRjkvMHcyNjFmS28rWW5teTFq?=
 =?utf-8?B?YlEvTlFkWWZyYk8rRzJXMHk4SlltRkFxTEFJOHRBK244dUY2QjlXN2ZuaXJm?=
 =?utf-8?B?WHNQcy9ERTR5RHptZll4Vno0alhuZFk2WmJEUEhPVEJod0dDVzNicXRXOEZ5?=
 =?utf-8?B?ZFcvdWkraGxuZGJwRW9nMDl4OXQ0OGcyTXcvVjI5b0hjSkJBUzJ3bitMand6?=
 =?utf-8?B?MW4xYzdDSkZaTU56YjZYbWtabU4zVmdxYmdwSjgvenJZTGlSTFhreVFQWXhw?=
 =?utf-8?B?czU2aitUZzZxbE1YVVY5eGRISFVOaVFmT00vUTRNVERjUVd5K3lmVnFoVjNl?=
 =?utf-8?B?U1NYcFRYOFhjVXluOXNLZ2xJMldIV0oxdmV3eG1oTHdjbi90VWZwMmFISWFC?=
 =?utf-8?B?d2Viemw2RVdLRjk5U3Bvc09kT3pCd2lhNlJuNUNHU2NZK2x5MGc2SkpiMXA1?=
 =?utf-8?B?enpVYnlUSHBBK1hpalpJWXA0c0daazRRNEpHQnF0TUFjYXRoREEyV1hyRWRa?=
 =?utf-8?B?aDZJM1VrOU4xSExERFQ3aDlodnNTaVBMaHVUV09CQUYwTGJZTDZDT3NWNFlz?=
 =?utf-8?B?THNyUHQwNGhyTU92eW5rejJqN2g1ZC9EQ2NldS9qQkd2d2kwTFB3aEprRGZO?=
 =?utf-8?B?NzJUZlVOTlRmYzFXRUdNTXNYRmI5aXR5VHFEQU9DbEl3bEJyRVdmZzZVUXpX?=
 =?utf-8?B?a1ZWRGdKNkQvcFo5SWZ1RzMyWHBQR1h1RmVuTkZ6UWZqem9NcE9vY0hteFc5?=
 =?utf-8?B?SEM2YTNYaXhIV29rQ1BYVk53bG9DN3RRT21EL29BTjllUVZnSGhOL2MxNHZR?=
 =?utf-8?B?M0hjZVBpSUxnRkJlMG13SGpwbnl3NG1WNTJWYUJhZVhKYTJSZTVyVWRVaXdQ?=
 =?utf-8?B?OHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7ad85273-9f09-4139-4f9e-08db69113f61
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2023 17:45:00.4192
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: A78Fe83NySACqvilTg/xd/yNqGr+CC2gvRoNRzVbF3PT1XHXYdGJnumNYotP6/Be+/hZ68qrSa5NlNiIgIlVzElXjdD14r+jsUIHP9+NNm8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7637
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/4/2023 5:19 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>  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@intel.com>