Re: [RFT][PATCH v1 6/6] ACPI: NFIT: core: Convert the driver to a platform one

[Ira Weiny <ira.weiny@intel.com>]

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> While binding drivers directly to struct acpi_device objects allows
> basic functionality to be provided, at least in the majority of cases,
> there are some problems with it, related to general consistency, sysfs
> layout, power management operation ordering, and code cleanliness.
> 
> Overall, it is better to bind drivers to platform devices than to their
> ACPI companions, so convert the ACPI NFIT core driver to a platform one.
> 
> While this is not expected to alter functionality, it changes sysfs
> layout and so it will be visible to user space.

I'm not sure right off why but when I run the libndctl test with this patch I
get the following panic.

[   17.483472] BUG: kernel NULL pointer dereference, address: 0000000000000008
[   17.484116] #PF: supervisor read access in kernel mode
[   17.484593] #PF: error_code(0x0000) - not-present page
[   17.485056] PGD 9def067 P4D 9def067 PUD 9df3067 PMD 0
[   17.485516] Oops: Oops: 0000 [#1] SMP NOPTI
[   17.485886] CPU: 2 UID: 0 PID: 1191 Comm: libndctl Tainted: G           O        6.18.0ira+ #34 PREEMPT(voluntary)
[   17.486804] Tainted: [O]=OOT_MODULE
[   17.487125] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20250812-18.fc42 08/12/2025
[   17.487749] RIP: 0010:acpi_nfit_ctl+0x40b/0xa00 [nfit]
[   17.488222] Code: 48 48 c7 44 24 28 28 f1 8c a1 48 8b 83 c8 01 00 00 44 89 e7 48 89 44 24 50 e8 01 83 fd ff 48 c7 44 24 40 10 58 8c a1 48 89 c3 <49> 8b 47 08 48 c7 44 24 30 30 f1 8c a1 48 89 44 24 18 e9 24 fd
ff
[   17.491668] RSP: 0018:ffffc9000f11ba28 EFLAGS: 00010286
[   17.492422] RAX: ffffffffa18679f0 RBX: ffffffffa18679f0 RCX: ffffc9000f11bb40
[   17.492903] RDX: 000000000000041e RSI: ffffffffa18cf116 RDI: 0000000000000003
[   17.493408] RBP: ffffc9000f11bb40 R08: 0000000000000008 R09: ffffc9000f11bafc
[   17.493888] R10: ffffc9000f11bae0 R11: 0000000000004019 R12: 0000000000000003
[   17.494396] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   17.494878] FS:  00007f432f5fd7c0(0000) GS:ffff8880f9fdd000(0000) knlGS:0000000000000000
[   17.495436] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   17.495826] CR2: 0000000000000008 CR3: 0000000009e0c005 CR4: 0000000000770ef0
[   17.496324] PKRU: 55555554
[   17.496516] Call Trace:
[   17.496691]  <TASK>
[   17.496844]  ? __kmalloc_noprof+0x410/0x650
[   17.497138]  ? setup_result+0x1b/0xa0 [nfit_test]
[   17.497474]  nfit_ctl_test+0x21a/0x780 [nfit_test]
[   17.497803]  ? preempt_count_add+0x51/0xd0
[   17.498086]  ? up_write+0x13/0x60
[   17.498333]  ? up_write+0x35/0x60
[   17.498565]  ? preempt_count_add+0x51/0xd0
[   17.498846]  ? kernfs_next_descendant_post+0x1b/0xe0
[   17.499196]  nfit_test_probe+0x350/0x4d0 [nfit_test]
[   17.499535]  platform_probe+0x38/0x70
[   17.499791]  really_probe+0xde/0x380
[   17.500039]  ? _raw_spin_unlock_irq+0x18/0x40
[   17.500354]  __driver_probe_device+0xc0/0x150
[   17.500656]  driver_probe_device+0x1f/0xa0                                                                                                                                                                       [   17.500939]  ? __pfx___driver_attach+0x10/0x10
[   17.501263]  __driver_attach+0xc7/0x200
[   17.501529]  bus_for_each_dev+0x63/0xa0
[   17.501794]  bus_add_driver+0x114/0x200
[   17.502059]  driver_register+0x71/0xe0
[   17.502480]  nfit_test_init+0x24e/0xff0 [nfit_test]
[   17.502956]  ? __pfx_nfit_test_init+0x10/0x10 [nfit_test]
[   17.503483]  do_one_initcall+0x42/0x210
[   17.503891]  do_init_module+0x62/0x230
[   17.504296]  init_module_from_file+0xb1/0xe0
[   17.504732]  idempotent_init_module+0xed/0x2d0
[   17.505184]  __x64_sys_finit_module+0x6d/0xe0
[   17.505626]  do_syscall_64+0x62/0x390
[   17.506016]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   17.506563] RIP: 0033:0x7f432f8920cd
[   17.506946] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 03 4d 0f 00 f7 d8 64 89 01
48
[   17.508548] RSP: 002b:00007fff0a6ccd98 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   17.509209] RAX: ffffffffffffffda RBX: 000000001f5def50 RCX: 00007f432f8920cd
[   17.509831] RDX: 0000000000000000 RSI: 00007f432f9aa965 RDI: 0000000000000003                                                                                                                                    [   17.510472] RBP: 00007fff0a6cce50 R08: 0000000000000000 R09: 00007fff0a6cce00
[   17.511091] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000020000
[   17.511727] R13: 000000001f5deb60 R14: 00007f432f9aa965 R15: 0000000000000000
[   17.512353]  </TASK>
[   17.512638] Modules linked in: nfit_test(O+) device_dax(O) nd_pmem(O) dax_pmem(O) kmem nd_btt(O) nfit(O) dax_cxl cxl_pci nd_e820(O) cxl_mock_mem(O) cxl_test(O) cxl_mem(O) cxl_pmem(O) cxl_acpi(O) cxl_port(O) cx
l_mock(O) libnvdimm(O) nfit_test_iomap(O) cxl_core(O) fwctl
[   17.514512] CR2: 0000000000000008
[   17.514878] ---[ end trace 0000000000000000 ]---


