Commit c5c6fd8be51207f0abd3 ("openrisc: Introduce new utility functions
to flush and invalidate caches") introduced new functions to flush or
invalidate a range of addresses. These functions make use of the mtspr
macro.

The kernel test robot reported an asm constraint-related warning and
error related to the usage of mtspr in these functions. This is because
the compiler is unable to verify that the constraints are not breached
by functions which call cache_loop_page().

Make cache_loop_page() inline so that the compiler can verify the
constraints of the operands used in mtspr.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202503311807.BZaUHY5L-lkp@intel.com/
Signed-off-by: Sahil Siddiq <sahilcdq@proton.me>
---
Hi,

I noticed that the previous patches have already been merged in the
for-next branch of openrisc's repo. So, I thought I would send a separate
patch to fix this.

I managed to reproduce the warning and error using or1k-linux-gcc 13.3
following the instructions found here [1]. The issue is not with the usage
of unsigned int in place of unsigned short. I tried replacing int with
short but that didn't work either. Apart from this, I see there are no
issues with mfspr even though this uses unsigned long for the register
"ret" and for the immediate value "add".

Also, from the gcc manual [2]:

> The compiler cannot check whether the operands have data types that are
> reasonable for the instruction being executed.

I am not sure if the above is just for output operands or all operands.

In mtspr, __spr is constrained to be an immediate value. I noticed that
all calls to mtspr (except for the ones called via cache_loop_page())
have the value of __spr evaluated to a constant at compile time. However,
the compiler is unable to determine if the constraints of the operands
are violated when mtspr is called via cache_loop_page(). Making
cache_loop_page() __always_inline solves this problem since this results
in the value of __spr being evaluated before compilation is completed.

This warning/error can also be observed for mfspr by changing its
prototype to:

__attribute__((__noinline__))
static unsigned long mfspr(unsigned long add) {...}

The compiler did not throw any other warnings/errors on my machine.

Thanks,
Sahil

[1] https://download.01.org/0day-ci/archive/20250331/202503311807.BZaUHY5L-lkp@intel.com/reproduce
[2] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Output-Operands

 arch/openrisc/mm/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c
index 7bdd07cfca60..0216d65e6f86 100644
--- a/arch/openrisc/mm/cache.c
+++ b/arch/openrisc/mm/cache.c
@@ -40,7 +40,7 @@ static __always_inline void cache_loop(unsigned long paddr, unsigned long end,
 	}
 }
 
-static void cache_loop_page(struct page *page, const unsigned int reg,
+static __always_inline void cache_loop_page(struct page *page, const unsigned int reg,
 					    const unsigned int cache_type)
 {
 	unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
-- 
2.48.1

Hi Sahil,


On Wed, Apr 02, 2025 at 01:31:29AM +0530, Sahil Siddiq wrote:
> Commit c5c6fd8be51207f0abd3 ("openrisc: Introduce new utility functions
> to flush and invalidate caches") introduced new functions to flush or
> invalidate a range of addresses. These functions make use of the mtspr
> macro.
> 
> The kernel test robot reported an asm constraint-related warning and
> error related to the usage of mtspr in these functions. This is because
> the compiler is unable to verify that the constraints are not breached
> by functions which call cache_loop_page().
> 
> Make cache_loop_page() inline so that the compiler can verify the
> constraints of the operands used in mtspr.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202503311807.BZaUHY5L-lkp@intel.com/
> Signed-off-by: Sahil Siddiq <sahilcdq@proton.me>
> ---
> Hi,
> 
> I noticed that the previous patches have already been merged in the
> for-next branch of openrisc's repo. So, I thought I would send a separate
> patch to fix this.
> 
> I managed to reproduce the warning and error using or1k-linux-gcc 13.3
> following the instructions found here [1]. The issue is not with the usage
> of unsigned int in place of unsigned short. I tried replacing int with
> short but that didn't work either. Apart from this, I see there are no
> issues with mfspr even though this uses unsigned long for the register
> "ret" and for the immediate value "add".
> 
> Also, from the gcc manual [2]:
> 
> > The compiler cannot check whether the operands have data types that are
> > reasonable for the instruction being executed.
> 
> I am not sure if the above is just for output operands or all operands.
> 
> In mtspr, __spr is constrained to be an immediate value. I noticed that
> all calls to mtspr (except for the ones called via cache_loop_page())
> have the value of __spr evaluated to a constant at compile time. However,
> the compiler is unable to determine if the constraints of the operands
> are violated when mtspr is called via cache_loop_page(). Making
> cache_loop_page() __always_inline solves this problem since this results
> in the value of __spr being evaluated before compilation is completed.
> 
> This warning/error can also be observed for mfspr by changing its
> prototype to:
> 
> __attribute__((__noinline__))
> static unsigned long mfspr(unsigned long add) {...}
> 
> The compiler did not throw any other warnings/errors on my machine.
> 
> Thanks,
> Sahil
> 
> [1] https://download.01.org/0day-ci/archive/20250331/202503311807.BZaUHY5L-lkp@intel.com/reproduce
> [2] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Output-Operands

I will just take this fix and apply it to the series (git fixup) rather than
take this patch as is.  Also, as registers should be unsigned short, I think
we should change the type to that.

I will fixup patches in place.

-Stafford

>  arch/openrisc/mm/cache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c
> index 7bdd07cfca60..0216d65e6f86 100644
> --- a/arch/openrisc/mm/cache.c
> +++ b/arch/openrisc/mm/cache.c
> @@ -40,7 +40,7 @@ static __always_inline void cache_loop(unsigned long paddr, unsigned long end,
>  	}
>  }
>  
> -static void cache_loop_page(struct page *page, const unsigned int reg,
> +static __always_inline void cache_loop_page(struct page *page, const unsigned int reg,
>  					    const unsigned int cache_type)
>  {
>  	unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
> -- 
> 2.48.1
>

Hi Stafford,

On 4/3/25 7:36 PM, Stafford Horne wrote:
> [...]
> I will just take this fix and apply it to the series (git fixup) rather than
> take this patch as is.  Also, as registers should be unsigned short, I think
> we should change the type to that.
> 
> I will fixup patches in place.
> 

Sure thing. Will you be changing the register types as well in place with this
fix? Let me know if I should send v2 of this patch so the changes are together.

Also, should the types used in mfspr also be changed?

Thanks,
Sahil