Re: [PATCH v3 3/6] cxl/test: Refactor platform device enumerations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing error code assignments on `platform_device_alloc` failure cause populate helpers to falsely return success, leading to an invalid pointer dereference crash.
- [Medium] Error handling cleanup loops mistakenly use `break` instead of `continue` in reverse iteration array cleanup functions, causing resource leaks on error paths.
--

commit 166861bc24df0a0108568d617198fe4532f08924
Author: Dave Jiang <dave.jiang@intel.com>
cxl/test: Refactor platform device enumerations

This commit refactors the platform device enumeration in the cxl test
module by splitting host bridges, root ports, and switch port setups
into separate helper functions.

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index e91c125d557d9..b29be2597064e 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
[ ... ]
> +static void host_bridges_remove(void)
> +{
> +	int i;
> +
> +	for (i = ARRAY_SIZE(cxl_host_bridge) - 1; i >= 0; i--) {
> +		struct platform_device *pdev = cxl_host_bridge[i];
> +
> +		if (!pdev)
> +			break;

[Severity: Medium]
Since this cleanup loop iterates backwards from the end of the array, won't
a break here immediately terminate the loop on the first empty slot?

In the original code, this used a continue to skip empty slots and
successfully reach the populated devices at lower indices. Does this cause
resource leaks when initialization fails partway through?

This same pattern appears to be repeated in cxl_rootports_remove(),
cxl_usps_remove(), and cxl_dsps_remove().

[ ... ]
> +static int host_bridges_populate(void)
> +{
> +	int rc = 0;
> +
> +	for (int i = 0; i < ARRAY_SIZE(cxl_host_bridge); i++) {
>  		struct acpi_device *adev = &host_bridge[i];
>  		struct platform_device *pdev;
>  
[ ... ]
> +		pdev = platform_device_alloc("cxl_host_bridge", i);
> +		if (!pdev)
> +			goto err_bridge;

[Severity: High]
If platform_device_alloc() fails here, does this function return 0
instead of an error code?

Because rc is initialized to 0, returning 0 would cause the main init
function to assume success and proceed.

If initialization proceeds to cxl_rootports_populate(), it will try to set
the parent pointer using a NULL bridge device:

cxl_rootports_populate()
    bridge = cxl_host_bridge[...];
    pdev->dev.parent = &bridge->dev;

Could this cause an invalid pointer dereference when platform_device_add()
is subsequently called? Should this set rc = -ENOMEM before jumping to
the error label?

This missing error code assignment also appears to happen in the new
cxl_rootports_populate(), cxl_usps_populate(), and cxl_dsps_populate()
helper functions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610222328.2272427-1-dave.jiang@intel.com?part=3