Re: [PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[Dave Jiang <dave.jiang@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]



On 10/1/26 9:32 PM, Lucero Palau, Alejandro wrote:
> 
> On 01/10/2026 21:31, Dave Jiang wrote:
>>
>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> PM initialization could not be necessary for some devices.
>>>
>>> Avoid checking for supplier PM initialization if so.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/base/core.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 4c0c373998a1..bf0513beafad 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
>>>        * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>>>        * because it only affects sync_state() callbacks.
>>>        */
>>> -    if (!device_pm_initialized(supplier)
>>> +    if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))
>>>           || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>>             device_is_dependent(consumer, supplier))) {
>>>           link = NULL;
>> A no PM supplier can now be linked at any point: before device_add(), while it fails, or after device_del(). Maybe replace with a helper like this?
> 
> 
> I would say you can not use a device as supplier before device_add() happens for such a supplier, and if it does happen after device_del(), something is wrong with the caller.

Yes that would be a bug, and device_link_add() is suppose to catch it. For PM devices, the device_pm_initialized() test is the gate. The change you made skips that test for no PM device case. I had LLM created a test module for verification, attached.

Essentially the logic in this patch removed the check for 2 states that the original code used to block.
1. before device_add(supplier)
2. after device_del(supplier) 

> 
> 
> Your suggestion is likely making the code more legible, but it does not change the functionality I added. Does it? Not saying it would not help, but I can not understand your comment for suggesting it which seems to point to potential problems I did not see.
> 

It does.
- before device_add(supplier): this patch creates the link, and the helper refuses it.
- after device_add(supplier): both create it.
- after device_del(supplier), before last put_device: this patch creates the link, and the helper refuses it.

delete_region() or root decoder teardown can unregister the region while the endpoint is still bound. cxl_get_range_and_link() can still be called on a region that has already been through device_del().

Without the helper, it's possible where the PFx driver can device_link_add() a deleted region that is still around due to endpoint still holds a reference.

DJ


> 
>> static bool device_link_supplier_ready(struct device *supplier)
>> {
>>        /* no PM devices never enter dpm_list, so check registration directly */
>>        if (device_pm_not_required(supplier))
>>                return device_is_registered(supplier);
>>
>>        return device_pm_initialized(supplier);
>> }
>>
>> ...
>>
>>        if (!device_link_supplier_ready(supplier) ||
>>            (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>             device_is_dependent(consumer, supplier))) {
>>                link = NULL;
>>                goto out;
>>        }

[-- Attachment #2: mock-pfx.patch --]
[-- Type: text/x-patch, Size: 4509 bytes --]

commit 0551e91e63c6009b71792133e0ef0cff1c0af6dd
Author: Dave Jiang <dave.jiang@intel.com>
Date:   Thu Oct 1 14:13:19 2026 -0700

    TEST ONLY: cxl/test: mock non-PF0 consumer for cxl_get_range_and_link()
    
    Not for submission. Exercises the multi-PF device-link path against the
    cxl_test type-2 accelerator, and self-tests device_link_add() on a no_pm
    supplier across registration.

diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
index 9a24ddc28488..279d89be4a4d 100644
--- a/tools/testing/cxl/test/Kbuild
+++ b/tools/testing/cxl/test/Kbuild
@@ -6,11 +6,13 @@ obj-m += cxl_mock.o
 obj-m += cxl_mock_mem.o
 obj-m += cxl_translate.o
 obj-m += cxl_mock_accel.o
+obj-m += cxl_mock_pfx.o
 
 cxl_test-y := cxl.o
 cxl_test-y += hmem_test.o
 cxl_mock-y := mock.o
 cxl_mock_mem-y := mem.o
 cxl_mock_accel-y := accel.o
+cxl_mock_pfx-y := pfx.o
 
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/test/pfx.c b/tools/testing/cxl/test/pfx.c
new file mode 100644
index 000000000000..55b752ebe9ca
--- /dev/null
+++ b/tools/testing/cxl/test/pfx.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TEST ONLY: mock non-PF0 consumer. Its probe links to the region attached
+ * to the cxl_test type-2 accelerator via cxl_get_range_and_link().
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <cxl/cxl.h>
+
+static char *pf0_name = "cxl_type2_accel.0";
+module_param(pf0_name, charp, 0444);
+
+static struct platform_device *pfx_pdev;
+
+static int cxl_mock_pfx_probe(struct platform_device *pdev)
+{
+	struct device *pf0;
+	struct range range;
+	int rc;
+
+	pf0 = bus_find_device_by_name(&platform_bus_type, NULL, pf0_name);
+	if (!pf0) {
+		dev_info(&pdev->dev, "pfx_test: %s not found\n", pf0_name);
+		return -ENODEV;
+	}
+
+	rc = cxl_get_range_and_link(pf0, &pdev->dev, &range);
+	put_device(pf0);
+	if (rc) {
+		dev_info(&pdev->dev, "pfx_test: link rc=%d\n", rc);
+		/* don't let -EPROBE_DEFER requeue us behind the test's back */
+		return rc == -EPROBE_DEFER ? -EAGAIN : rc;
+	}
+
+	dev_info(&pdev->dev, "pfx_test: link rc=0 range=%pra\n", &range);
+	return 0;
+}
+
+static void cxl_mock_pfx_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "pfx_test: removed\n");
+}
+
+static struct platform_driver cxl_mock_pfx_driver = {
+	.probe = cxl_mock_pfx_probe,
+	.remove = cxl_mock_pfx_remove,
+	.driver = {
+		.name = "cxl_mock_pfx",
+	},
+};
+
+static void dev_release(struct device *dev)
+{
+	kfree(dev);
+}
+
+/* device_link_add() must refuse a no_pm supplier that is not registered */
+static void devlink_no_pm_selftest(struct device *consumer)
+{
+	struct device_link *link;
+	struct device *sup;
+	int pass = 0;
+
+	sup = kzalloc_obj(*sup);
+	if (!sup)
+		return;
+	device_initialize(sup);
+	sup->release = dev_release;
+	device_set_pm_not_required(sup);
+	dev_set_name(sup, "pfx_test_supplier");
+
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm before device_add: link %s\n",
+		link ? "CREATED (FAIL)" : "refused (PASS)");
+	if (link)
+		device_link_del(link);
+	else
+		pass++;
+
+	if (device_add(sup)) {
+		put_device(sup);
+		return;
+	}
+
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm after device_add: link %s\n",
+		link ? "created (PASS)" : "REFUSED (FAIL)");
+	if (link) {
+		device_link_del(link);
+		pass++;
+	}
+
+	device_del(sup);
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm after device_del: link %s\n",
+		link ? "CREATED (FAIL)" : "refused (PASS)");
+	if (link)
+		device_link_del(link);
+	else
+		pass++;
+
+	put_device(sup);
+	pr_info("pfx_test: no_pm selftest %d/3 passed\n", pass);
+}
+
+static int __init cxl_mock_pfx_init(void)
+{
+	int rc;
+
+	pfx_pdev = platform_device_register_simple("cxl_mock_pfx", 1, NULL, 0);
+	if (IS_ERR(pfx_pdev))
+		return PTR_ERR(pfx_pdev);
+
+	devlink_no_pm_selftest(&pfx_pdev->dev);
+
+	rc = platform_driver_register(&cxl_mock_pfx_driver);
+	if (rc)
+		platform_device_unregister(pfx_pdev);
+	return rc;
+}
+module_init(cxl_mock_pfx_init);
+
+static void __exit cxl_mock_pfx_exit(void)
+{
+	platform_driver_unregister(&cxl_mock_pfx_driver);
+	platform_device_unregister(pfx_pdev);
+}
+module_exit(cxl_mock_pfx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("cxl_test: TEST ONLY mock non-PF0 consumer");
+MODULE_IMPORT_NS("CXL");