[PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Li Ming]

In CXL subsystem, many functions need to check an address availability
by checking if the resource range contains the address. Providing a new
helper function resource_contains_addr() to check if the resource range
contains the input address.

Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
v2:
* Implement a general helper resource_contains_addr(). (Alison)

base-commit: 0a46f60a9fe16f5596b6b4b3ee1a483ea7854136 cxl/fixes
---
 include/linux/ioport.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index e8b2d6aa4013..75f5e9ccd549 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -308,6 +308,14 @@ static inline bool resource_contains(const struct resource *r1, const struct res
 	return r1->start <= r2->start && r1->end >= r2->end;
 }
 
+/* True if res contains addr */
+static inline bool resource_contains_addr(const struct resource *res, const resource_size_t addr)
+{
+	if (res->flags & IORESOURCE_UNSET)
+		return false;
+	return res->start <= addr && addr <= res->end;
+}
+
 /* True if any part of r1 overlaps r2 */
 static inline bool resource_overlaps(const struct resource *r1, const struct resource *r2)
 {
-- 
2.34.1

[PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

[Li Ming]

DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
of a CXL device. So the correct implementation should be checking if the
DPA is in the DPA range of the CXL device rather than checking if the
DPA is equal to 0.

Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/edac.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 623aaa4439c4..1cf65b1538b9 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
 static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
 {
 	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
+	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
+	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
+	    val != EDAC_DO_MEM_REPAIR)
 		return -EINVAL;
 
 	return cxl_mem_perform_ppr(cxl_ppr_ctx);
-- 
2.34.1

[PATCH v2 3/3] cxl/core: Using resource_contains_addr() to check address availability

[Li Ming]

Helper function resource_contains_addr() can be used to check if a
resource range contains an input address. Use it to replace all
code that checks whether a resource range contains the input
DPA/HPA/SPA.

Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/edac.c   | 4 ++--
 drivers/cxl/core/memdev.c | 2 +-
 drivers/cxl/core/region.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 1cf65b1538b9..a3c19ec8033f 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -1523,7 +1523,7 @@ static int cxl_mem_sparing_set_dpa(struct device *dev, void *drv_data, u64 dpa)
 	struct cxl_memdev *cxlmd = ctx->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-	if (dpa < cxlds->dpa_res.start || dpa > cxlds->dpa_res.end)
+	if (!resource_contains_addr(&cxlds->dpa_res, dpa))
 		return -EINVAL;
 
 	ctx->dpa = dpa;
@@ -1892,7 +1892,7 @@ static int cxl_ppr_set_dpa(struct device *dev, void *drv_data, u64 dpa)
 	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-	if (dpa < cxlds->dpa_res.start || dpa > cxlds->dpa_res.end)
+	if (!resource_contains_addr(&cxlds->dpa_res, dpa))
 		return -EINVAL;
 
 	cxl_ppr_ctx->dpa = dpa;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f88a13adf7fa..4a5dc1d93af1 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -267,7 +267,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
 		dev_dbg(cxlds->dev, "device has no dpa resource\n");
 		return -EINVAL;
 	}
-	if (dpa < cxlds->dpa_res.start || dpa > cxlds->dpa_res.end) {
+	if (!resource_contains_addr(&cxlds->dpa_res, dpa)) {
 		dev_dbg(cxlds->dev, "dpa:0x%llx not in resource:%pR\n",
 			dpa, &cxlds->dpa_res);
 		return -EINVAL;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6e5e1460068d..79d8211f8ed0 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2847,7 +2847,7 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
 	if (!cxled || !cxled->dpa_res || !resource_size(cxled->dpa_res))
 		return 0;
 
-	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
+	if (!resource_contains_addr(cxled->dpa_res, dpa))
 		return 0;
 
 	/*
@@ -2959,7 +2959,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 	if (cxlrd->hpa_to_spa)
 		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);
 
-	if (hpa < p->res->start || hpa > p->res->end) {
+	if (!resource_contains_addr(p->res, hpa)) {
 		dev_dbg(&cxlr->dev,
 			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
 		return ULLONG_MAX;
@@ -3499,7 +3499,7 @@ u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa)
 	xa_for_each(&endpoint->regions, index, iter) {
 		struct cxl_region_params *p = &iter->region->params;
 
-		if (p->res->start <= spa && spa <= p->res->end) {
+		if (resource_contains_addr(p->res, spa)) {
 			if (!p->cache_size)
 				return ~0ULL;
 
-- 
2.34.1

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> In CXL subsystem, many functions need to check an address availability
> by checking if the resource range contains the address. Providing a new
> helper function resource_contains_addr() to check if the resource range
> contains the input address.

resources are about ranges and not addresses. At bare minimum naming sucks
here. Also there is no symmetry with the intersection API. But I would argue
to use resource_contains() and just provide necessary parameter with both
start and end to be set at the same value.

	struct resource r = DEFINE_RES...(addr);

	if (resource_contains, res, &r)
		...do stuff...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> > In CXL subsystem, many functions need to check an address availability
> > by checking if the resource range contains the address. Providing a new
> > helper function resource_contains_addr() to check if the resource range
> > contains the input address.
> 
> resources are about ranges and not addresses. At bare minimum naming sucks
> here. Also there is no symmetry with the intersection API. But I would argue

I meant overlaps, sorry for the confusion.

> to use resource_contains() and just provide necessary parameter with both
> start and end to be set at the same value.
> 
> 	struct resource r = DEFINE_RES...(addr);
> 
> 	if (resource_contains, res, &r)
> 		...do stuff...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> In CXL subsystem, many functions need to check an address availability
> by checking if the resource range contains the address. Providing a new
> helper function resource_contains_addr() to check if the resource range
> contains the input address.

Hint: Use cover letter when you submit more than one patch. It will help a lot
in understanding what's going on. For example, I have a script [1] to submit
patches which makes me sure not to avoid that. Or you can try `b4 relay`...

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 03:20:07PM +0800, Li Ming wrote:
> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
> of a CXL device. So the correct implementation should be checking if the
> DPA is in the DPA range of the CXL device rather than checking if the
> DPA is equal to 0.

Looking at the use cases, I even like the first patch less. You can do
it locally in the macro or function in cxl code. We may see in the future
if something like this will be needed for others.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Alison Schofield]

On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> > In CXL subsystem, many functions need to check an address availability
> > by checking if the resource range contains the address. Providing a new
> > helper function resource_contains_addr() to check if the resource range
> > contains the input address.
> 
> resources are about ranges and not addresses. At bare minimum naming #####
> here. Also there is no symmetry with the intersection API. But I would argue
> to use resource_contains() and just provide necessary parameter with both
> start and end to be set at the same value.
> 
> 	struct resource r = DEFINE_RES...(addr);
> 
> 	if (resource_contains, res, &r)
> 		...do stuff...

Hi Andy,

Thanks for the review. Two alternative approaches were considered:

1) A CXL-only helper with direct comparison:
This duplicates range checking logic that's already common in the
resource API. We'd end up with CXL specific when the resource API
already provides the semantic framework for containment checks.

2) A CXL helper using temporary resource + resource_contains():
While this maintains API consistency, it's unnecessarily heavyweight.
Creating a temporary resource struct just to check if a single address
falls within a range feels like overengineering.
(This is your suggestion above.)

This patch proposes, a new helper in ioport.h, provides a clean, reusable
API that follows the existing resource_contains() pattern while being
optimized for the single address case. I guess I can agree with the lack
of symmetry with the existing API, but that's only if one thinks an API
cannot be extended. The new helper actually complements resource_contains()
nicely: one checks resource vs resource containment, the other checks
address vs resource containment.

Revisited the name, I can see how extending an existing name, like 
resource_contains() is a poor choice because then one might expect the
pair to be resource_contains_resource() and resource_contains_addr()
which it is not.

Do either of these fit better, or something else?
- resource_includes_addr() 
- resource_has_addr()

The new helper would immediately be used 7 times in the CXL subsystem.
What are you thinking the bar is for adding to ioport.h?

-- Alison

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

[Alison Schofield]

On Wed, Jul 02, 2025 at 03:20:07PM +0800, Li Ming wrote:
> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
> of a CXL device. So the correct implementation should be checking if the
> DPA is in the DPA range of the CXL device rather than checking if the
> DPA is equal to 0.
> 
> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  drivers/cxl/core/edac.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 623aaa4439c4..1cf65b1538b9 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
>  static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
>  {
>  	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
> +	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
> -	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
> +	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
> +	    val != EDAC_DO_MEM_REPAIR)
>  		return -EINVAL;

Hi Ming,

I think this one needs a user visible impact statement.

I'm hoping the broader helper gets accepted. That may be the ioport.h
addition, or maybe we end up with a CXL special helper.

However, if this patch is aiming to go upstream as a FIX in a 6.16 rc,
then we are probably better off fixing the check inline right here, and
then you follow on with the other 2 patches to be considered for the
next merge window.

Please share that impact and suggest whether it can wait for next merge
window.

-- Alison


>  
>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Li Ming]

On 7/2/2025 4:21 PM, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
>> In CXL subsystem, many functions need to check an address availability
>> by checking if the resource range contains the address. Providing a new
>> helper function resource_contains_addr() to check if the resource range
>> contains the input address.
> Hint: Use cover letter when you submit more than one patch. It will help a lot
> in understanding what's going on. For example, I have a script [1] to submit
> patches which makes me sure not to avoid that. Or you can try `b4 relay`...
>
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
>
Sure, I will add cover letter in next version, thank you.


Ming

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Andy Shevchenko]

Wed, Jul 02, 2025 at 12:16:07PM -0700, Alison Schofield kirjoitti:
> On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> > > In CXL subsystem, many functions need to check an address availability
> > > by checking if the resource range contains the address. Providing a new
> > > helper function resource_contains_addr() to check if the resource range
> > > contains the input address.
> > 
> > resources are about ranges and not addresses. At bare minimum naming #####
> > here. Also there is no symmetry with the intersection API. But I would argue
> > to use resource_contains() and just provide necessary parameter with both
> > start and end to be set at the same value.
> > 
> > 	struct resource r = DEFINE_RES...(addr);
> > 
> > 	if (resource_contains, res, &r)
> > 		...do stuff...

(1) ^^^

> Thanks for the review. Two alternative approaches were considered:
> 
> 1) A CXL-only helper with direct comparison:
> This duplicates range checking logic that's already common in the
> resource API. We'd end up with CXL specific when the resource API
> already provides the semantic framework for containment checks.

> 2) A CXL helper using temporary resource + resource_contains():
> While this maintains API consistency, it's unnecessarily heavyweight.

How is it? It's mostly matter of stack usage. Do I miss anything?

> Creating a temporary resource struct just to check if a single address
> falls within a range feels like overengineering.
> (This is your suggestion above.)

I don' think so. This won't create anything, it will declare the resource
data on a stack.

...

> This patch proposes, a new helper in ioport.h, provides a clean, reusable
> API that follows the existing resource_contains() pattern while being
> optimized for the single address case. I guess I can agree with the lack
> of symmetry with the existing API, but that's only if one thinks an API
> cannot be extended. The new helper actually complements resource_contains()
> nicely: one checks resource vs resource containment, the other checks
> address vs resource containment.

The point is _there is no address_ in the resources! There is a _range_.

> Revisited the name, I can see how extending an existing name, like 
> resource_contains() is a poor choice because then one might expect the
> pair to be resource_contains_resource() and resource_contains_addr()
> which it is not.
> 
> Do either of these fit better, or something else?
> - resource_includes_addr() 
> - resource_has_addr()

That's why (see above) naming sucks. Doing it local to CXL makes it clearer as
CXL knows the semantic of the range, struct resource doesn't and shouldn't.
(Yeah, I know that we have something there that relies on the range being an
address range, but those APIs provide more and TBH I would rather split them
to a separate header)

> The new helper would immediately be used 7 times in the CXL subsystem.
> What are you thinking the bar is for adding to ioport.h?

It's only a single subsystem, find 3+ more outside and we can reconsider.
Also for the symmetrical API,

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

[Shiju Jose]

>-----Original Message-----
>From: Li Ming <ming.li@zohomail.com>
>Sent: 02 July 2025 08:20
>To: akpm@linux-foundation.org; andriy.shevchenko@linux.intel.com;
>bhelgaas@google.com; ilpo.jarvinen@linux.intel.com; dave@stgolabs.net;
>Jonathan Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dan.j.williams@intel.com; Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org; Li Ming
><ming.li@zohomail.com>
>Subject: [PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation
>
>DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get Partition
>Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get Partition Info(Opcode
>4100h)", it mentions that DPA 0 is a valid address of a CXL device. So the correct
>implementation should be checking if the DPA is in the DPA range of the CXL
>device rather than checking if the DPA is equal to 0.
>
>Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control
>feature")
>Signed-off-by: Li Ming <ming.li@zohomail.com>
Hi Ming,
Thanks for the fix. 

Just found that, along with Table 8-143,  CXL spec 3.2 Device Decode Logic (Page 576) describes as
"The DPA mappings for a device typically start at 'DPA 0' for Decoder[0] and 
are sequentially accumulated with each additional decoder used"

Tested-by: Shiju Jose <shiju.jose@huawei.com>
Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
>---
> drivers/cxl/core/edac.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c index
>623aaa4439c4..1cf65b1538b9 100644
>--- a/drivers/cxl/core/edac.c
>+++ b/drivers/cxl/core/edac.c
>@@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device
>*dev, void *drv_data,  static int cxl_do_ppr(struct device *dev, void *drv_data,
>u32 val)  {
> 	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
>+	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
>+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>
>-	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
>+	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
>+	    val != EDAC_DO_MEM_REPAIR)
> 		return -EINVAL;
>
> 	return cxl_mem_perform_ppr(cxl_ppr_ctx);
>--
>2.34.1


Thanks,
Shiju

RE: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Shiju Jose]

>-----Original Message-----
>From: Li Ming <ming.li@zohomail.com>
>Sent: 02 July 2025 08:20
>To: akpm@linux-foundation.org; andriy.shevchenko@linux.intel.com;
>bhelgaas@google.com; ilpo.jarvinen@linux.intel.com; dave@stgolabs.net;
>Jonathan Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dan.j.williams@intel.com; Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org; Li Ming
><ming.li@zohomail.com>
>Subject: [PATCH v2 1/3] resource: Introduce a new helper
>resource_contains_addr()
>
>In CXL subsystem, many functions need to check an address availability by
>checking if the resource range contains the address. Providing a new helper
>function resource_contains_addr() to check if the resource range contains the
>input address.
>
>Suggested-by: Alison Schofield <alison.schofield@intel.com>
>Signed-off-by: Li Ming <ming.li@zohomail.com>
Tested-by: Shiju Jose <shiju.jose@huawei.com>
>---
>v2:
>* Implement a general helper resource_contains_addr(). (Alison)
>
>base-commit: 0a46f60a9fe16f5596b6b4b3ee1a483ea7854136 cxl/fixes
>---
> include/linux/ioport.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/include/linux/ioport.h b/include/linux/ioport.h index
>e8b2d6aa4013..75f5e9ccd549 100644
>--- a/include/linux/ioport.h
>+++ b/include/linux/ioport.h
>@@ -308,6 +308,14 @@ static inline bool resource_contains(const struct
>resource *r1, const struct res
> 	return r1->start <= r2->start && r1->end >= r2->end;  }
>
>+/* True if res contains addr */
>+static inline bool resource_contains_addr(const struct resource *res,
>+const resource_size_t addr) {
>+	if (res->flags & IORESOURCE_UNSET)
>+		return false;
>+	return res->start <= addr && addr <= res->end; }
>+
> /* True if any part of r1 overlaps r2 */  static inline bool
>resource_overlaps(const struct resource *r1, const struct resource *r2)  {
>--
>2.34.1

Re: [PATCH v2 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

[Li Ming]

On 7/3/2025 3:38 AM, Alison Schofield wrote:
> On Wed, Jul 02, 2025 at 03:20:07PM +0800, Li Ming wrote:
>> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
>> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
>> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
>> of a CXL device. So the correct implementation should be checking if the
>> DPA is in the DPA range of the CXL device rather than checking if the
>> DPA is equal to 0.
>>
>> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>  drivers/cxl/core/edac.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
>> index 623aaa4439c4..1cf65b1538b9 100644
>> --- a/drivers/cxl/core/edac.c
>> +++ b/drivers/cxl/core/edac.c
>> @@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
>>  static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
>>  {
>>  	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
>> +	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  
>> -	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
>> +	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
>> +	    val != EDAC_DO_MEM_REPAIR)
>>  		return -EINVAL;
> Hi Ming,
>
> I think this one needs a user visible impact statement.
>
> I'm hoping the broader helper gets accepted. That may be the ioport.h
> addition, or maybe we end up with a CXL special helper.
>
> However, if this patch is aiming to go upstream as a FIX in a 6.16 rc,
> then we are probably better off fixing the check inline right here, and
> then you follow on with the other 2 patches to be considered for the
> next merge window.
>
> Please share that impact and suggest whether it can wait for next merge
> window.
>
> -- Alison
>
Hi Alison,


I think the impact of this issue is limited, because the issue is inside CXL edac part, just causes that user cannot issue PPR maintenance operation for the DPA 0 of a CXL device. (A PPR maintenance operation requests the CXL device to perform a repair operation on its media.)

I am not sure if cxl subsystem will have another fixes PR for 6.16 rc, my understanding is that it is not worth to submit an additional PR only for this issue. So  I think merging it in next merge window is also good.


Ming

>>  
>>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);
>> -- 
>> 2.34.1
>>

Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Li Ming]

On 7/3/2025 5:31 PM, Andy Shevchenko wrote:
> Wed, Jul 02, 2025 at 12:16:07PM -0700, Alison Schofield kirjoitti:
>> On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
>>> On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
>>>> In CXL subsystem, many functions need to check an address availability
>>>> by checking if the resource range contains the address. Providing a new
>>>> helper function resource_contains_addr() to check if the resource range
>>>> contains the input address.
>>> resources are about ranges and not addresses. At bare minimum naming #####
>>> here. Also there is no symmetry with the intersection API. But I would argue
>>> to use resource_contains() and just provide necessary parameter with both
>>> start and end to be set at the same value.
>>>
>>> 	struct resource r = DEFINE_RES...(addr);
>>>
>>> 	if (resource_contains, res, &r)
>>> 		...do stuff...
> (1) ^^^
>
>> Thanks for the review. Two alternative approaches were considered:
>>
>> 1) A CXL-only helper with direct comparison:
>> This duplicates range checking logic that's already common in the
>> resource API. We'd end up with CXL specific when the resource API
>> already provides the semantic framework for containment checks.
>> 2) A CXL helper using temporary resource + resource_contains():
>> While this maintains API consistency, it's unnecessarily heavyweight.
> How is it? It's mostly matter of stack usage. Do I miss anything?
>
>> Creating a temporary resource struct just to check if a single address
>> falls within a range feels like overengineering.
>> (This is your suggestion above.)
> I don' think so. This won't create anything, it will declare the resource
> data on a stack.
>
> ...
>
>> This patch proposes, a new helper in ioport.h, provides a clean, reusable
>> API that follows the existing resource_contains() pattern while being
>> optimized for the single address case. I guess I can agree with the lack
>> of symmetry with the existing API, but that's only if one thinks an API
>> cannot be extended. The new helper actually complements resource_contains()
>> nicely: one checks resource vs resource containment, the other checks
>> address vs resource containment.
> The point is _there is no address_ in the resources! There is a _range_.
>
>> Revisited the name, I can see how extending an existing name, like 
>> resource_contains() is a poor choice because then one might expect the
>> pair to be resource_contains_resource() and resource_contains_addr()
>> which it is not.
>>
>> Do either of these fit better, or something else?
>> - resource_includes_addr() 
>> - resource_has_addr()
> That's why (see above) naming sucks. Doing it local to CXL makes it clearer as
> CXL knows the semantic of the range, struct resource doesn't and shouldn't.
> (Yeah, I know that we have something there that relies on the range being an
> address range, but those APIs provide more and TBH I would rather split them
> to a separate header)
>
>> The new helper would immediately be used 7 times in the CXL subsystem.
>> What are you thinking the bar is for adding to ioport.h?
> It's only a single subsystem, find 3+ more outside and we can reconsider.
> Also for the symmetrical API,
>
Hi Andy,

Thanks for your review. After checking other module code, there are more than 5 modules/subsystem using similar code to check if a resource contains an "address".

But as you said, struct resource is not only used for address range management, like pci_ecam_map_bus() in drivers/pci/ecam.c, it checks if an resource contains an bus id. So resource_contains_addr() is not a good implementation for all resource type, I will implmement it inside cxl subsystem in next version.


Thanks

Ming