Re: arch/arm64/kernel/fpsimd.c:1944:55: sparse: sparse: dereference of noderef expression

[Catalin Marinas <catalin.marinas@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On Wed, Jun 01, 2022 at 09:50:39AM +0200, Mark Brown wrote:
> On Tue, May 31, 2022 at 05:57:42PM +0100, Catalin Marinas wrote:
> > This looks wrong indeed. IIUC, something like below but waiting for Mark
> > to confirm:
> > 
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index 819979398127..ac521b3403d6 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -1965,7 +1965,7 @@ void __efi_fpsimd_end(void)
> >  							   0,
> >  							   SVCR_SM_MASK);
> >  					if (!system_supports_fa64())
> > -						ffr = efi_sm_state;
> > +						ffr = false;
> 
> Yes, that looks right - feel free to add my Reviewed-by or I'll
> do that when I see the patch sent properly.

I'm no longer convinced that's the right fix. In __efi_fpsimd_begin(),
ffr is true if either !system_supports_fa64() or SVCR.SM is set. The FFR
state would be saved in this case.

In __efi_fpsimd_end(), shouldn't we just restore the FFR state if
efi_sm_state, irrespective of system_supports_fa64() (as it was checked
in the beginning). Something like:

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 819979398127..8bc9080adab2 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1960,13 +1960,12 @@ void __efi_fpsimd_end(void)
 			 * streaming mode.
 			 */
 			if (system_supports_sme()) {
-				if (__this_cpu_read(efi_sm_state)) {
+				if (__this_cpu_read(efi_sm_state))
 					sysreg_clear_set_s(SYS_SVCR,
 							   0,
 							   SVCR_SM_MASK);
-					if (!system_supports_fa64())
-						ffr = efi_sm_state;
-				}
+				else
+					ffr = false;
 			}
 
 			sve_load_state(sve_state + sve_ffr_offset(sve_max_vl()),

-- 
Catalin