Re: [PATCH] cxl/acpi: Defer probe when ACPI0016 PCI root bridge is not ready

[Chen Pei <cp0613@linux.alibaba.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2420 bytes --]

On Tue, 26 May 2026 03:12:00 +0000, sashiko-bot@kernel.org wrote:

> > +
> > +	/* Clear _DEP dependencies to allow consumers to enumerate */
> > +	acpi_dev_clear_dependencies(device);
> > +
> >  	return 1;
> >  
> >  remove_dmar:
> >  	if (hotadd)
> >  		dmar_device_remove(handle);
> >  end:
> >  	kfree(root);
> 
> [Severity: High]
> This is a pre-existing issue, but does the dmar_device_add() error path
> leave a dangling pointer in device->driver_data?
> 
> Earlier in acpi_pci_root_add(), the allocated root object is assigned to
> device->driver_data:
> 
> 	root->device = device;
> 	root->segment = segment & 0xFFFF;
> 	device->driver_data = root;
> 
> If dmar_device_add() fails during a hot-add, the code jumps to the end
> label and frees the root pointer via kfree().
> 
> However, it doesn't explicitly reset device->driver_data to NULL, unlike
> the later pci_acpi_scan_root() error path. Since the ACPI core does not
> automatically clear driver_data on an attach failure, could subsequent calls
> to acpi_pci_find_root() by other subsystems fetch and dereference this freed
> pointer?

Good catch — you're right. On the dmar_device_add() failure path, the
code jumps to end: and frees root via kfree(), but
device->driver_data is left pointing at the freed memory. The later
pci_acpi_scan_root() failure path explicitly clears it to NULL, which
makes the omission on the dmar path look like an oversight rather than
intentional.

Since this is unrelated to the _DEP clearing change in this patch, I'd
prefer to keep it out of this patch and send a separate fix on top. Both
failure paths converge at end: and need the same cleanup, so I plan to
consolidate the NULL assignment there:

@@ -724,7 +724,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
		if (!root->bus) {
				dev_err(&device->dev,
						"Bus %04x:%02x not present in PCI namespace\n",
						root->segment, (unsigned int)root->secondary.start);
-				device->driver_data = NULL;
				result = -ENODEV;
				goto remove_dmar;
		}
@@ -764,6 +763,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
remove_dmar:
		if (hotadd)
				dmar_device_remove(handle);
end:
+		device->driver_data = NULL;
		kfree(root);
		return result;
}

This way every path reaching end: clears driver_data before freeing
root, and any future error path that does goto end; is automatically
correct. Does that split sound OK to you?

Thanks,
Pei