On 6/2/2023 12:29 PM, Thadeu Lima de Souza Cascardo wrote:
> On Thu, Jun 01, 2023 at 03:17:21PM +0200, 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@xxxxxxxxx>
>> ---
>> drivers/platform/x86/classmate-laptop.c | 53 +++++++++++++++++++------
>> 1 file changed, 41 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
>> index 2edaea2492df..2d36abf5ecfe 100644
>> --- a/drivers/platform/x86/classmate-laptop.c
>> +++ b/drivers/platform/x86/classmate-laptop.c
>> @@ -180,8 +180,9 @@ static acpi_status cmpc_get_accel_v4(acpi_handle handle,
>> return status;
>> }
>>
>> -static void cmpc_accel_handler_v4(struct acpi_device *dev, u32 event)
>> +static void cmpc_accel_handler_v4(acpi_handle handle, u32 event, void *data)
>> {
>> + struct acpi_device *dev = data;
>> if (event == 0x81) {
>> int16_t x, y, z;
>> acpi_status status;
>> @@ -407,6 +408,11 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi)
>> inputdev = dev_get_drvdata(&acpi->dev);
>> dev_set_drvdata(&inputdev->dev, accel);
>>
>> + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY,
>> + cmpc_accel_handler_v4);
>> + if (error)
>> + goto failed_input;
>> +
> In all cases, acpi_device_install_event_handler is being called after
> cmpc_add_acpi_notify_device, which allocates and registers an input
> device.
>
> You should cleanup in case acpi_device_install_event_handler fails and
> call cmpc_remove_acpi_notify_device.
>
> Cascardo.
Hi Cascardo
Yeah I see this now, I'm not freeing the resource in the error path properly,
will add another label for example 'failed_notify_install' that would call
cmpc_remove_acpi_notify_device,
Thanks !
Michał
>
>> return 0;
>>
>> failed_input:
>> @@ -420,6 +426,7 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi)
>>
>> static void cmpc_accel_remove_v4(struct acpi_device *acpi)
>> {
>> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler_v4);
>> device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr_v4);
>> device_remove_file(&acpi->dev, &cmpc_accel_g_select_attr_v4);
>> cmpc_remove_acpi_notify_device(acpi);
>> @@ -441,7 +448,6 @@ static struct acpi_driver cmpc_accel_acpi_driver_v4 = {
>> .ops = {
>> .add = cmpc_accel_add_v4,
>> .remove = cmpc_accel_remove_v4,
>> - .notify = cmpc_accel_handler_v4,
>> },
>> .drv.pm = &cmpc_accel_pm,
>> };
>> @@ -523,8 +529,10 @@ static acpi_status cmpc_get_accel(acpi_handle handle,
>> return status;
>> }
>>
>> -static void cmpc_accel_handler(struct acpi_device *dev, u32 event)
>> +static void cmpc_accel_handler(acpi_handle handle, u32 event, void *data)
>> {
>> + struct acpi_device *dev = data;
>> +
>> if (event == 0x81) {
>> unsigned char x, y, z;
>> acpi_status status;
>> @@ -639,6 +647,11 @@ static int cmpc_accel_add(struct acpi_device *acpi)
>> inputdev = dev_get_drvdata(&acpi->dev);
>> dev_set_drvdata(&inputdev->dev, accel);
>>
>> + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY,
>> + cmpc_accel_handler);
>> + if (error)
>> + goto failed_input;
>> +
>> return 0;
>>
>> failed_input:
>> @@ -650,6 +663,7 @@ static int cmpc_accel_add(struct acpi_device *acpi)
>>
>> static void cmpc_accel_remove(struct acpi_device *acpi)
>> {
>> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler);
>> device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr);
>> cmpc_remove_acpi_notify_device(acpi);
>> }
>> @@ -667,7 +681,6 @@ static struct acpi_driver cmpc_accel_acpi_driver = {
>> .ops = {
>> .add = cmpc_accel_add,
>> .remove = cmpc_accel_remove,
>> - .notify = cmpc_accel_handler,
>> }
>> };
>>
>> @@ -693,8 +706,9 @@ static acpi_status cmpc_get_tablet(acpi_handle handle,
>> return status;
>> }
>>
>> -static void cmpc_tablet_handler(struct acpi_device *dev, u32 event)
>> +static void cmpc_tablet_handler(acpi_handle handle, u32 event, void *data)
>> {
>> + struct acpi_device *dev = data;
>> unsigned long long val = 0;
>> struct input_dev *inputdev = dev_get_drvdata(&dev->dev);
>>
>> @@ -723,12 +737,20 @@ static void cmpc_tablet_idev_init(struct input_dev *inputdev)
>>
>> static int cmpc_tablet_add(struct acpi_device *acpi)
>> {
>> - return cmpc_add_acpi_notify_device(acpi, "cmpc_tablet",
>> - cmpc_tablet_idev_init);
>> + int ret;
>> +
>> + ret = cmpc_add_acpi_notify_device(acpi, "cmpc_tablet",
>> + cmpc_tablet_idev_init);
>> + if (ret)
>> + return ret;
>> +
>> + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY,
>> + cmpc_tablet_handler);
>> }
>>
>> static void cmpc_tablet_remove(struct acpi_device *acpi)
>> {
>> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_tablet_handler);
>> cmpc_remove_acpi_notify_device(acpi);
>> }
>>
>> @@ -761,7 +783,6 @@ static struct acpi_driver cmpc_tablet_acpi_driver = {
>> .ops = {
>> .add = cmpc_tablet_add,
>> .remove = cmpc_tablet_remove,
>> - .notify = cmpc_tablet_handler,
>> },
>> .drv.pm = &cmpc_tablet_pm,
>> };
>> @@ -1026,8 +1047,9 @@ static int cmpc_keys_codes[] = {
>> KEY_MAX
>> };
>>
>> -static void cmpc_keys_handler(struct acpi_device *dev, u32 event)
>> +static void cmpc_keys_handler(acpi_handle handle, u32 event, void *data)
>> {
>> + struct acpi_device *dev = data;
>> struct input_dev *inputdev;
>> int code = KEY_MAX;
>>
>> @@ -1049,12 +1071,20 @@ static void cmpc_keys_idev_init(struct input_dev *inputdev)
>>
>> static int cmpc_keys_add(struct acpi_device *acpi)
>> {
>> - return cmpc_add_acpi_notify_device(acpi, "cmpc_keys",
>> - cmpc_keys_idev_init);
>> + int error;
>> +
>> + error = cmpc_add_acpi_notify_device(acpi, "cmpc_keys",
>> + cmpc_keys_idev_init);
>> + if (error)
>> + return error;
>> +
>> + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY,
>> + cmpc_keys_handler);
>> }
>>
>> static void cmpc_keys_remove(struct acpi_device *acpi)
>> {
>> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_keys_handler);
>> cmpc_remove_acpi_notify_device(acpi);
>> }
>>
>> @@ -1071,7 +1101,6 @@ static struct acpi_driver cmpc_keys_acpi_driver = {
>> .ops = {
>> .add = cmpc_keys_add,
>> .remove = cmpc_keys_remove,
>> - .notify = cmpc_keys_handler,
>> }
>> };
>>
>> --
>> 2.40.1
>>
