Re: [PATCH] cxl/region: Create node access attributes for CFMWS-only NUMA nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] cxl: backwards naming of cxl_need_node_perf_attrs_update()
--

commit b11d76869ff914a767abdc38146d89e156a8de78
Author: Guixin Liu <kanie@linux.alibaba.com>

cxl/region: Create node access attributes for CFMWS-only NUMA nodes

This patch handles a case where a NUMA node only backs a CXL fixed memory
window without being enumerated by SRAT. When such a node is not backed by
a real SRAT PXM, it creates the node access attributes using
node_set_perf_attrs() rather than just trying to update them.

> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index df9261ba84e5a..1ac7ce3a6127e 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c

[ ... ]

> @@ -1072,3 +1072,8 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
>  		cxlr->coord[i].write_bandwidth += perf->coord[i].write_bandwidth;
>  	}
>  }
> +
> +bool cxl_need_node_perf_attrs_update(int nid)
> +{
> +	return !acpi_node_backed_by_real_pxm(nid);
> +}

[Severity: Low]
Does this function name accurately describe what it checks for? It seems to
return true when the node is NOT backed by a real SRAT PXM, which means it
needs creation rather than an update.

[ ... ]

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 4f6069451fed6..11902e7f6a1c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c

[ ... ]

> @@ -2631,7 +2631,10 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  
>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
>  		if (cxlr->coord[i].read_bandwidth) {
> -			node_update_perf_attrs(nid, &cxlr->coord[i], i);
> +			if (cxl_need_node_perf_attrs_update(nid))
> +				node_set_perf_attrs(nid, &cxlr->coord[i], i);
> +			else
> +				node_update_perf_attrs(nid, &cxlr->coord[i], i);

[Severity: Low]
Is the logic backwards here because of the helper name? 

If cxl_need_node_perf_attrs_update() returns true, this calls
node_set_perf_attrs() to create the attributes. If it returns false, it
calls node_update_perf_attrs() to update them.

This effectively reads as "if we need to update, we set; otherwise we
update". Would renaming the helper to something like
cxl_need_node_perf_attrs_creation() make this logic clearer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916120338.369436-1-kanie@linux.alibaba.com?part=1