[v2,5/6] ACPI: NFIT: Replace acpi_driver with platform_driver

  NFIT driver uses struct acpi_driver incorrectly to register itself.
This is wrong as the instances of the ACPI devices are not meant
to be literal devices, they're supposed to describe ACPI entry of a
particular device.

Use platform_driver instead of acpi_driver. In relevant places call
platform devices instances pdev to make a distinction with ACPI
devices instances.

NFIT driver uses devm_*() family of functions extensively. This change
has no impact on correct functioning of the whole devm_*() family of
functions, since the lifecycle of the device stays the same. It is still
being created during the enumeration, and destroyed on platform device
removal.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)
  

On 10/6/2023 7:30 PM, Michal Wilczynski wrote:
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>

Hi Dan,
Rafael already reviewed this patch series and merged patches
that he likes. We're waiting on your input regarding two NFIT
commits in this series.

Thanks a lot !
Michał

> ---
>  drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 942b84d94078..fb0bc16fa186 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -15,6 +15,7 @@
>  #include <linux/sort.h>
>  #include <linux/io.h>
>  #include <linux/nd.h>
> +#include <linux/platform_device.h>
>  #include <asm/cacheflush.h>
>  #include <acpi/nfit.h>
>  #include "intel.h"
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
>  			|| 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)
> @@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
>  
>  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)
> @@ -3329,11 +3330,12 @@ 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;
> @@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
>  	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;
> @@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
>  		return rc;
>  
>  	rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> -					     acpi_nfit_notify, adev);
> +					     acpi_nfit_notify, dev);
>  	if (rc)
>  		return rc;
>  
> @@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
>  };
>  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 = KBUILD_MODNAME,
> +		.acpi_match_table = acpi_nfit_ids,
>  	},
>  };
>  
> @@ -3517,7 +3519,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);
> @@ -3530,7 +3532,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));
>  }
  

On Tue, Oct 17, 2023 at 12:45 PM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
>
> On 10/6/2023 7:30 PM, Michal Wilczynski wrote:
> > NFIT driver uses struct acpi_driver incorrectly to register itself.
> > This is wrong as the instances of the ACPI devices are not meant
> > to be literal devices, they're supposed to describe ACPI entry of a
> > particular device.
> >
> > Use platform_driver instead of acpi_driver. In relevant places call
> > platform devices instances pdev to make a distinction with ACPI
> > devices instances.
> >
> > NFIT driver uses devm_*() family of functions extensively. This change
> > has no impact on correct functioning of the whole devm_*() family of
> > functions, since the lifecycle of the device stays the same. It is still
> > being created during the enumeration, and destroyed on platform device
> > removal.
> >
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>
> Hi Dan,
> Rafael already reviewed this patch series and merged patches
> that he likes. We're waiting on your input regarding two NFIT
> commits in this series.

I actually haven't looked at the NFIT patches in this series myself
and this is not urgent at all IMV.

Thanks!
  

On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 942b84d94078..fb0bc16fa186 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -15,6 +15,7 @@
>  #include <linux/sort.h>
>  #include <linux/io.h>
>  #include <linux/nd.h>
> +#include <linux/platform_device.h>
>  #include <asm/cacheflush.h>
>  #include <acpi/nfit.h>
>  #include "intel.h"
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
>                         || 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)
> @@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
>
>  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);

Careful here.

The ACPI device locking is changed to platform device locking without
a word of explanation in the changelog.

Do you actually know what the role of the locking around
__acpi_nfit_notify() is and whether or not it can be replaced with
platform device locking safely?

>  }
>
>  static void acpi_nfit_remove_notify_handler(void *data)
> @@ -3329,11 +3330,12 @@ 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;
> @@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
>         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);

You seem to think that replacing adev->dev with pdev->dev everywhere
in this driver will work,

Have you verified that in any way?  If so, then how?

