Re: [PATCH v6 7/9] acpi/nfit: Move handler installing logic to driver

[kernel test robot]

Hi Michal,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on nvdimm/libnvdimm-for-next]
[cannot apply to nvdimm/dax-misc]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Michal-Wilczynski/acpi-bus-Introduce-wrappers-for-ACPICA-event-handler-install-remove/20230701-023629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git 
linux-next
patch link:
https://lore.kernel.org/r/20230630183344.891077-8-michal.wilczynski%40intel.com
patch subject: [PATCH v6 7/9] acpi/nfit: Move handler installing logic to driver
config: x86_64-allyesconfig 
(https://download.01.org/0day-ci/archive/20230701/202307010426.iatkxynx-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: 
(https://download.01.org/0day-ci/archive/20230701/202307010426.iatkxynx-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-kbuild-all/202307010426.iatkxynx-...@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/acpi/nfit/core.c:3294:6: warning: no previous prototype for 
>> 'acpi_nfit_remove_notify_handler' [-Wmissing-prototypes]
3294 | void acpi_nfit_remove_notify_handler(void *data)
 |  ^~~


vim +/acpi_nfit_remove_notify_handler +3294 drivers/acpi/nfit/core.c

  3293  
> 3294  void acpi_nfit_remove_notify_handler(void *data)
  3295  {
  3296  struct acpi_device *adev = data;
  3297  
  3298  acpi_dev_remove_notify_handler(adev,
  3299 ACPI_DEVICE_NOTIFY,
  3300 acpi_nfit_notify);
  3301  }
  3302  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[GIT PULL] NVDIMM and DAX for 6.5

[Verma, Vishal L]

Hi Linus, please pull from

  git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git 
tags/libnvdimm-for-6.5

... to receive  the libnvdimm and DAX update for v6.5.

This is mostly small cleanups and fixes, with the biggest change being the
change to the DAX fault handler allowing it to return VM_FAULT_HWPOISON.

It has appeared in linux-next with no reported issues.

On an operational note, as Dan handed off the branch to me for this cycle, we
missed that the original few commits were inadvertently made on top of a few
CXL commits that went in in the 6.4-rc cycle via the CXL tree.

git-request-pull included these, and hence they appear in the shortlog and
diffstat below, but the actual merge correctly identifies and skips over them.
I kept it as it is to preserve the linux-next soak time, but if I should have
done it differently, please let me know.

---

The following changes since commit f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6:

  Linux 6.4-rc2 (2023-05-14 12:51:40 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git 
tags/libnvdimm-for-6.5

for you to fetch changes up to 1ea7ca1b090145519aad998679222f0a14ab8fce:

  dax: enable dax fault handler to report VM_FAULT_HWPOISON (2023-06-26 
07:54:23 -0600)


libnvdimm for 6.5

- DAX fixes and cleanups including a use after free, extra references,
  and device unregistration, and a redundant variable.

- Allow the DAX fault handler to return VM_FAULT_HWPOISON

- A few libnvdimm cleanups such as making some functions and variables
static where sufficient.

- Add a few missing prototypes for wrapped functions in
tools/testing/nvdimm


Arnd Bergmann (3):
  dax: fix missing-prototype warnings
  testing: nvdimm: add missing prototypes for wrapped functions
  libnvdimm: mark 'security_show' static again

Ben Dooks (2):
  nvdimm: make nd_class variable static
  nvdimm: make security_show static

Colin Ian King (1):
  fsdax: remove redundant variable 'error'

Dan Williams (5):
  cxl/port: Enable the HDM decoder capability for switch ports
  dax: Fix dax_mapping_release() use after free
  dax: Use device_unregister() in unregister_dax_mapping()
  dax: Introduce alloc_dev_dax_id()
  dax: Cleanup extra dax_region references

Dave Jiang (2):
  cxl: Wait Memory_Info_Valid before access memory related info
  cxl: Move cxl_await_media_ready() to before capacity info retrieval

Jane Chu (1):
  dax: enable dax fault handler to report VM_FAULT_HWPOISON

Tarun Sahu (1):
  dax/kmem: Pass valid argument to memory_group_register_static

Uwe Kleine-König (1):
  tools/testing/nvdimm: Drop empty platform remove function

Vishal Verma (1):
  Merge branch 'for-6.5/dax-cleanups' into nvdimm-for-next

 include/linux/dax.h   |  13 
 include/linux/mm.h|   2 +
 drivers/cxl/cxl.h |   1 +
 drivers/cxl/cxlmem.h  |   2 +
 drivers/cxl/cxlpci.h  |   2 +
 drivers/dax/bus.h |   8 ---
 drivers/dax/dax-private.h |  11 +++-
 tools/testing/nvdimm/test/nfit_test.h |  29 +
 drivers/cxl/core/mbox.c   |  15 +++--
 drivers/cxl/core/pci.c| 112 ++
 drivers/cxl/mem.c |   3 +
 drivers/cxl/pci.c |   6 ++
 drivers/cxl/port.c|  20 +++---
 drivers/dax/bus.c |  64 +++
 drivers/dax/cxl.c |   8 +--
 drivers/dax/device.c  |   3 +-
 drivers/dax/hmem/hmem.c   |   8 +--
 drivers/dax/kmem.c|   2 +-
 drivers/dax/pmem.c|   7 +--
 drivers/dax/super.c   |   5 +-
 drivers/nvdimm/bus.c  |   2 +-
 drivers/nvdimm/dimm_devs.c|   4 +-
 drivers/nvdimm/pmem.c |   2 +-
 drivers/s390/block/dcssblk.c  |   3 +-
 fs/dax.c  |  14 ++---
 fs/fuse/virtio_fs.c   |   3 +-
 tools/testing/cxl/test/mem.c  |   1 +
 tools/testing/cxl/test/mock.c |  15 +
 tools/testing/nvdimm/test/nfit.c  |   6 --
 tools/testing/cxl/Kbuild  |   1 +
 30 files changed, 265 insertions(+), 107 deletions(-)

[PATCH v6 9/9] acpi/thermal: Move handler installing logic to driver

[Michal Wilczynski]

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_dev_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.

While at it, fix whitespaces in .remove() callback.

Suggested-by: Rafael J. Wysocki 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/thermal.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index f9f6ebb08fdb..97858ad59d68 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -825,8 +825,9 @@ static void acpi_queue_thermal_check(struct acpi_thermal 
*tz)
queue_work(acpi_thermal_pm_queue, >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_device *device = data;
struct acpi_thermal *tz = acpi_driver_data(device);
 
if (!tz)
@@ -997,11 +998,21 @@ 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;
+
+   return 0;
+
+flush_wq:
+   flush_workqueue(acpi_thermal_pm_queue);
+   acpi_thermal_unregister_thermal_zone(tz);
 free_memory:
kfree(tz);
-end:
+
return result;
 }
 
@@ -1012,10 +1023,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 +1094,6 @@ static struct acpi_driver acpi_thermal_driver = {
.ops = {
.add = acpi_thermal_add,
.remove = acpi_thermal_remove,
-   .notify = acpi_thermal_notify,
},
.drv.pm = _thermal_pm,
 };
-- 
2.41.0

[PATCH v6 8/9] acpi/nfit: Remove unnecessary .remove callback

[Michal Wilczynski]

Nfit driver doesn't use .remove() callback and provide an empty function
as it's .remove() callback. Remove empty acpi_nfit_remove() and
initialization of callback.

Suggested-by: Dan Williams 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/nfit/core.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index ee2365a80aa1..daaea23cacfd 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3402,11 +3402,6 @@ static int acpi_nfit_add(struct acpi_device *adev)
adev);
 }
 
-static void acpi_nfit_remove(struct acpi_device *adev)
-{
-   /* see acpi_nfit_unregister */
-}
-
 static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
 {
struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
@@ -3488,7 +3483,6 @@ static struct acpi_driver acpi_nfit_driver = {
.ids = acpi_nfit_ids,
.ops = {
.add = acpi_nfit_add,
-   .remove = acpi_nfit_remove,
},
 };
 
-- 
2.41.0

[PATCH v6 7/9] acpi/nfit: Move handler installing logic to driver

[Michal Wilczynski]

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
acpi_nfit_shutdown(). Change arguments passed to the notify function to
match with what's required by acpi_dev_install_notify_handler(). Remove
.notify callback initialization in acpi_driver.

Introduce a new devm action acpi_nfit_remove_notify_handler.

Move acpi_nfit_notify() upwards in the file, so it can be used inside
acpi_nfit_add() and acpi_nfit_remove_notify_handler().

Suggested-by: Rafael J. Wysocki 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/nfit/core.c | 41 +++-
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 07204d482968..ee2365a80aa1 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3282,6 +3282,24 @@ static void acpi_nfit_put_table(void *table)
acpi_put_table(table);
 }
 
+static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
+{
+   struct acpi_device *adev = data;
+
+   device_lock(>dev);
+   __acpi_nfit_notify(>dev, handle, event);
+   device_unlock(>dev);
+}
+
+void acpi_nfit_remove_notify_handler(void *data)
+{
+   struct acpi_device *adev = data;
+
+   acpi_dev_remove_notify_handler(adev,
+  ACPI_DEVICE_NOTIFY,
+  acpi_nfit_notify);
+}
+
 void acpi_nfit_shutdown(void *data)
 {
struct acpi_nfit_desc *acpi_desc = data;
@@ -3368,7 +3386,20 @@ static int acpi_nfit_add(struct acpi_device *adev)
 
if (rc)
return rc;
-   return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
+
+   rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
+   if (rc)
+   return rc;
+
+   rc = acpi_dev_install_notify_handler(adev,
+ACPI_DEVICE_NOTIFY,
+acpi_nfit_notify);
+   if (rc)
+   return rc;
+
+   return devm_add_action_or_reset(dev,
+   acpi_nfit_remove_notify_handler,
+   adev);
 }
 
 static void acpi_nfit_remove(struct acpi_device *adev)
@@ -3446,13 +3477,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(>dev);
-   __acpi_nfit_notify(>dev, adev->handle, event);
-   device_unlock(>dev);
-}
-
 static const struct acpi_device_id acpi_nfit_ids[] = {
{ "ACPI0012", 0 },
{ "", 0 },
@@ -3465,7 +3489,6 @@ static struct acpi_driver acpi_nfit_driver = {
.ops = {
.add = acpi_nfit_add,
.remove = acpi_nfit_remove,
-   .notify = acpi_nfit_notify,
},
 };
 
-- 
2.41.0

[PATCH v6 6/9] acpi/hed: Move handler installing logic to driver

[Michal Wilczynski]

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_dev_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.

Suggested-by: Rafael J. Wysocki 
Signed-off-by: Michal Wilczynski 
---
 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(_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

[PATCH v6 5/9] acpi/battery: Move handler installing logic to driver

[Michal Wilczynski]

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_dev_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 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/battery.c | 26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9c67ed02d797..4c634a4c32dd 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1034,8 +1034,9 @@ 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_device *device = data;
struct acpi_battery *battery = acpi_driver_data(device);
struct power_supply *old;
 
@@ -1212,13 +1213,23 @@ static int acpi_battery_add(struct acpi_device *device)
 
device_init_wakeup(>dev, 1);
 
-   return result;
+   result = acpi_dev_install_notify_handler(device,
+ACPI_ALL_NOTIFY,
+acpi_battery_notify);
+   if (result)
+   goto fail_pm;
+
+   return 0;
 
+fail_pm:
+   device_init_wakeup(>dev, 0);
+   unregister_pm_notifier(>pm_nb);
 fail:
sysfs_remove_battery(battery);
mutex_destroy(>lock);
mutex_destroy(>sysfs_lock);
kfree(battery);
+
return result;
 }
 
@@ -1228,10 +1239,17 @@ static void acpi_battery_remove(struct acpi_device 
*device)
 
if (!device || !acpi_driver_data(device))
return;
-   device_init_wakeup(>dev, 0);
+
battery = acpi_driver_data(device);
+
+   acpi_dev_remove_notify_handler(device,
+  ACPI_ALL_NOTIFY,
+  acpi_battery_notify);
+
+   device_init_wakeup(>dev, 0);
unregister_pm_notifier(>pm_nb);
sysfs_remove_battery(battery);
+
mutex_destroy(>lock);
mutex_destroy(>sysfs_lock);
kfree(battery);
@@ -1264,11 +1282,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 = _battery_pm,
 };
