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] [day] [month] [year] [list]

Message-ID: <db9ac034-35db-417a-9328-b97e5f9a5a11@intel.com>
Date:   Wed, 18 Oct 2023 17:38:43 +0200
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     Dan Williams <dan.j.williams@...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>, <vishal.l.verma@...el.com>,
        <ira.weiny@...el.com>
Subject: Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with
 platform_driver



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ł