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ł