-- 
2.41.0

[PATCH v6 4/9] acpi/video: Move handler installing logic to driver

[Michal Wilczynski]

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_dev_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.

Suggested-by: Rafael J. Wysocki 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/acpi_video.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 62f4364e4460..168bb43e0c65 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,8 +1526,9 @@ 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_device *device = data;
struct acpi_video_bus *video = acpi_driver_data(device);
struct input_dev *input;
int keycode = 0;
@@ -2053,8 +2053,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;
+
return 0;
 
+err_remove:
+   mutex_lock(_list_lock);
+   list_del(>entry);
+   mutex_unlock(_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 +2087,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(_list_lock);
list_del(>entry);
mutex_unlock(_list_lock);
-- 
2.41.0

[PATCH v6 3/9] acpi/ac: Move handler installing logic to driver

[Michal Wilczynski]

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_dev_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.

Suggested-by: Rafael J. Wysocki 
Signed-off-by: Michal Wilczynski 
---
 drivers/acpi/ac.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 1ace70b831cd..f6feff1f3118 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
 
 static int acpi_ac_add(struct acpi_device *device);
 static void acpi_ac_remove(struct acpi_device *device);
-static void acpi_ac_notify(struct acpi_device *device, u32 event);
+static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
 
 static const struct acpi_device_id ac_device_ids[] = {
{"ACPI0003", 0},
@@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
.name = "ac",
.class = ACPI_AC_CLASS,
.ids = ac_device_ids,
-   .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
.ops = {
.add = acpi_ac_add,
.remove = acpi_ac_remove,
-   .notify = acpi_ac_notify,
},
.drv.pm = _ac_pm,
 };
@@ -128,8 +126,9 @@ static enum power_supply_property ac_props[] = {
 };
 
 /* Driver Model */
-static void acpi_ac_notify(struct acpi_device *device, u32 event)
+static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
 {
+   struct acpi_device *device = data;
struct acpi_ac *ac = acpi_driver_data(device);
 
if (!ac)
@@ -235,7 +234,7 @@ static int acpi_ac_add(struct acpi_device *device)
 
result = acpi_ac_get_state(ac);
if (result)
-   goto end;
+   goto err_release_ac;
 
psy_cfg.drv_data = ac;
 
@@ -248,7 +247,7 @@ static int acpi_ac_add(struct acpi_device *device)
>charger_desc, _cfg);
if (IS_ERR(ac->charger)) {
result = PTR_ERR(ac->charger);
-   goto end;
+   goto err_release_ac;
}
 
pr_info("%s [%s] (%s)\n", acpi_device_name(device),
@@ -256,9 +255,20 @@ static int acpi_ac_add(struct acpi_device *device)
 
ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(>battery_nb);
-end:
+
+   result = acpi_dev_install_notify_handler(device,
+ACPI_ALL_NOTIFY,
+acpi_ac_notify);
if (result)
-   kfree(ac);
+   goto err_unregister;
+
+   return 0;
+
+err_unregister:
+   power_supply_unregister(ac->charger);
+   unregister_acpi_notifier(>battery_nb);
+err_release_ac:
+   kfree(ac);
 
return result;
 }
@@ -297,6 +307,9 @@ static void acpi_ac_remove(struct acpi_device *device)
 
ac = acpi_driver_data(device);
 
+   acpi_dev_remove_notify_handler(device,
+  ACPI_ALL_NOTIFY,
+  acpi_ac_notify);
power_supply_unregister(ac->charger);
unregister_acpi_notifier(>battery_nb);
 
-- 
2.41.0

[PATCH v6 2/9] acpi/bus: Set driver_data to NULL every time .add() fails

[Michal Wilczynski]

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 
---
 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 2d6f1f45d44e..c087fd6e8398 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1014,8 +1014,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

[PATCH v6 1/9] acpi/bus: Introduce wrappers for ACPICA event handler install/remove

[Michal Wilczynski]

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 
Signed-off-by: Michal Wilczynski 
---
 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 20cdfb37da79..2d6f1f45d44e 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -554,6 +554,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

[PATCH v6 0/9] Remove .notify callback in acpi_device_ops

[Michal Wilczynski]

*** 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.

This is part 1 - only drivers in acpi directory to ease up review
process.

v6:
 - fixed unnecessary RCT in all drivers, as it's not a purpose of
   this patch series
 - changed error label names to simplify them
 - dropped commit that remove a comma
 - squashed commit moving code for nfit
 - improved nfit driver to use devm instead of .remove()
 - re-based as Rafael changes [1] are merged already

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 (9):
  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 handler installing logic to driver
  acpi/nfit: Remove unnecessary .remove callback
  acpi/thermal: Move handler installing logic to driver

 drivers/acpi/ac.c | 29 ++---
 drivers/acpi/acpi_video.c | 22 ---
 drivers/acpi/battery.c| 26 +-
 drivers/acpi/bus.c| 30 +-
 drivers/acpi/hed.c| 17 ---
 drivers/acpi/nfit/core.c  | 45 +++
 drivers/acpi/thermal.c| 25 +-
 include/acpi/acpi_bus.h   |  6 ++
 8 files changed, 161 insertions(+), 39 deletions(-)

-- 
2.41.0

Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/30/2023 7:19 PM, Dan Williams wrote:
> Wilczynski, Michal wrote:
>>
>> On 6/29/2023 10:54 PM, Dan Williams wrote:
>>> 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_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 
 Signed-off-by: Michal Wilczynski 
 ---
  drivers/acpi/nfit/core.c | 24 ++--
  1 file changed, 18 insertions(+), 6 deletions(-)

 diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
 index 95930e9d776c..a281bdfee8a0 100644
 --- a/drivers/acpi/nfit/core.c
 +++ b/drivers/acpi/nfit/core.c
 @@ -3312,11 +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)
 +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
  {
 -  device_lock(>dev);
 -  __acpi_nfit_notify(>dev, adev->handle, event);
 -  device_unlock(>dev);
 +  struct acpi_device *device = data;
 +
 +  device_lock(>dev);
 +  __acpi_nfit_notify(>dev, handle, event);
 +  device_unlock(>dev);
  }
  
  static int acpi_nfit_add(struct acpi_device *adev)
 @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
  
