Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <db9ac034-35db-417a-9328-b97e5f9a5a11@intel.com>
Date:   Wed, 18 Oct 2023 17:38:43 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with
 platform_driver
Content-Language: en-US
To:     Dan Williams <dan.j.williams@intel.com>,
        <linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <nvdimm@lists.linux.dev>
CC:     <rafael.j.wysocki@intel.com>, <andriy.shevchenko@intel.com>,
        <lenb@kernel.org>, <vishal.l.verma@intel.com>,
        <ira.weiny@intel.com>
References: <20231006173055.2938160-1-michal.wilczynski@intel.com>
 <20231006173055.2938160-6-michal.wilczynski@intel.com>
 <652ed155ef8e_780ef294f@dwillia2-xfh.jf.intel.com.notmuch>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <652ed155ef8e_780ef294f@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UlRkWXBUQzJ0V2dvMzRyQllRa0ZDWFNieS9mT1pqVWtJRUxWTlVLcHZYRGkw?=
 =?utf-8?B?ZzRaQllDOUtUbTJLd3NINnU0anFvaUxPK3E2Vkg0eVhWWXE0ZWViSUVBajZo?=
 =?utf-8?B?RkQ4RzlkWi9aM3Z5QXQxUThRR0lUNk5TNWV6OVZ6YWRwYjhlT2djN2xJcUNL?=
 =?utf-8?B?VTkzK013K2RzK0I3am44eHF5SWNPbXhRVW83NEdWamd0VkQyN2dpOGFKSkxW?=
 =?utf-8?B?Q0dvU0JaWGZXMHNYWm1lVGZxYlVqR0U1REd3SjRmdUhWTkZhWkR1Tk44aWRU?=
 =?utf-8?B?b3BNTFVKVUNpRWoyd2tRY2l4d1g0TjIvTFUzbDZHOURINXc4aHdZWEYrUmJ5?=
 =?utf-8?B?ZUduUXo5eHBDVU8xZVFFK1JoaHFoLzQyWlJENjhIeWRhK2FjWS84N2lXTVZo?=
 =?utf-8?B?Ky9ic2xxZDlqZlgwY1BocVhTaWJwNDlJWU9jOGFPVjN5cUxMY1B4LzRzU0px?=
 =?utf-8?B?ZkZHRVJXWk1SUENuc0t3NjBmQjhPckdpWlFKUWduQldsQkt2UktZeXR1TE9D?=
 =?utf-8?B?RW54UjNlRG93a2ZmMjdmT1J6T1dOQ09xdFljMDArUTNoZlVnWU5jY1NOV081?=
 =?utf-8?B?VFdEMG1scytDaHdObTVsTjUrd3hDZWwwbXRNbit6UUJybkxRM0tFL1E3bHNy?=
 =?utf-8?B?MEpMeE1PQXR5d0pXc0Q0d3JhTjM2Snp4a0FndjNxYWJ4aE9FbW5HQTZGd2tz?=
 =?utf-8?B?T0EwV05xdXIrMDM3cENzMXVXWVZEdHRXaGR5S3M4Z2V5VFJhb1JSY0ZyNXRZ?=
 =?utf-8?B?Skd2YzZQRENGQnhVMStsRUNMMU1MTlFraU96WDVLK0orZnFNekk0Mkg5aWJQ?=
 =?utf-8?B?eDJEUjZOVWwySnV2QU94NUdZWTNYa1VZRldWMnBMR0svc24zT3I4ZVZTK0FU?=
 =?utf-8?B?SzdHQThGalAzdXBQLzZjUUlZeU15eEQzd2ZQWGRJTlpKaG9IT2J2RFFQYVQz?=
 =?utf-8?B?M3VEMUI2ajlqSGhQQTFoMWlGcEVueGpIQi9oUTAxVWZaY0tDSWplQWxkU2NO?=
 =?utf-8?B?RFQxWHJldCt3TmVRc0lVT0hHYWgxWEZCSDZvWG1acDhwSmVFTEtnaHVkZEdj?=
 =?utf-8?B?U1JVc0p0UTRwcGZvZzd1SzVYQ3VzNVIxVVU5clR2Y0pYNGZScnR2UDN1UTcr?=
 =?utf-8?B?dEpOY3V3U1QybnU2b2V4NDRMbUorRC9EMW01bUpKZU5ZNC9kaks2akFENXFn?=
 =?utf-8?B?L011OWpoOGsrUXErM2VCQ3pDTERXTWlTcVBDNXFWZ0ZjaW5yU0VvWERNdGNp?=
 =?utf-8?B?Q1dwUWpTR3VKVUlxTXQ0eFRObUZBdVh1NWpOVHYwTmZwTVo1Z1N4djlQNVky?=
 =?utf-8?B?WEFRMVVqWWdEUXZVTG45cFdhWFY3amtRLzVmeUJ1aTBkZS9LVit6Y2JQc3c2?=
 =?utf-8?B?dGsrTFc3Q0liMlB3am5KQUFOWTQ4Sy80M2JqWDRIN29kUmFHOWk3WlR2dXpX?=
 =?utf-8?B?dHRpZTdYZjBjVTd4Sk9iSUlsang1K0xGNllwQS9hL1A0QzNwL1JuK0UyTlBL?=
 =?utf-8?B?d3JOYjl0cUVPanM3NG5LbFhJbzdpNXdiakxoSFBKS05RYzZOL2NiazlhOEdm?=
 =?utf-8?B?cmg1VGFZNDU4VkRNNURoNXdSb1dNMXMrdzZUaFhYN3BTbTJrbFo0Vkd0Sk9Z?=
 =?utf-8?B?WHc5WWM0VFZWQ2YzU3QvcmZKTDVQYnl4ellXSmJaZFZzWGhyRmdpTnFWZVlK?=
 =?utf-8?B?S0RTYVRVbVFVZ2cvR3JYVmJReUtpY2JHUCtXYVBiMms3R1kyWURhU3VrTUNx?=
 =?utf-8?B?ckwwWVdvd1NlMWo5N081RjVvUGF3TFhiUGxQeUYzQ2F1UENXUGhVYzN5aytR?=
 =?utf-8?B?OTNOeWsrc3dXTktuSzZvMjdHMXNzREZsUGg1bHJHMTRIMXk2MndETTV5eW9J?=
 =?utf-8?B?TkRXUVNZYVdQRklTbXRsUHBCWkNxd2ZHdCt4aHc5TUplaEN0Q24yZGlaemYy?=
 =?utf-8?B?NXVWLzVReTkvTVFsbWNIL3d2WlVEcDNCaXJhL2s2WHlQUEN3YldVdGMybkU4?=
 =?utf-8?B?YnNHRGR2d0YrNjVyeWYvYjUzZUlLZ0VCbGFlOFUvNWN2LzVQelVwbDgzQ2xz?=
 =?utf-8?B?a2V5azk3ZFF6c09ybEpWS1lMdnlCTXdLY09VeHc5c2pwRW5ZU05qdHVHSFdx?=
 =?utf-8?B?b01INDhiTG1vWWJKRU5iMUwydWRpWjdRWnFua3pjY3MzdmMyQWpNeElpRTBk?=
 =?utf-8?B?TkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 18e7a8d5-f45b-477b-0b1d-08dbcff052cb
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2023 15:38:49.4125
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PM14BpfH3E1jnVZ0ZG3lW+T8Pv8sNaQrgxutfrx6jDZeXF0O4lmJTagsKDfrqc+exyDIkV9khOOYJUttHKosLrp4u3OLBaBH9droo8jcsgE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5136
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 18 Oct 2023 08:39:05 -0700 (PDT)


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ł