Re: [PATCH v6 3/9] cxl: Add reset-idle and cache flush helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Dereferencing an ERR_PTR as a device pointer leads to a kernel panic.
- [Medium] Using a pointer address as an XArray index creates an unnecessarily deep radix tree, wasting memory.
- [Low] Silent truncation of region size when calling cpu_cache_invalidate_memregion() on 32-bit systems.
--

commit 87c5409ded1cea23c3df266a42ac4769f6e16636
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Add reset-idle and cache flush helpers

This commit adds helpers to collect CXL regions affected by a memdev
reset, verify they are idle, and invalidate CPU caches for those regions.
These helpers use an xarray to track regions and prepare the data path
for the actual reset orchestration.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index d1f487b3d809a..318744695f628 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
[ ... ]
> @@ -926,3 +928,171 @@ int cxl_port_get_possible_dports(struct cxl_port *port)
[ ... ]
> +static int cxl_reset_add_region(struct cxl_reset_region_context *ctx,
> +				struct cxl_region *cxlr)
> +{
> +	int rc;
> +
> +	if (!cxlr || !cxlr->params.res)
> +		return 0;
> +
> +	rc = xa_insert(&ctx->regions, (unsigned long)cxlr, cxlr, GFP_KERNEL);

[Severity: Medium]
Since kernel pointers have their highest bits set on 64-bit systems, won't
using the pointer address as the XArray index force the radix tree to its
maximum depth and unnecessarily waste memory?

Could cxlr->id be used as the index instead to deduplicate regions more
efficiently?

> +
> +	/* A region may be referenced by multiple affected endpoint decoders. */
> +	return rc == -EBUSY ? 0 : rc;
> +}
[ ... ]
> +static int __maybe_unused
> +cxl_reset_collect_memdev_regions(struct cxl_reset_region_context *ctx,
> +				 struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint;
> +
> +	if (!cxlmd || !cxlmd->cxlds)
> +		return -ENODEV;
> +
> +	endpoint = cxlmd->endpoint;
> +	if (!endpoint)

[Severity: High]
If cxlmd->endpoint hasn't been successfully probed and attached yet, it
holds an ERR_PTR value rather than NULL. Will this check fail to catch the
error pointer, allowing it to be passed into device_for_each_child()?

If so, dereferencing &endpoint->dev would cause a kernel panic. Should this
check use IS_ERR(endpoint) instead?

> +		return 0;
> +
> +	return device_for_each_child(&endpoint->dev, ctx,
> +				     cxl_reset_collect_region);
> +}
[ ... ]
> +static int cxl_reset_flush_region_cache(struct cxl_region *cxlr)
> +{
> +	struct resource *res = cxlr->params.res;
> +	int rc;
> +
> +	if (!res)
> +		return 0;
> +
> +	rc = cpu_cache_invalidate_memregion(res->start, resource_size(res));

[Severity: Low]
Can this silently truncate the region size on 32-bit systems?

The function cpu_cache_invalidate_memregion() takes a size_t for its length
parameter, while resource_size() returns a resource_size_t. On 32-bit
systems with PAE enabled, resource_size_t is 64-bit while size_t is 32-bit,
which could cause regions larger than 4GB to be partially skipped.

> +	if (rc)
> +		dev_err(&cxlr->dev, "Failed to invalidate CPU cache [%pr]: %d\n",
> +			res, rc);
> +
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528083154.137979-1-smadhavan@nvidia.com?part=3