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 <l...@intel.com>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202503311807.bzauhy5l-...@intel.com/
> Signed-off-by: Sahil Siddiq <sahil...@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-...@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
>