I'll try and find some time to dig into it but perhaps yall have a quick
idea of what it could be?

Ira

> 
> This change was mostly developed by Michal Wilczynski [1].
> 
> Linu: https://lore.kernel.org/linux-acpi/20231011083334.3987477-6-michal.wilczynski@intel.com/ [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/nfit/core.c |   34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright(c) 2013-2015 Intel Corporation. All rights reserved.
>   */
> +#include <linux/platform_device.h>
>  #include <linux/list_sort.h>
>  #include <linux/libnvdimm.h>
>  #include <linux/module.h>
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(s
>  			|| strcmp(nd_desc->provider_name, "ACPI.NFIT") != 0)
>  		return NULL;
>  
> -	return to_acpi_device(acpi_desc->dev);
> +	return ACPI_COMPANION(acpi_desc->dev);
>  }
>  
>  static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
> @@ -3283,11 +3284,11 @@ static void acpi_nfit_put_table(void *ta
>  
>  static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>  {
> -	struct acpi_device *adev = data;
> +	struct device *dev = data;
>  
> -	device_lock(&adev->dev);
> -	__acpi_nfit_notify(&adev->dev, handle, event);
> -	device_unlock(&adev->dev);
> +	device_lock(dev);
> +	__acpi_nfit_notify(dev, handle, event);
> +	device_unlock(dev);
>  }
>  
>  static void acpi_nfit_remove_notify_handler(void *data)
> @@ -3328,18 +3329,19 @@ void acpi_nfit_shutdown(void *data)
>  }
>  EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>  
> -static int acpi_nfit_add(struct acpi_device *adev)
> +static int acpi_nfit_probe(struct platform_device *pdev)
>  {
>  	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>  	struct acpi_nfit_desc *acpi_desc;
> -	struct device *dev = &adev->dev;
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>  	struct acpi_table_header *tbl;
>  	acpi_status status = AE_OK;
>  	acpi_size sz;
>  	int rc = 0;
>  
>  	rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> -					     acpi_nfit_notify, adev);
> +					     acpi_nfit_notify, dev);
>  	if (rc)
>  		return rc;
>  
> @@ -3369,7 +3371,7 @@ static int acpi_nfit_add(struct acpi_dev
>  	acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
>  	if (!acpi_desc)
>  		return -ENOMEM;
> -	acpi_nfit_desc_init(acpi_desc, &adev->dev);
> +	acpi_nfit_desc_init(acpi_desc, dev);
>  
>  	/* Save the acpi header for exporting the revision via sysfs */
>  	acpi_desc->acpi_header = *tbl;
> @@ -3474,11 +3476,11 @@ static const struct acpi_device_id acpi_
>  };
>  MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>  
> -static struct acpi_driver acpi_nfit_driver = {
> -	.name = KBUILD_MODNAME,
> -	.ids = acpi_nfit_ids,
> -	.ops = {
> -		.add = acpi_nfit_add,
> +static struct platform_driver acpi_nfit_driver = {
> +	.probe = acpi_nfit_probe,
> +	.driver = {
> +		.name = "acpi-nfit",
> +		.acpi_match_table = acpi_nfit_ids,
>  	},
>  };
>  
> @@ -3516,7 +3518,7 @@ static __init int nfit_init(void)
>  		return -ENOMEM;
>  
>  	nfit_mce_register();
> -	ret = acpi_bus_register_driver(&acpi_nfit_driver);
> +	ret = platform_driver_register(&acpi_nfit_driver);
>  	if (ret) {
>  		nfit_mce_unregister();
>  		destroy_workqueue(nfit_wq);
> @@ -3529,7 +3531,7 @@ static __init int nfit_init(void)
>  static __exit void nfit_exit(void)
>  {
>  	nfit_mce_unregister();
> -	acpi_bus_unregister_driver(&acpi_nfit_driver);
> +	platform_driver_unregister(&acpi_nfit_driver);
>  	destroy_workqueue(nfit_wq);
>  	WARN_ON(!list_empty(&acpi_descs));
>  }
> 
> 
>