if (rc)
return rc;
 -  return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
 +
 +  rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
 +  if (rc)
 +  return rc;
 +
 +  return acpi_dev_install_notify_handler(adev,
 + ACPI_DEVICE_NOTIFY,
 + acpi_nfit_notify);
  }
  
  static void acpi_nfit_remove(struct acpi_device *adev)
  {
/* see acpi_nfit_unregister */
 +
 +  acpi_dev_remove_notify_handler(adev,
 + ACPI_DEVICE_NOTIFY,
 + acpi_nfit_notify);
>>> Please use devm to trigger this release rather than making
>>> acpi_nfit_remove() contain any logic.
>> I think adding separate devm action to remove event handler is not
>> necessary. I'll put the removal in the beggining of acpi_nfit_shutdown() if 
>> you
>> don't object.
> How do you plan to handle an acpi_dev_install_notify_handler() failure?
> acpi_nfit_shutdown() will need to have extra logic to know that it can
> skip acpi_dev_remove_notify_handler() in some cases and not other..
> Maybe it is ok to remove a handler that was never installed, but I would
> rather not go look that up. A devm callback for
> acpi_dev_remove_notify_handler() avoids that.

Sure, I looked at the code and it seems to me that trying to remove a callback 
that doesn't
exist shouldn't cause any problems. But maybe it's not very elegant and we 
shouldn't rely
on that behavior.

Will add separate devm action for that then.

Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Dan Williams]

