Message-ID: <Z-6VyRhGdInVidsw@antec>
Date: Thu, 3 Apr 2025 15:06:01 +0100
From: Stafford Horne <shorne@...il.com>
To: Sahil Siddiq <icegambit91@...il.com>
Cc: jonas@...thpole.se, stefan.kristiansson@...nalahti.fi,
	sahilcdq@...ton.me, linux-openrisc@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] openrisc: Fix build warning in cache.c

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@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202503311807.BZaUHY5L-lkp@intel.com/
> Signed-off-by: Sahil Siddiq <sahilcdq@...ton.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
> 

Powered by blists - more mailing lists