[PATCH RFC] Why does cpu_cache_invalidate_memregion() return 0 if there are no handlers?

[PATCH RFC] Why does cpu_cache_invalidate_memregion() return 0 if there are no handlers?

[Bence Csokas]

I noticed cpu_cache_invalidate_memregion() returns 0 if there are no
registered `cache_coherency_ops_inst`s, which I find counter-intuitive.

Is this intended?

From what I've seen, there are no in-tree users who do not check
cpu_cache_has_invalidate_memregion(). However, IMO the better solution
would be if the function would perform this check, and not the callee.

Furthermore, arch/x86/mm/pat/set_memory.c also implements this function,
but actually does the feature check, and WARNs and returns -ENXIO if it
fails that check.

As part of this RFC, I'll attach a patch that provides a low-overhead fix.

Signed-off-by: Bence Csokas <bence.csokas@arm.com>
---
Bence Csokas (1):
      cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

 lib/cache_maint.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20260921-cci_enxio-f3d31d9acec7

Best regards,
--  
Bence Csokas <bence.csokas@arm.com>

[PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Bence Csokas]

Currently, if there are no registered handlers,
cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
of calling this function. If a caller forgets to do this, they may
erroneously think the operation succeeded. This is in contrast to x86,
where cpu_cache_invalidate_memregion() itself checks if the operation is
supported, and returns -ENXIO if it is not.

Signed-off-by: Bence Csokas <bence.csokas@arm.com>
---
 lib/cache_maint.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/cache_maint.c b/lib/cache_maint.c
index 9256a9ffc34c..132d2816d6b1 100644
--- a/lib/cache_maint.c
+++ b/lib/cache_maint.c
@@ -59,12 +59,12 @@ static int cache_inval_done_one(struct cache_coherency_ops_inst *cci)
 
 static int cache_invalidate_memregion(phys_addr_t addr, size_t size)
 {
-	int ret;
 	struct cache_coherency_ops_inst *cci;
 	struct cc_inval_params params = {
 		.addr = addr,
 		.size = size,
 	};
+	int ret = -ENXIO;
 
 	guard(rwsem_read)(&cache_ops_instance_list_lock);
 	list_for_each_entry(cci, &cache_ops_instance_list, node) {
@@ -78,7 +78,7 @@ static int cache_invalidate_memregion(phys_addr_t addr, size_t size)
 			return ret;
 	}
 
-	return 0;
+	return ret;
 }
 
 struct cache_coherency_ops_inst *

-- 
2.55.0

Re: [PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Jonathan Cameron]

On Tue, 22 Sep 2026 09:29:31 +0200
Bence Csokas <bence.csokas@arm.com> wrote:

> Currently, if there are no registered handlers,
> cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
> cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
> of calling this function. If a caller forgets to do this, they may
> erroneously think the operation succeeded. This is in contrast to x86,
> where cpu_cache_invalidate_memregion() itself checks if the operation is
> supported, and returns -ENXIO if it is not.
> 
> Signed-off-by: Bence Csokas <bence.csokas@arm.com>

As you noted short of a race we should never hit this in practice as
all callers must check for support first. Still good to close
that race.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Jonathan

