linux-kernel - RE: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <652ed155ef8e_780ef294f@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Tue, 17 Oct 2023 11:24:21 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Michal Wilczynski <michal.wilczynski@...el.com>,
        <linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <nvdimm@...ts.linux.dev>
CC:     <rafael.j.wysocki@...el.com>, <andriy.shevchenko@...el.com>,
        <lenb@...nel.org>, <dan.j.williams@...el.com>,
        <vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
        Michal Wilczynski <michal.wilczynski@...el.com>
Subject: RE: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with
 platform_driver

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