*** IMPORTANT ***
This is part 1 - only drivers in acpi directory to ease up review
process. Rest of the drivers will be handled in separate patchsets.
Currently drivers support ACPI event handlers by defining .notify
callback in acpi_device_ops. This solution is suboptimal as event
handler installer installs intermediary function acpi_notify_device as a
handler in every driver. Also this approach requires extra variable
'flags' for specifying event types that the driver want to subscribe to.
Additionally this is a pre-work required to align acpi_driver with
platform_driver and eventually replace acpi_driver with platform_driver.
Remove .notify callback from the acpi_device_ops. Replace it with each
driver installing and removing it's event handlers.
v5:
- rebased on top of Rafael changes [1], they're not merged yet
- fixed rollback in multiple drivers so they don't leak resources on
failure
- made this part 1, meaning only drivers in acpi directory, rest of
the drivers will be handled in separate patchsets to ease up review
v4:
- added one commit for previously missed driver sony-laptop,
refactored return statements, added NULL check for event installer
v3:
- lkp still reported some failures for eeepc, fujitsu and
toshiba_bluetooth, fix those
v2:
- fix compilation errors for drivers
[1]: https://lore.kernel.org/linux-acpi/1847933.atdPhlSkOF@kreacher/
Michal Wilczynski (10):
acpi/bus: Introduce wrappers for ACPICA event handler install/remove
acpi/bus: Set driver_data to NULL every time .add() fails
acpi/ac: Move handler installing logic to driver
acpi/video: Move handler installing logic to driver
acpi/battery: Move handler installing logic to driver
acpi/hed: Move handler installing logic to driver
acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()
acpi/nfit: Improve terminator line in acpi_nfit_ids
acpi/nfit: Move handler installing logic to driver
acpi/thermal: Move handler installing logic to driver
drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
drivers/acpi/battery.c | 30 ++++++++++++++++++++++++------
drivers/acpi/bus.c | 30 +++++++++++++++++++++++++++++-
drivers/acpi/hed.c | 17 ++++++++++++++---
drivers/acpi/nfit/core.c | 32 ++++++++++++++++++++++----------
drivers/acpi/thermal.c | 28 ++++++++++++++++++++++------
include/acpi/acpi_bus.h | 6 ++++++
8 files changed, 163 insertions(+), 39 deletions(-)
--
2.41.0
To use new style of installing event handlers acpi_nfit_notify() needs
to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
the file, so it can be used inside acpi_nfit_add().
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/nfit/core.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 07204d482968..aff79cbc2190 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
}
EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
+static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
+{
+ device_lock(&adev->dev);
+ __acpi_nfit_notify(&adev->dev, adev->handle, event);
+ device_unlock(&adev->dev);
+}
+
static int acpi_nfit_add(struct acpi_device *adev)
{
struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
}
EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
-static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
-{
- device_lock(&adev->dev);
- __acpi_nfit_notify(&adev->dev, adev->handle, event);
- device_unlock(&adev->dev);
-}
-
static const struct acpi_device_id acpi_nfit_ids[] = {
{ "ACPI0012", 0 },
{ "", 0 },
--
2.41.0
Currently terminator line contains redunant characters. Remove them and
also remove a comma at the end.
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index aff79cbc2190..95930e9d776c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
static const struct acpi_device_id acpi_nfit_ids[] = {
{ "ACPI0012", 0 },
- { "", 0 },
+ {}
};
MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
--
2.41.0
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_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.
While at it, fix lack of whitespaces in .remove() callback.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/battery.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9c67ed02d797..6337e7b1f6e1 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
}
/* Driver Interface */
-static void acpi_battery_notify(struct acpi_device *device, u32 event)
+static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_battery *battery = acpi_driver_data(device);
+ struct acpi_device *device = data;
+ struct acpi_battery *battery;
struct power_supply *old;
+ battery = acpi_driver_data(device);
+
if (!battery)
return;
old = battery->bat;
@@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device *device)
device_init_wakeup(&device->dev, 1);
- return result;
+ result = acpi_dev_install_notify_handler(device,
+ ACPI_ALL_NOTIFY,
+ acpi_battery_notify);
+ if (result)
+ goto fail_deinit_wakup_and_unregister;
+
+ return 0;
+fail_deinit_wakup_and_unregister:
+ device_init_wakeup(&device->dev, 0);
+ unregister_pm_notifier(&battery->pm_nb);
fail:
sysfs_remove_battery(battery);
mutex_destroy(&battery->lock);
mutex_destroy(&battery->sysfs_lock);
kfree(battery);
+
return result;
}
@@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device *device)
if (!device || !acpi_driver_data(device))
return;
- device_init_wakeup(&device->dev, 0);
+
battery = acpi_driver_data(device);
+
+ acpi_dev_remove_notify_handler(device,
+ ACPI_ALL_NOTIFY,
+ acpi_battery_notify);
+
+ device_init_wakeup(&device->dev, 0);
unregister_pm_notifier(&battery->pm_nb);
sysfs_remove_battery(battery);
+
mutex_destroy(&battery->lock);
mutex_destroy(&battery->sysfs_lock);
kfree(battery);
@@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
.name = "battery",
.class = ACPI_BATTERY_CLASS,
.ids = battery_device_ids,
- .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
.ops = {
.add = acpi_battery_add,
.remove = acpi_battery_remove,
- .notify = acpi_battery_notify,
},
.drv.pm = &acpi_battery_pm,
};
--
2.41.0
Most drivers set driver_data during .add() callback, but usually
they don't set it back to NULL in case of a failure. Set driver_data to
NULL in acpi_device_probe() to avoid code duplication.
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 22468589c551..c1cb570c8d8c 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1017,8 +1017,10 @@ static int acpi_device_probe(struct device *dev)
return -ENOSYS;
ret = acpi_drv->ops.add(acpi_dev);
- if (ret)
+ if (ret) {
+ acpi_dev->driver_data = NULL;
return ret;
+ }
pr_debug("Driver [%s] successfully bound to device [%s]\n",
acpi_drv->name, acpi_dev->pnp.bus_id);
--
2.41.0
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_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.
While at it, fix whitespaces in .remove() callback and move tz
assignment upwards.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/thermal.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index f9f6ebb08fdb..84716e4b967c 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -825,9 +825,12 @@ static void acpi_queue_thermal_check(struct acpi_thermal *tz)
queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work);
}
-static void acpi_thermal_notify(struct acpi_device *device, u32 event)
+static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_thermal *tz = acpi_driver_data(device);
+ struct acpi_device *device = data;
+ struct acpi_thermal *tz;
+
+ tz = acpi_driver_data(device);
if (!tz)
return;
@@ -997,11 +1000,20 @@ static int acpi_thermal_add(struct acpi_device *device)
pr_info("%s [%s] (%ld C)\n", acpi_device_name(device),
acpi_device_bid(device), deci_kelvin_to_celsius(tz->temperature));
- goto end;
+ result = acpi_dev_install_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_thermal_notify);
+ if (result)
+ goto flush_wq_and_unregister;
+
+ return 0;
+
+flush_wq_and_unregister:
+ flush_workqueue(acpi_thermal_pm_queue);
+ acpi_thermal_unregister_thermal_zone(tz);
free_memory:
kfree(tz);
-end:
return result;
}
@@ -1012,10 +1024,15 @@ static void acpi_thermal_remove(struct acpi_device *device)
if (!device || !acpi_driver_data(device))
return;
- flush_workqueue(acpi_thermal_pm_queue);
tz = acpi_driver_data(device);
+ acpi_dev_remove_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_thermal_notify);
+
+ flush_workqueue(acpi_thermal_pm_queue);
acpi_thermal_unregister_thermal_zone(tz);
+
kfree(tz);
}
@@ -1078,7 +1095,6 @@ static struct acpi_driver acpi_thermal_driver = {
.ops = {
.add = acpi_thermal_add,
.remove = acpi_thermal_remove,
- .notify = acpi_thermal_notify,
},
.drv.pm = &acpi_thermal_pm,
};
--
2.41.0
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_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 62f4364e4460..60b7013d0009 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
static LIST_HEAD(video_bus_head);
static int acpi_video_bus_add(struct acpi_device *device);
static void acpi_video_bus_remove(struct acpi_device *device);
-static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
+static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
/*
* Indices in the _BCL method response: the first two items are special,
@@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
.ops = {
.add = acpi_video_bus_add,
.remove = acpi_video_bus_remove,
- .notify = acpi_video_bus_notify,
},
};
@@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
acpi_osi_is_win8() ? 0 : 1);
}
-static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
+static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_video_bus *video = acpi_driver_data(device);
+ struct acpi_device *device = data;
+ struct acpi_video_bus *video;
struct input_dev *input;
int keycode = 0;
+ video = acpi_driver_data(device);
+
if (!video || !video->input)
return;
@@ -2053,8 +2055,20 @@ static int acpi_video_bus_add(struct acpi_device *device)
acpi_video_bus_add_notify_handler(video);
+ error = acpi_dev_install_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_bus_notify);
+ if (error)
+ goto err_remove_and_unregister_video;
+
return 0;
+err_remove_and_unregister_video:
+ mutex_lock(&video_list_lock);
+ list_del(&video->entry);
+ mutex_unlock(&video_list_lock);
+ acpi_video_bus_remove_notify_handler(video);
+ acpi_video_bus_unregister_backlight(video);
err_put_video:
acpi_video_bus_put_devices(video);
kfree(video->attached_array);
@@ -2075,6 +2089,10 @@ static void acpi_video_bus_remove(struct acpi_device *device)
video = acpi_driver_data(device);
+ acpi_dev_remove_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_video_bus_notify);
+
mutex_lock(&video_list_lock);
list_del(&video->entry);
mutex_unlock(&video_list_lock);
--
2.41.0
Introduce new acpi_dev_install_notify_handler() and
acpi_dev_remove_notify_handler(). Those functions are replacing old
installers, and after all drivers switch to the new model, old installers
will be removed.
Make acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler()
non-static, and export symbols. This will allow the drivers to call them
directly, instead of relying on .notify callback.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/bus.c | 26 ++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 6 ++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b6ab3608d782..22468589c551 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -557,6 +557,32 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device,
acpi_os_wait_events_complete();
}
+int acpi_dev_install_notify_handler(struct acpi_device *adev,
+ u32 handler_type,
+ acpi_notify_handler handler)
+{
+ acpi_status status;
+
+ status = acpi_install_notify_handler(adev->handle,
+ handler_type,
+ handler,
+ adev);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ return 0;
+}
+EXPORT_SYMBOL(acpi_dev_install_notify_handler);
+
+void acpi_dev_remove_notify_handler(struct acpi_device *adev,
+ u32 handler_type,
+ acpi_notify_handler handler)
+{
+ acpi_remove_notify_handler(adev->handle, handler_type, handler);
+ acpi_os_wait_events_complete();
+}
+EXPORT_SYMBOL(acpi_dev_remove_notify_handler);
+
/* Handle events targeting \_SB device (at present only graceful shutdown) */
#define ACPI_SB_NOTIFY_SHUTDOWN_REQUEST 0x81
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index c941d99162c0..23fbe4a16972 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -515,6 +515,12 @@ void acpi_bus_private_data_handler(acpi_handle, void *);
int acpi_bus_get_private_data(acpi_handle, void **);
int acpi_bus_attach_private_data(acpi_handle, void *);
void acpi_bus_detach_private_data(acpi_handle);
+int acpi_dev_install_notify_handler(struct acpi_device *adev,
+ u32 handler_type,
+ acpi_notify_handler handler);
+void acpi_dev_remove_notify_handler(struct acpi_device *adev,
+ u32 handler_type,
+ acpi_notify_handler handler);
extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
extern int register_acpi_notifier(struct notifier_block *);
extern int unregister_acpi_notifier(struct notifier_block *);
--
2.41.0
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_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/hed.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/hed.c b/drivers/acpi/hed.c
index 78d44e3fe129..8f54560c6d1c 100644
--- a/drivers/acpi/hed.c
+++ b/drivers/acpi/hed.c
@@ -42,22 +42,34 @@ EXPORT_SYMBOL_GPL(unregister_acpi_hed_notifier);
* it is used by HEST Generic Hardware Error Source with notify type
* SCI.
*/
-static void acpi_hed_notify(struct acpi_device *device, u32 event)
+static void acpi_hed_notify(acpi_handle handle, u32 event, void *data)
{
blocking_notifier_call_chain(&acpi_hed_notify_list, 0, NULL);
}
static int acpi_hed_add(struct acpi_device *device)
{
+ int err;
+
/* Only one hardware error device */
if (hed_handle)
return -EINVAL;
hed_handle = device->handle;
- return 0;
+
+ err = acpi_dev_install_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_hed_notify);
+ if (err)
+ hed_handle = NULL;
+
+ return err;
}
static void acpi_hed_remove(struct acpi_device *device)
{
+ acpi_dev_remove_notify_handler(device,
+ ACPI_DEVICE_NOTIFY,
+ acpi_hed_notify);
hed_handle = NULL;
}
@@ -68,7 +80,6 @@ static struct acpi_driver acpi_hed_driver = {
.ops = {
.add = acpi_hed_add,
.remove = acpi_hed_remove,
- .notify = acpi_hed_notify,
},
};
module_acpi_driver(acpi_hed_driver);
--
2.41.0
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> 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_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> While at it, fix lack of whitespaces in .remove() callback.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/battery.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 9c67ed02d797..6337e7b1f6e1 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
> }
>
> /* Driver Interface */
> -static void acpi_battery_notify(struct acpi_device *device, u32 event)
> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_battery *battery = acpi_driver_data(device);
> + struct acpi_device *device = data;
> + struct acpi_battery *battery;
> struct power_supply *old;
>
> + battery = acpi_driver_data(device);
> +
> if (!battery)
> return;
> old = battery->bat;
> @@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device *device)
>
> device_init_wakeup(&device->dev, 1);
>
> - return result;
> + result = acpi_dev_install_notify_handler(device,
> + ACPI_ALL_NOTIFY,
> + acpi_battery_notify);
> + if (result)
> + goto fail_deinit_wakup_and_unregister;
You could call the label "fail_pm", for example, which would be more
concise and so slightly easier to follow, without any loss of clarity
AFAICS.
> +
> + return 0;
>
> +fail_deinit_wakup_and_unregister:
> + device_init_wakeup(&device->dev, 0);
> + unregister_pm_notifier(&battery->pm_nb);
> fail:
> sysfs_remove_battery(battery);
> mutex_destroy(&battery->lock);
> mutex_destroy(&battery->sysfs_lock);
> kfree(battery);
> +
> return result;
> }
>
> @@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device *device)
>
> if (!device || !acpi_driver_data(device))
> return;
> - device_init_wakeup(&device->dev, 0);
> +
> battery = acpi_driver_data(device);
> +
> + acpi_dev_remove_notify_handler(device,
> + ACPI_ALL_NOTIFY,
> + acpi_battery_notify);
> +
> + device_init_wakeup(&device->dev, 0);
> unregister_pm_notifier(&battery->pm_nb);
> sysfs_remove_battery(battery);
> +
> mutex_destroy(&battery->lock);
> mutex_destroy(&battery->sysfs_lock);
> kfree(battery);
> @@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
> .name = "battery",
> .class = ACPI_BATTERY_CLASS,
> .ids = battery_device_ids,
> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> .ops = {
> .add = acpi_battery_add,
> .remove = acpi_battery_remove,
> - .notify = acpi_battery_notify,
> },
> .drv.pm = &acpi_battery_pm,
> };
> --
> 2.41.0
>
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> 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_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 62f4364e4460..60b7013d0009 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
> static LIST_HEAD(video_bus_head);
> static int acpi_video_bus_add(struct acpi_device *device);
> static void acpi_video_bus_remove(struct acpi_device *device);
> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
>
> /*
> * Indices in the _BCL method response: the first two items are special,
> @@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
> .ops = {
> .add = acpi_video_bus_add,
> .remove = acpi_video_bus_remove,
> - .notify = acpi_video_bus_notify,
> },
> };
>
> @@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
> acpi_osi_is_win8() ? 0 : 1);
> }
>
> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_video_bus *video = acpi_driver_data(device);
> + struct acpi_device *device = data;
> + struct acpi_video_bus *video;
> struct input_dev *input;
> int keycode = 0;
>
> + video = acpi_driver_data(device);
> +
> if (!video || !video->input)
> return;
>
> @@ -2053,8 +2055,20 @@ static int acpi_video_bus_add(struct acpi_device *device)
>
> acpi_video_bus_add_notify_handler(video);
>
> + error = acpi_dev_install_notify_handler(device,
> + ACPI_DEVICE_NOTIFY,
> + acpi_video_bus_notify);
> + if (error)
> + goto err_remove_and_unregister_video;
This label name is a bit too long and the second half of it doesn't
really add any value IMV. err_remove would be sufficient.
> +
> return 0;
>
> +err_remove_and_unregister_video:
> + mutex_lock(&video_list_lock);
> + list_del(&video->entry);
> + mutex_unlock(&video_list_lock);
> + acpi_video_bus_remove_notify_handler(video);
> + acpi_video_bus_unregister_backlight(video);
> err_put_video:
> acpi_video_bus_put_devices(video);
> kfree(video->attached_array);
> @@ -2075,6 +2089,10 @@ static void acpi_video_bus_remove(struct acpi_device *device)
>
> video = acpi_driver_data(device);
>
> + acpi_dev_remove_notify_handler(device,
> + ACPI_DEVICE_NOTIFY,
> + acpi_video_bus_notify);
> +
> mutex_lock(&video_list_lock);
> list_del(&video->entry);
> mutex_unlock(&video_list_lock);
> --
> 2.41.0
>
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> wrote:
>
> To use new style of installing event handlers acpi_nfit_notify() needs
> to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
> the file, so it can be used inside acpi_nfit_add().
>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 07204d482968..aff79cbc2190 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +{
> + device_lock(&adev->dev);
> + __acpi_nfit_notify(&adev->dev, adev->handle, event);
> + device_unlock(&adev->dev);
> +}
> +
> static int acpi_nfit_add(struct acpi_device *adev)
> {
> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
> }
> EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> -{
> - device_lock(&adev->dev);
> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> - device_unlock(&adev->dev);
> -}
> -
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> { "", 0 },
> --
Please fold this patch into the next one. By itself, it is an
artificial change IMV.
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> wrote:
>
> Currently terminator line contains redunant characters.
Well, they are terminating the list properly AFAICS, so they aren't
redundant and the size of it before and after the change is actually
the same, isn't it?
> Remove them and also remove a comma at the end.
I suppose that this change is made for consistency with the other ACPI
code, so this would be the motivation to give in the changelog.
In any case, it doesn't seem to be related to the rest of the series.
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index aff79cbc2190..95930e9d776c 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> - { "", 0 },
> + {}
> };
> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> --
> 2.41.0
>
Michal Wilczynski wrote:
> Currently terminator line contains redunant characters. Remove them and
> also remove a comma at the end.
>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index aff79cbc2190..95930e9d776c 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> - { "", 0 },
> + {}
Looks like a pointless change to me.
On 6/29/2023 5:58 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> 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_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki <[email protected]>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
>> 1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 62f4364e4460..60b7013d0009 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
>> static LIST_HEAD(video_bus_head);
>> static int acpi_video_bus_add(struct acpi_device *device);
>> static void acpi_video_bus_remove(struct acpi_device *device);
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
>>
>> /*
>> * Indices in the _BCL method response: the first two items are special,
>> @@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
>> .ops = {
>> .add = acpi_video_bus_add,
>> .remove = acpi_video_bus_remove,
>> - .notify = acpi_video_bus_notify,
>> },
>> };
>>
>> @@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
>> acpi_osi_is_win8() ? 0 : 1);
>> }
>>
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_video_bus *video = acpi_driver_data(device);
>> + struct acpi_device *device = data;
>> + struct acpi_video_bus *video;
>> struct input_dev *input;
>> int keycode = 0;
>>
>> + video = acpi_driver_data(device);
>> +
>> if (!video || !video->input)
>> return;
>>
>> @@ -2053,8 +2055,20 @@ static int acpi_video_bus_add(struct acpi_device *device)
>>
>> acpi_video_bus_add_notify_handler(video);
>>
>> + error = acpi_dev_install_notify_handler(device,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_video_bus_notify);
>> + if (error)
>> + goto err_remove_and_unregister_video;
> This label name is a bit too long and the second half of it doesn't
> really add any value IMV. err_remove would be sufficient.
I've seen different patterns in the code, sometimes the label describe what failed,
sometimes it describe what needs to be cleaned up. I don't really have a strong
preference, just thought it might be useful to the reader. Will change as suggested.
>
>> +
>> return 0;
>>
>> +err_remove_and_unregister_video:
>> + mutex_lock(&video_list_lock);
>> + list_del(&video->entry);
>> + mutex_unlock(&video_list_lock);
>> + acpi_video_bus_remove_notify_handler(video);
>> + acpi_video_bus_unregister_backlight(video);
>> err_put_video:
>> acpi_video_bus_put_devices(video);
>> kfree(video->attached_array);
>> @@ -2075,6 +2089,10 @@ static void acpi_video_bus_remove(struct acpi_device *device)
>>
>> video = acpi_driver_data(device);
>>
>> + acpi_dev_remove_notify_handler(device,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_video_bus_notify);
>> +
>> mutex_lock(&video_list_lock);
>> list_del(&video->entry);
>> mutex_unlock(&video_list_lock);
>> --
>> 2.41.0
>>
On 6/29/2023 6:05 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> 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_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> While at it, fix lack of whitespaces in .remove() callback.
>>
>> Suggested-by: Rafael J. Wysocki <[email protected]>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/battery.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 9c67ed02d797..6337e7b1f6e1 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
>> }
>>
>> /* Driver Interface */
>> -static void acpi_battery_notify(struct acpi_device *device, u32 event)
>> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_battery *battery = acpi_driver_data(device);
>> + struct acpi_device *device = data;
>> + struct acpi_battery *battery;
>> struct power_supply *old;
>>
>> + battery = acpi_driver_data(device);
>> +
>> if (!battery)
>> return;
>> old = battery->bat;
>> @@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device *device)
>>
>> device_init_wakeup(&device->dev, 1);
>>
>> - return result;
>> + result = acpi_dev_install_notify_handler(device,
>> + ACPI_ALL_NOTIFY,
>> + acpi_battery_notify);
>> + if (result)
>> + goto fail_deinit_wakup_and_unregister;
> You could call the label "fail_pm", for example, which would be more
> concise and so slightly easier to follow, without any loss of clarity
> AFAICS.
Sure
>
>> +
>> + return 0;
>>
>> +fail_deinit_wakup_and_unregister:
>> + device_init_wakeup(&device->dev, 0);
>> + unregister_pm_notifier(&battery->pm_nb);
>> fail:
>> sysfs_remove_battery(battery);
>> mutex_destroy(&battery->lock);
>> mutex_destroy(&battery->sysfs_lock);
>> kfree(battery);
>> +
>> return result;
>> }
>>
>> @@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device *device)
>>
>> if (!device || !acpi_driver_data(device))
>> return;
>> - device_init_wakeup(&device->dev, 0);
>> +
>> battery = acpi_driver_data(device);
>> +
>> + acpi_dev_remove_notify_handler(device,
>> + ACPI_ALL_NOTIFY,
>> + acpi_battery_notify);
>> +
>> + device_init_wakeup(&device->dev, 0);
>> unregister_pm_notifier(&battery->pm_nb);
>> sysfs_remove_battery(battery);
>> +
>> mutex_destroy(&battery->lock);
>> mutex_destroy(&battery->sysfs_lock);
>> kfree(battery);
>> @@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
>> .name = "battery",
>> .class = ACPI_BATTERY_CLASS,
>> .ids = battery_device_ids,
>> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>> .ops = {
>> .add = acpi_battery_add,
>> .remove = acpi_battery_remove,
>> - .notify = acpi_battery_notify,
>> },
>> .drv.pm = &acpi_battery_pm,
>> };
>> --
>> 2.41.0
>>
On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> wrote:
>> To use new style of installing event handlers acpi_nfit_notify() needs
>> to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
>> the file, so it can be used inside acpi_nfit_add().
>>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/nfit/core.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 07204d482968..aff79cbc2190 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>> }
>> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>>
>> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> +{
>> + device_lock(&adev->dev);
>> + __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> + device_unlock(&adev->dev);
>> +}
>> +
>> static int acpi_nfit_add(struct acpi_device *adev)
>> {
>> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>> @@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
>> }
>> EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> -{
>> - device_lock(&adev->dev);
>> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> - device_unlock(&adev->dev);
>> -}
>> -
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> { "", 0 },
>> --
> Please fold this patch into the next one. By itself, it is an
> artificial change IMV.
I agree with you, but I got told specifically to do that.
https://lore.kernel.org/linux-acpi/[email protected]/
On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> wrote:
>> Currently terminator line contains redunant characters.
> Well, they are terminating the list properly AFAICS, so they aren't
> redundant and the size of it before and after the change is actually
> the same, isn't it?
This syntax is correct of course, but we have an internal guidelines specifically
saying that terminator line should NOT contain a comma at the end. Justification:
"Terminator line is established for the data structure arrays which may have unknown,
to the caller, sizes. The purpose of it is to stop iteration over an array and avoid
out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to avoid
potential, but unlike, event of adding the entry after terminator, already at compile time.
This will be achieved by not putting comma at the end of terminator line"
>
>> Remove them and also remove a comma at the end.
> I suppose that this change is made for consistency with the other ACPI
> code, so this would be the motivation to give in the changelog.
>
> In any case, it doesn't seem to be related to the rest of the series.
>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/nfit/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index aff79cbc2190..95930e9d776c 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> - { "", 0 },
>> + {}
>> };
>> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>>
>> --
>> 2.41.0
>>
On 6/29/2023 10:51 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> Currently terminator line contains redunant characters. Remove them and
>> also remove a comma at the end.
>>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/nfit/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index aff79cbc2190..95930e9d776c 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> - { "", 0 },
>> + {}
> Looks like a pointless change to me.
It's not very consequential, but isn't totally pointless in my view:
"Terminator line is established for the data structure arrays which may have unknown,
to the caller, sizes. The purpose of it is to stop iteration over an array and avoid
out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to avoid
potential, but unlike, event of adding the entry after terminator, already at compile time.
This will be achieved by not putting comma at the end of terminator line"
Anyway I can drop this change, it's just confusing everyone
On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
<[email protected]> wrote:
>
>
>
> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <[email protected]> wrote:
> >> Currently terminator line contains redunant characters.
> > Well, they are terminating the list properly AFAICS, so they aren't
> > redundant and the size of it before and after the change is actually
> > the same, isn't it?
>
> This syntax is correct of course, but we have an internal guidelines specifically
> saying that terminator line should NOT contain a comma at the end. Justification:
>
> "Terminator line is established for the data structure arrays which may have unknown,
> to the caller, sizes. The purpose of it is to stop iteration over an array and avoid
> out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to avoid
> potential, but unlike, event of adding the entry after terminator, already at compile time.
> This will be achieved by not putting comma at the end of terminator line"
This certainly applies to any new code.
The existing code, however, is what it is and the question is how much
of an improvement the given change makes.
So yes, it may not follow the current rules for new code, but then it
may not be worth changing to follow these rules anyway.
On Fri, Jun 30, 2023 at 11:48 AM Wilczynski, Michal
<[email protected]> wrote:
>
>
>
> On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <[email protected]> wrote:
> >> To use new style of installing event handlers acpi_nfit_notify() needs
> >> to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
> >> the file, so it can be used inside acpi_nfit_add().
> >>
> >> Signed-off-by: Michal Wilczynski <[email protected]>
> >> ---
> >> drivers/acpi/nfit/core.c | 14 +++++++-------
> >> 1 file changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 07204d482968..aff79cbc2190 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> >> }
> >> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
> >>
> >> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> >> +{
> >> + device_lock(&adev->dev);
> >> + __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> + device_unlock(&adev->dev);
> >> +}
> >> +
> >> static int acpi_nfit_add(struct acpi_device *adev)
> >> {
> >> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> >> @@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
> >> }
> >> EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
> >>
> >> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> >> -{
> >> - device_lock(&adev->dev);
> >> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> - device_unlock(&adev->dev);
> >> -}
> >> -
> >> static const struct acpi_device_id acpi_nfit_ids[] = {
> >> { "ACPI0012", 0 },
> >> { "", 0 },
> >> --
> > Please fold this patch into the next one. By itself, it is an
> > artificial change IMV.
>
> I agree with you, but I got told specifically to do that.
> https://lore.kernel.org/linux-acpi/[email protected]/
Whether or not this is easier to review is kind of subjective.
If there were more code to move, I would agree, but in this particular
case having to review two patches instead of just one is a bit of a
hassle IMV.
On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
> <[email protected]> wrote:
> >
> >
> >
> > On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > > <[email protected]> wrote:
> > >> Currently terminator line contains redunant characters.
> > > Well, they are terminating the list properly AFAICS, so they aren't
> > > redundant and the size of it before and after the change is actually
> > > the same, isn't it?
> >
> > This syntax is correct of course, but we have an internal guidelines specifically
> > saying that terminator line should NOT contain a comma at the end. Justification:
> >
> > "Terminator line is established for the data structure arrays which may have unknown,
> > to the caller, sizes. The purpose of it is to stop iteration over an array and avoid
> > out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to avoid
> > potential, but unlike, event of adding the entry after terminator, already at compile time.
> > This will be achieved by not putting comma at the end of terminator line"
>
> This certainly applies to any new code.
>
> The existing code, however, is what it is and the question is how much
> of an improvement the given change makes.
>
> So yes, it may not follow the current rules for new code, but then it
> may not be worth changing to follow these rules anyway.
This is a bit like housing in a city.
Usually, there are strict requirements that must be followed while
constructing a new building, but existing buildings are not
reconstructed to follow them in the majority of cases. It may not
even be a good idea to do that.
On 6/30/2023 1:13 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki <[email protected]> wrote:
>> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
>> <[email protected]> wrote:
>>>
>>>
>>> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
>>>> <[email protected]> wrote:
>>>>> Currently terminator line contains redunant characters.
>>>> Well, they are terminating the list properly AFAICS, so they aren't
>>>> redundant and the size of it before and after the change is actually
>>>> the same, isn't it?
>>> This syntax is correct of course, but we have an internal guidelines specifically
>>> saying that terminator line should NOT contain a comma at the end. Justification:
>>>
>>> "Terminator line is established for the data structure arrays which may have unknown,
>>> to the caller, sizes. The purpose of it is to stop iteration over an array and avoid
>>> out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to avoid
>>> potential, but unlike, event of adding the entry after terminator, already at compile time.
>>> This will be achieved by not putting comma at the end of terminator line"
>> This certainly applies to any new code.
>>
>> The existing code, however, is what it is and the question is how much
>> of an improvement the given change makes.
>>
>> So yes, it may not follow the current rules for new code, but then it
>> may not be worth changing to follow these rules anyway.
> This is a bit like housing in a city.
>
> Usually, there are strict requirements that must be followed while
> constructing a new building, but existing buildings are not
> reconstructed to follow them in the majority of cases. It may not
> even be a good idea to do that.
Thanks, great explanation ! I think it's a shared sentiment among maintainers.
I've been watching upstreaming effort of intel new idpf driver, and it got rejected
basically because new drivers are held to a higher standard (they didn't modernize
their code to use new page pool API).
https://lore.kernel.org/netdev/[email protected]/#t