> ---
>  lib/cache_maint.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/cache_maint.c b/lib/cache_maint.c
> index 9256a9ffc34c..132d2816d6b1 100644
> --- a/lib/cache_maint.c
> +++ b/lib/cache_maint.c
> @@ -59,12 +59,12 @@ static int cache_inval_done_one(struct cache_coherency_ops_inst *cci)
>  
>  static int cache_invalidate_memregion(phys_addr_t addr, size_t size)
>  {
> -	int ret;
>  	struct cache_coherency_ops_inst *cci;
>  	struct cc_inval_params params = {
>  		.addr = addr,
>  		.size = size,
>  	};
> +	int ret = -ENXIO;
>  
>  	guard(rwsem_read)(&cache_ops_instance_list_lock);
>  	list_for_each_entry(cci, &cache_ops_instance_list, node) {
> @@ -78,7 +78,7 @@ static int cache_invalidate_memregion(phys_addr_t addr, size_t size)
>  			return ret;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  struct cache_coherency_ops_inst *
> 

Re: [PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Andrew Morton]

On Tue, 22 Sep 2026 09:29:31 +0200 Bence Csokas <bence.csokas@arm.com> wrote:

> Currently, if there are no registered handlers,
> cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
> cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
> of calling this function. If a caller forgets to do this, they may
> erroneously think the operation succeeded. This is in contrast to x86,
> where cpu_cache_invalidate_memregion() itself checks if the operation is
> supported, and returns -ENXIO if it is not.

Do you expect any callers to be altered as a result of this change?

If so, full details would be helpful.

Re: [PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Bence Csókás]

Hi,

On 2026. 09. 23. 4:18, Andrew Morton wrote:
> On Tue, 22 Sep 2026 09:29:31 +0200 Bence Csokas <bence.csokas@arm.com> wrote:
> 
>> Currently, if there are no registered handlers,
>> cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
>> cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
>> of calling this function. If a caller forgets to do this, they may
>> erroneously think the operation succeeded. This is in contrast to x86,
>> where cpu_cache_invalidate_memregion() itself checks if the operation is
>> supported, and returns -ENXIO if it is not.
> 
> Do you expect any callers to be altered as a result of this change?
> 
> If so, full details would be helpful.

Current callers, not necessarily. Future callers, maybe.

Presently, as far as I see, there are 2 consumers: CXL and NVDIMM, with 
3 call sites in total.

2 of these call sites [1] [2] are in the form of:

	if (!has_invalidate()) {
		if (TEST)
			return 0;
		return -ENXIO;
	}
	invalidate();

These types of call sites can stay as-is.

The remaining one [3] is in a void function, and in the form:

	if (has_invalidate())
		invalidate();

Lastly, while not a consumer, here's what x86 [4] does. It's possible a 
future caller will want to replicate this:

	if (!has_invalidate())
		return -ENXIO;
	invalidate();

These types of call sites can do away with calling has_invalidate() 
going forward.

[1] 
https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/cxl/core/region.c#L242
[2] 
https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region_devs.c#L93
[3] 
https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region.c#L113
[4] 
https://elixir.bootlin.com/linux/v7.3-rc3/source/arch/x86/mm/pat/set_memory.c#L372

Bence

Re: [PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Jonathan Cameron]

On Wed, 23 Sep 2026 10:21:15 +0200
Bence Csókás <bence.csokas@arm.com> wrote:

> Hi,
> 
> On 2026. 09. 23. 4:18, Andrew Morton wrote:
> > On Tue, 22 Sep 2026 09:29:31 +0200 Bence Csokas <bence.csokas@arm.com> wrote:
> >   
> >> Currently, if there are no registered handlers,
> >> cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
> >> cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
> >> of calling this function. If a caller forgets to do this, they may
> >> erroneously think the operation succeeded. This is in contrast to x86,
> >> where cpu_cache_invalidate_memregion() itself checks if the operation is
> >> supported, and returns -ENXIO if it is not.  
> > 
> > Do you expect any callers to be altered as a result of this change?
> > 
> > If so, full details would be helpful.  
> 
> Current callers, not necessarily. Future callers, maybe.

There is a race condition if someone unbinds the handlers that might
expose an error after this (correctly) when it didn't before.

In general that race exists in the setup path as well because we don't have
any way to know 'how many' devices to expect with assumption being
they have all turned up if any have.  Not great, but short of adding
something to the firmware specification there wasn't a solution and
that process would be slow.  We can revisit that if it becomes a problem
in practice.


> 
> Presently, as far as I see, there are 2 consumers: CXL and NVDIMM, with 
> 3 call sites in total.
> 
> 2 of these call sites [1] [2] are in the form of:
> 
> 	if (!has_invalidate()) {
> 		if (TEST)
> 			return 0;
> 		return -ENXIO;
> 	}
> 	invalidate();
> 
> These types of call sites can stay as-is.
> 
> The remaining one [3] is in a void function, and in the form:
> 
> 	if (has_invalidate())
> 		invalidate();
> 
> Lastly, while not a consumer, here's what x86 [4] does. It's possible a 
> future caller will want to replicate this:
> 
> 	if (!has_invalidate())
> 		return -ENXIO;
> 	invalidate();
> 
> These types of call sites can do away with calling has_invalidate() 
> going forward.

I'd be against any callers doing that as would rely on future implementations
of the interface.  No support vs got an error tend to need different handling
and relying on particular error codes is likely to bite us eventually.


Jonathan

> 
> [1] 
> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/cxl/core/region.c#L242
> [2] 
> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region_devs.c#L93
> [3] 
> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region.c#L113
> [4] 
> https://elixir.bootlin.com/linux/v7.3-rc3/source/arch/x86/mm/pat/set_memory.c#L372
> 
> Bence

Re: [PATCH RFC] cache: Make cpu_cache_invalidate_memregion() error out if there are no handlers

[Bence Csókás]

Hi,

On 2026. 09. 23. 19:31, Jonathan Cameron wrote:
> On Wed, 23 Sep 2026 10:21:15 +0200
> Bence Csókás <bence.csokas@arm.com> wrote:
> 
>> Hi,
>>
>> On 2026. 09. 23. 4:18, Andrew Morton wrote:
>>> On Tue, 22 Sep 2026 09:29:31 +0200 Bence Csokas <bence.csokas@arm.com> wrote:
>>>    
>>>> Currently, if there are no registered handlers,
>>>> cpu_cache_invalidate_memregion() returns 0. Callers are expected to check
>>>> cpu_cache_has_invalidate_memregion() and return -ENXIO themselves, instead
>>>> of calling this function. If a caller forgets to do this, they may
>>>> erroneously think the operation succeeded. This is in contrast to x86,
>>>> where cpu_cache_invalidate_memregion() itself checks if the operation is
>>>> supported, and returns -ENXIO if it is not.
>>>
>>> Do you expect any callers to be altered as a result of this change?
>>>
>>> If so, full details would be helpful.
>>
>> Current callers, not necessarily. Future callers, maybe.
> 
> There is a race condition if someone unbinds the handlers that might
> expose an error after this (correctly) when it didn't before.
> 
> In general that race exists in the setup path as well because we don't have
> any way to know 'how many' devices to expect with assumption being
> they have all turned up if any have.  Not great, but short of adding
> something to the firmware specification there wasn't a solution and
> that process would be slow.  We can revisit that if it becomes a problem
> in practice.
> 
> 
>>
>> Presently, as far as I see, there are 2 consumers: CXL and NVDIMM, with
>> 3 call sites in total.
>>
>> 2 of these call sites [1] [2] are in the form of:
>>
>> 	if (!has_invalidate()) {
>> 		if (TEST)
>> 			return 0;
>> 		return -ENXIO;
>> 	}
>> 	invalidate();
>>
>> These types of call sites can stay as-is.
>>
>> The remaining one [3] is in a void function, and in the form:
>>
>> 	if (has_invalidate())
>> 		invalidate();
>>
>> Lastly, while not a consumer, here's what x86 [4] does. It's possible a
>> future caller will want to replicate this:
>>
>> 	if (!has_invalidate())
>> 		return -ENXIO;
>> 	invalidate();
>>
>> These types of call sites can do away with calling has_invalidate()
>> going forward.
> 
> I'd be against any callers doing that as would rely on future implementations
> of the interface.  No support vs got an error tend to need different handling
> and relying on particular error codes is likely to bite us eventually.

Yes, if the callers need to handle these separate cases (as in call 
sites [1] and [2]), then for sure keep the existing logic. But some call 
sites may not need to handle these cases differently, and will simply 
return -ENXIO if they find no support. In those cases, they may simply do

	return cpu_cache_invalidate_memregion();

(I now realize that I forgot the `return` in my previous post, sorry for 
the confusion.)

But at the end of the day, I don't have any strong opinion on this. For 
now, current callers can stay as-is, and for future callers, we'll 
decide in that thread.

> Jonathan
> 
>>
>> [1]
>> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/cxl/core/region.c#L242
>> [2]
>> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region_devs.c#L93
>> [3]
>> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/nvdimm/region.c#L113
>> [4]
>> https://elixir.bootlin.com/linux/v7.3-rc3/source/arch/x86/mm/pat/set_memory.c#L372
>>
>> Bence
> 

So the consensus is that my hunch was correct, and this is something to 
address? Should I resubmit this patch as non-RFC then?

Bence