>
>         /* Save the acpi header for exporting the revision via sysfs */
>         acpi_desc->acpi_header = *tbl;
> @@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
>                 return rc;
>
>         rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> -                                            acpi_nfit_notify, adev);
> +                                            acpi_nfit_notify, dev);
>         if (rc)
>                 return rc;
>
> @@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
>  };
>  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 = KBUILD_MODNAME,
> +               .acpi_match_table = acpi_nfit_ids,
>         },
>  };
>
> @@ -3517,7 +3519,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);
> @@ -3530,7 +3532,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));
>  }
> --
  

Michal Wilczynski wrote:
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
> 
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
> 
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.

I notice this verbiage has the same fundamental misunderstanding of devm
allocation lifetime as the acpi_nfit_init_interleave_set() discussion.
The devm allocation lifetime typically starts in driver->probe() and
ends either with driver->probe() failure, or the driver->remove() call.
Note that the driver->remove() call is invoked not only for
platform-device removal, but also driver "unbind" events. So the
"destroyed on platform device removal" is the least likely way that
these allocations are torn down given ACPI0012 devices are never
removed.

Outside of that, my main concern about this patch is that I expect it
breaks unit tests. The infrastructure in
tools/testing/nvdimm/test/nfit.c emulates an ACPI0012 device that allows
for deeper regression testing given hardware is difficult to come by,
and because QEMU does not implement some of the tricky corner cases that
the unit tests cover.

This needs to pass tests, but fair warning, 
tools/testing/nvdimm/test/nfit.c does some non-idiomatic + "strange"
things to achieve deeper test coverage. So I expect that if this breaks
tests as I expect the effort needed to fix the emulation could be
significant.

If you want to give running the tests a try the easiest would be to use
"run_qemu.sh" with --nfit-test option [1], or you can try to setup an
environment manually using the ndctl instructions [2].

[1]: https://github.com/pmem/run_qemu
[2]: https://github.com/pmem/ndctl#readme
  

On 10/17/2023 8:24 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> NFIT driver uses struct acpi_driver incorrectly to register itself.
>> This is wrong as the instances of the ACPI devices are not meant
>> to be literal devices, they're supposed to describe ACPI entry of a
>> particular device.
>>
>> Use platform_driver instead of acpi_driver. In relevant places call
>> platform devices instances pdev to make a distinction with ACPI
>> devices instances.
>>
>> NFIT driver uses devm_*() family of functions extensively. This change
>> has no impact on correct functioning of the whole devm_*() family of
>> functions, since the lifecycle of the device stays the same. It is still
>> being created during the enumeration, and destroyed on platform device
>> removal.
> I notice this verbiage has the same fundamental misunderstanding of devm
> allocation lifetime as the acpi_nfit_init_interleave_set() discussion.
> The devm allocation lifetime typically starts in driver->probe() and
> ends either with driver->probe() failure, or the driver->remove() call.
> Note that the driver->remove() call is invoked not only for
> platform-device removal, but also driver "unbind" events. So the
> "destroyed on platform device removal" is the least likely way that
> these allocations are torn down given ACPI0012 devices are never
> removed.
>
> Outside of that, my main concern about this patch is that I expect it
> breaks unit tests. The infrastructure in
> tools/testing/nvdimm/test/nfit.c emulates an ACPI0012 device that allows
> for deeper regression testing given hardware is difficult to come by,
> and because QEMU does not implement some of the tricky corner cases that
> the unit tests cover.
>
> This needs to pass tests, but fair warning, 
> tools/testing/nvdimm/test/nfit.c does some non-idiomatic + "strange"
> things to achieve deeper test coverage. So I expect that if this breaks
> tests as I expect the effort needed to fix the emulation could be
> significant.
>
> If you want to give running the tests a try the easiest would be to use
> "run_qemu.sh" with --nfit-test option [1], or you can try to setup an
> environment manually using the ndctl instructions [2].
>
> [1]: https://github.com/pmem/run_qemu
> [2]: https://github.com/pmem/ndctl#readme

Thanks a lot !
I will run qemu tests and fix the verbiage,

Michał