Wilczynski, Michal wrote:
> 
> 
> On 6/29/2023 10:54 PM, Dan Williams wrote:
> > 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_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 
> >> Signed-off-by: Michal Wilczynski 
> >> ---
> >>  drivers/acpi/nfit/core.c | 24 ++--
> >>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 95930e9d776c..a281bdfee8a0 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -3312,11 +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)
> >> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> >>  {
> >> -  device_lock(>dev);
> >> -  __acpi_nfit_notify(>dev, adev->handle, event);
> >> -  device_unlock(>dev);
> >> +  struct acpi_device *device = data;
> >> +
> >> +  device_lock(>dev);
> >> +  __acpi_nfit_notify(>dev, handle, event);
> >> +  device_unlock(>dev);
> >>  }
> >>  
> >>  static int acpi_nfit_add(struct acpi_device *adev)
> >> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
> >>  
> >>if (rc)
> >>return rc;
> >> -  return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> >> +
> >> +  rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> >> +  if (rc)
> >> +  return rc;
> >> +
> >> +  return acpi_dev_install_notify_handler(adev,
> >> + ACPI_DEVICE_NOTIFY,
> >> + acpi_nfit_notify);
> >>  }
> >>  
> >>  static void acpi_nfit_remove(struct acpi_device *adev)
> >>  {
> >>/* see acpi_nfit_unregister */
> >> +
> >> +  acpi_dev_remove_notify_handler(adev,
> >> + ACPI_DEVICE_NOTIFY,
> >> + acpi_nfit_notify);
> > Please use devm to trigger this release rather than making
> > acpi_nfit_remove() contain any logic.
> 
> I think adding separate devm action to remove event handler is not
> necessary. I'll put the removal in the beggining of acpi_nfit_shutdown() if 
> you
> don't object.

How do you plan to handle an acpi_dev_install_notify_handler() failure?
acpi_nfit_shutdown() will need to have extra logic to know that it can
skip acpi_dev_remove_notify_handler() in some cases and not other..
Maybe it is ok to remove a handler that was never installed, but I would
rather not go look that up. A devm callback for
acpi_dev_remove_notify_handler() avoids that.

Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 10:54 PM, Dan Williams wrote:
> 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_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 
>> Signed-off-by: Michal Wilczynski 
>> ---
>>  drivers/acpi/nfit/core.c | 24 ++--
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 95930e9d776c..a281bdfee8a0 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,11 +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)
>> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>>  {
>> -device_lock(>dev);
>> -__acpi_nfit_notify(>dev, adev->handle, event);
>> -device_unlock(>dev);
>> +struct acpi_device *device = data;
>> +
>> +device_lock(>dev);
>> +__acpi_nfit_notify(>dev, handle, event);
>> +device_unlock(>dev);
>>  }
>>  
>>  static int acpi_nfit_add(struct acpi_device *adev)
>> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>>  
>>  if (rc)
>>  return rc;
>> -return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +
>> +rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +if (rc)
>> +return rc;
>> +
>> +return acpi_dev_install_notify_handler(adev,
>> +   ACPI_DEVICE_NOTIFY,
>> +   acpi_nfit_notify);
>>  }
>>  
>>  static void acpi_nfit_remove(struct acpi_device *adev)
>>  {
>>  /* see acpi_nfit_unregister */
>> +
>> +acpi_dev_remove_notify_handler(adev,
>> +   ACPI_DEVICE_NOTIFY,
>> +   acpi_nfit_notify);
> Please use devm to trigger this release rather than making
> acpi_nfit_remove() contain any logic.

I think adding separate devm action to remove event handler is not
necessary. I'll put the removal in the beggining of acpi_nfit_shutdown() if you
don't object.

>
> An additional cleanup opportunity with the ->add() path fully devm
> instrumented would be to just delete acpi_nfit_remove() since it is
> optional and serves no purpose.

Will do,

Thanks !

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

On 6/30/2023 1:13 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki  wrote:
>> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
>>  wrote:
>>>
>>>
>>> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
 On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
  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/20230621122106.56cb5...@kernel.org/#t

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki  wrote:
>
> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
>  wrote:
> >
> >
> >
> > On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > >  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.

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
 wrote:
>
>
>
> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> >  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.

Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:55 AM Wilczynski, Michal
 wrote:
>
>
>
> On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM 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_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 
> >> Signed-off-by: Michal Wilczynski 
> >> ---
> >>  drivers/acpi/nfit/core.c | 24 ++--
> >>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 95930e9d776c..a281bdfee8a0 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -3312,11 +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)
> >> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> >>  {
> >> -   device_lock(>dev);
> >> -   __acpi_nfit_notify(>dev, adev->handle, event);
> >> -   device_unlock(>dev);
> > It's totally not necessary to rename the ACPI device variable here.
> >
> > Just add
> >
> > struct acpi_device *adev = data;
> >
> > to this function.
>
> Sure, is adev a preferred name for acpi_device ?

In new code, it is.

In the existing code, it depends.  If you do a one-line change, it is
better to retain the original naming (for the sake of clarity of the
change itself).  If you rearrange it completely, you may as well
change the names while at it.  And there is a spectrum in between.

>  I've seen a mix of different naming
> in drivers, some use device, adev, acpi_dev and so on. I suppose it's not a 
> big deal, but
> it would be good to know.

Personally, I prefer adev, but this isn't a very strong preference.

Using "device" as a name of a struct acpi_device object (or a pointer
to one of these for that matter) is slightly misleading IMV, because
those things represent AML entities rather than actual hardware.

Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:48 AM Wilczynski, Michal
 wrote:
>
>
>
> On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> >  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 
> >> ---
> >>  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(>dev);
> >> +   __acpi_nfit_notify(>dev, adev->handle, event);
> >> +   device_unlock(>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(>dev);
> >> -   __acpi_nfit_notify(>dev, adev->handle, event);
> >> -   device_unlock(>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/e0f67199-9feb-432c-f0cb-7bdbdaf9f...@linux.intel.com/

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.

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

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 
>> ---
>>  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

Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM 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_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 
>> Signed-off-by: Michal Wilczynski 
>> ---
>>  drivers/acpi/nfit/core.c | 24 ++--
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 95930e9d776c..a281bdfee8a0 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,11 +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)
>> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>>  {
>> -   device_lock(>dev);
>> -   __acpi_nfit_notify(>dev, adev->handle, event);
>> -   device_unlock(>dev);
> It's totally not necessary to rename the ACPI device variable here.
>
> Just add
>
> struct acpi_device *adev = data;
>
> to this function.

Sure, is adev a preferred name for acpi_device ? I've seen a mix of different 
naming
in drivers, some use device, adev, acpi_dev and so on. I suppose it's not a big 
deal, but
it would be good to know.

>
>> +   struct acpi_device *device = data;
>> +
>> +   device_lock(>dev);
>> +   __acpi_nfit_notify(>dev, handle, event);
>> +   device_unlock(>dev);
>>  }
>>
>>  static int acpi_nfit_add(struct acpi_device *adev)
>> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>>
>> if (rc)
>> return rc;
>> -   return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +
>> +   rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +   if (rc)
>> +   return rc;
>> +
>> +   return acpi_dev_install_notify_handler(adev,
>> +  ACPI_DEVICE_NOTIFY,
>> +  acpi_nfit_notify);
>>  }
>>
>>  static void acpi_nfit_remove(struct acpi_device *adev)
>>  {
>> /* see acpi_nfit_unregister */
>> +
>> +   acpi_dev_remove_notify_handler(adev,
>> +  ACPI_DEVICE_NOTIFY,
>> +  acpi_nfit_notify);
>>  }
>>
>>  static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
>> @@ -3465,7 +3478,6 @@ static struct acpi_driver acpi_nfit_driver = {
>> .ops = {
>> .add = acpi_nfit_add,
>> .remove = acpi_nfit_remove,
>> -   .notify = acpi_nfit_notify,
>> },
>>  };
>>
>> --

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
>  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 
>> ---
>>  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
>>

Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 6:05 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM 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_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 
>> Signed-off-by: Michal Wilczynski 
>> ---
>>  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(>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(>dev, 0);
>> +   unregister_pm_notifier(>pm_nb);
>>  fail:
>> sysfs_remove_battery(battery);
>> mutex_destroy(>lock);
>> mutex_destroy(>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(>dev, 0);
>> +
>> battery = acpi_driver_data(device);
>> +
>> +   acpi_dev_remove_notify_handler(device,
>> +  ACPI_ALL_NOTIFY,
>> +  acpi_battery_notify);
>> +
>> +   device_init_wakeup(>dev, 0);
>> unregister_pm_notifier(>pm_nb);
>> sysfs_remove_battery(battery);
>> +
>> mutex_destroy(>lock);
>> mutex_destroy(>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 = _battery_pm,
>>  };
>> --
>> 2.41.0
>>

Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Wilczynski, Michal]

On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
>  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 
>> ---
>>  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(>dev);
>> +   __acpi_nfit_notify(>dev, adev->handle, event);
>> +   device_unlock(>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(>dev);
>> -   __acpi_nfit_notify(>dev, adev->handle, event);
>> -   device_unlock(>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/e0f67199-9feb-432c-f0cb-7bdbdaf9f...@linux.intel.com/

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:41 AM Rafael J. Wysocki  wrote:
>
> On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
>  wrote:
> >
> >
> >
> > On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 16, 2023 at 6:51 PM 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_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 
> > >> Signed-off-by: Michal Wilczynski 
> > >> ---
> > >>  drivers/acpi/ac.c | 33 -
> > >>  1 file changed, 24 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> > >> index 1ace70b831cd..207ee3c85bad 100644
> > >> --- a/drivers/acpi/ac.c
> > >> +++ b/drivers/acpi/ac.c
> > >> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
> > >>
> > >>  static int acpi_ac_add(struct acpi_device *device);
> > >>  static void acpi_ac_remove(struct acpi_device *device);
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
> > >>
> > >>  static const struct acpi_device_id ac_device_ids[] = {
> > >> {"ACPI0003", 0},
> > >> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> > >> .name = "ac",
> > >> .class = ACPI_AC_CLASS,
> > >> .ids = ac_device_ids,
> > >> -   .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> > >> .ops = {
> > >> .add = acpi_ac_add,
> > >> .remove = acpi_ac_remove,
> > >> -   .notify = acpi_ac_notify,
> > >> },
> > >> .drv.pm = _ac_pm,
> > >>  };
> > >> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> > >>  };
> > >>
> > >>  /* Driver Model */
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> > >>  {
> > >> -   struct acpi_ac *ac = acpi_driver_data(device);
> > > This line doesn't need to be changed.  Just add the device variable
> > > definition above it.
> > >
> > > And the same pattern is present in the other patches in the series.
> >
> > I like the Reverse Christmas Tree, but sure will change that
>
> I do too, but in the cases when it costs 3 extra lines of code I'd
> rather have something simpler.

Besides, moving code around is not strictly related to the functional
changes made by the patch and it kind of hides those changes.  It is
better to move code around in a separate patch if you really want to
do that.

Re: [PATCH v5 04/10] acpi/video: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 5:58 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM 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_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 
>> Signed-off-by: Michal Wilczynski 
>> ---
>>  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(_list_lock);
>> +   list_del(>entry);
>> +   mutex_unlock(_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(_list_lock);
>> list_del(>entry);
>> mutex_unlock(_list_lock);
>> --
>> 2.41.0
>>

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
 wrote:
>
>
>
> On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM 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_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 
> >> Signed-off-by: Michal Wilczynski 
> >> ---
> >>  drivers/acpi/ac.c | 33 -
> >>  1 file changed, 24 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> >> index 1ace70b831cd..207ee3c85bad 100644
> >> --- a/drivers/acpi/ac.c
> >> +++ b/drivers/acpi/ac.c
> >> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
> >>
> >>  static int acpi_ac_add(struct acpi_device *device);
> >>  static void acpi_ac_remove(struct acpi_device *device);
> >> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
> >>
> >>  static const struct acpi_device_id ac_device_ids[] = {
> >> {"ACPI0003", 0},
> >> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> >> .name = "ac",
> >> .class = ACPI_AC_CLASS,
> >> .ids = ac_device_ids,
> >> -   .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> >> .ops = {
> >> .add = acpi_ac_add,
> >> .remove = acpi_ac_remove,
> >> -   .notify = acpi_ac_notify,
> >> },
> >> .drv.pm = _ac_pm,
> >>  };
> >> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> >>  };
> >>
> >>  /* Driver Model */
> >> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> >>  {
> >> -   struct acpi_ac *ac = acpi_driver_data(device);
> > This line doesn't need to be changed.  Just add the device variable
> > definition above it.
> >
> > And the same pattern is present in the other patches in the series.
>
> I like the Reverse Christmas Tree, but sure will change that

I do too, but in the cases when it costs 3 extra lines of code I'd
rather have something simpler.

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM 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_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 
>> Signed-off-by: Michal Wilczynski 
>> ---
>>  drivers/acpi/ac.c | 33 -
>>  1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 1ace70b831cd..207ee3c85bad 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
>>
>>  static int acpi_ac_add(struct acpi_device *device);
>>  static void acpi_ac_remove(struct acpi_device *device);
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>>
>>  static const struct acpi_device_id ac_device_ids[] = {
>> {"ACPI0003", 0},
>> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
>> .name = "ac",
>> .class = ACPI_AC_CLASS,
>> .ids = ac_device_ids,
>> -   .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>> .ops = {
>> .add = acpi_ac_add,
>> .remove = acpi_ac_remove,
>> -   .notify = acpi_ac_notify,
>> },
>> .drv.pm = _ac_pm,
>>  };
>> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
>>  };
>>
>>  /* Driver Model */
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
>>  {
>> -   struct acpi_ac *ac = acpi_driver_data(device);
> This line doesn't need to be changed.  Just add the device variable
> definition above it.
>
> And the same pattern is present in the other patches in the series.

I like the Reverse Christmas Tree, but sure will change that

>
>> +   struct acpi_device *device = data;
>> +   struct acpi_ac *ac;
>> +
>> +   ac = acpi_driver_data(device);
>>
>> if (!ac)
>> return;