[PATCH] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[PATCH] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Henning Schild]

From: Henning Schild <henning.schild@siemens.com>

We should mark the current task as not owning the fpu anymore if it does
actually own the fpu, not if the fpu itself is active.

Fixes cb52e6c7438fa

Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 kernel/cobalt/arch/x86/thread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cobalt/arch/x86/thread.c b/kernel/cobalt/arch/x86/thread.c
index a09560075..0c6054338 100644
--- a/kernel/cobalt/arch/x86/thread.c
+++ b/kernel/cobalt/arch/x86/thread.c
@@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
 	switch_fpu_finish(&current->thread.fpu, smp_processor_id());
 #else
 	/* mark current thread as not owning the FPU anymore */
-	if (&current->thread.fpu.fpstate_active)
+	if (&current->thread.fpu.fpregs_active)
 		fpregs_deactivate(&current->thread.fpu);
 #endif
 }
-- 
2.19.2

Re: [PATCH] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Jan Kiszka]

On 21.12.18 14:44, Henning Schild via Xenomai wrote:
> From: Henning Schild <henning.schild@siemens.com>
> 
> We should mark the current task as not owning the fpu anymore if it does
> actually own the fpu, not if the fpu itself is active.
> 
> Fixes cb52e6c7438fa
> 
> Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>   kernel/cobalt/arch/x86/thread.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cobalt/arch/x86/thread.c b/kernel/cobalt/arch/x86/thread.c
> index a09560075..0c6054338 100644
> --- a/kernel/cobalt/arch/x86/thread.c
> +++ b/kernel/cobalt/arch/x86/thread.c
> @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
>   	switch_fpu_finish(&current->thread.fpu, smp_processor_id());
>   #else
>   	/* mark current thread as not owning the FPU anymore */
> -	if (&current->thread.fpu.fpstate_active)
> +	if (&current->thread.fpu.fpregs_active)

Well, if you had used fpregs_active() here, you would have resolved also the 
second bug of that line (which I only spotted now):
"if (&something)" is always true...

Jan

>   		fpregs_deactivate(&current->thread.fpu);
>   #endif
>   }
> 

Re: [PATCH] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Mauro Salvini]

On Sat, 2018-12-22 at 12:42 +0100, Jan Kiszka via Xenomai wrote:
> On 21.12.18 14:44, Henning Schild via Xenomai wrote:
> > From: Henning Schild <henning.schild@siemens.com>
> > 
> > We should mark the current task as not owning the fpu anymore if it
> > does
> > actually own the fpu, not if the fpu itself is active.
> > 
> > Fixes cb52e6c7438fa
> > 
> > Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >   kernel/cobalt/arch/x86/thread.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/cobalt/arch/x86/thread.c
> > b/kernel/cobalt/arch/x86/thread.c
> > index a09560075..0c6054338 100644
> > --- a/kernel/cobalt/arch/x86/thread.c
> > +++ b/kernel/cobalt/arch/x86/thread.c
> > @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
> >   	switch_fpu_finish(&current->thread.fpu,
> > smp_processor_id());
> >   #else
> >   	/* mark current thread as not owning the FPU anymore */
> > -	if (&current->thread.fpu.fpstate_active)
> > +	if (&current->thread.fpu.fpregs_active)
> 
> Well, if you had used fpregs_active() here, you would have resolved
> also the 
> second bug of that line (which I only spotted now):
> "if (&something)" is always true...

Hi,

I'm trying this fix, I'll report results.

Regards
Mauro

> 
> Jan
> 
> >   		fpregs_deactivate(&current->thread.fpu);
> >   #endif
> >   }
> > 
> 
> 

[PATCHv2] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Henning Schild]

From: Henning Schild <henning.schild@siemens.com>

We should mark the current task as not owning the fpu anymore if it does
actually own the fpu, not if the fpu itself is active.

Fixes cb52e6c7438fa

Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 kernel/cobalt/arch/x86/thread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cobalt/arch/x86/thread.c b/kernel/cobalt/arch/x86/thread.c
index a09560075..ba807ac1e 100644
--- a/kernel/cobalt/arch/x86/thread.c
+++ b/kernel/cobalt/arch/x86/thread.c
@@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
 	switch_fpu_finish(&current->thread.fpu, smp_processor_id());
 #else
 	/* mark current thread as not owning the FPU anymore */
-	if (&current->thread.fpu.fpstate_active)
+	if (fpregs_active())
 		fpregs_deactivate(&current->thread.fpu);
 #endif
 }
-- 
2.19.2

Re: [PATCHv2] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Mauro Salvini]

On Mon, 2019-01-07 at 14:04 +0100, Henning Schild wrote:
> From: Henning Schild <henning.schild@siemens.com>
> 
> We should mark the current task as not owning the fpu anymore if it
> does
> actually own the fpu, not if the fpu itself is active.
> 
> Fixes cb52e6c7438fa
> 
> Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  kernel/cobalt/arch/x86/thread.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cobalt/arch/x86/thread.c
> b/kernel/cobalt/arch/x86/thread.c
> index a09560075..ba807ac1e 100644
> --- a/kernel/cobalt/arch/x86/thread.c
> +++ b/kernel/cobalt/arch/x86/thread.c
> @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
>  	switch_fpu_finish(&current->thread.fpu, smp_processor_id());
>  #else
>  	/* mark current thread as not owning the FPU anymore */
> -	if (&current->thread.fpu.fpstate_active)
> +	if (fpregs_active())
>  		fpregs_deactivate(&current->thread.fpu);
>  #endif
>  }

Hi all,

I tried to launch xeno-test several times under same previous
conditions and I confirm that this patch fixes the bug.

A side note: would not #include in
kernel/cobalt/arch/x86/thread.c lines 47,48 and 54 be redundant with
same includes in kernel/cobalt/arch/x86/include/asm/xenomai/thread.h
(that is indirectly included in thread.c)?

Thanks, regards
Mauro

Re: [PATCHv2] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Henning Schild]

Am Tue, 8 Jan 2019 15:17:11 +0100
schrieb Mauro Salvini <mauro.salvini@smigroup.net>:

> On Mon, 2019-01-07 at 14:04 +0100, Henning Schild wrote:
> > From: Henning Schild <henning.schild@siemens.com>
> > 
> > We should mark the current task as not owning the fpu anymore if it
> > does
> > actually own the fpu, not if the fpu itself is active.
> > 
> > Fixes cb52e6c7438fa
> > 
> > Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  kernel/cobalt/arch/x86/thread.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/cobalt/arch/x86/thread.c
> > b/kernel/cobalt/arch/x86/thread.c
> > index a09560075..ba807ac1e 100644
> > --- a/kernel/cobalt/arch/x86/thread.c
> > +++ b/kernel/cobalt/arch/x86/thread.c
> > @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
> >  	switch_fpu_finish(&current->thread.fpu,
> > smp_processor_id()); #else
> >  	/* mark current thread as not owning the FPU anymore */
> > -	if (&current->thread.fpu.fpstate_active)
> > +	if (fpregs_active())
> >  		fpregs_deactivate(&current->thread.fpu);
> >  #endif
> >  }  
> 
> Hi all,
> 
> I tried to launch xeno-test several times under same previous
> conditions and I confirm that this patch fixes the bug.

Good to hear, thanks!

> A side note: would not #include in
> kernel/cobalt/arch/x86/thread.c lines 47,48 and 54 be redundant with
> same includes in kernel/cobalt/arch/x86/include/asm/xenomai/thread.h
> (that is indirectly included in thread.c)?

The latter does not have those includes. It is still very possible
that there are a few too many but that is what you have inclusion guards
for. And the code you are looking at 47,48 and 54 is all to support
legacy/old kernels. You are in the IPIPE_X86_FPU_EAGER case.

Henning

> Thanks, regards
> Mauro

Re: [PATCHv2] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Mauro Salvini]

On Tue, 2019-01-08 at 18:51 +0100, Henning Schild wrote:
> Am Tue, 8 Jan 2019 15:17:11 +0100
> schrieb Mauro Salvini <mauro.salvini@smigroup.net>:
> 
> > On Mon, 2019-01-07 at 14:04 +0100, Henning Schild wrote:
> > > From: Henning Schild <henning.schild@siemens.com>
> > > 
> > > We should mark the current task as not owning the fpu anymore if
> > > it
> > > does
> > > actually own the fpu, not if the fpu itself is active.
> > > 
> > > Fixes cb52e6c7438fa
> > > 
> > > Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > > ---
> > >  kernel/cobalt/arch/x86/thread.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/cobalt/arch/x86/thread.c
> > > b/kernel/cobalt/arch/x86/thread.c
> > > index a09560075..ba807ac1e 100644
> > > --- a/kernel/cobalt/arch/x86/thread.c
> > > +++ b/kernel/cobalt/arch/x86/thread.c
> > > @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
> > >  	switch_fpu_finish(&current->thread.fpu,
> > > smp_processor_id()); #else
> > >  	/* mark current thread as not owning the FPU anymore */
> > > -	if (&current->thread.fpu.fpstate_active)
> > > +	if (fpregs_active())
> > >  		fpregs_deactivate(&current->thread.fpu);
> > >  #endif
> > >  }  
> > 
> > Hi all,
> > 
> > I tried to launch xeno-test several times under same previous
> > conditions and I confirm that this patch fixes the bug.
> 
> Good to hear, thanks!
> 
> > A side note: would not #include in
> > kernel/cobalt/arch/x86/thread.c lines 47,48 and 54 be redundant
> > with
> > same includes in
> > kernel/cobalt/arch/x86/include/asm/xenomai/thread.h
> > (that is indirectly included in thread.c)?
> 
> The latter does not have those includes. It is still very possible
> that there are a few too many but that is what you have inclusion
> guards
> for. And the code you are looking at 47,48 and 54 is all to support
> legacy/old kernels. You are in the IPIPE_X86_FPU_EAGER case.

Yes, sorry, I wanted to write
kernel/cobalt/arch/x86/include/asm/xenomai/wrappers.h

Anyway no problem to have them in both files.

Regards
Mauro

> 
> Henning
> 
> > Thanks, regards
> > Mauro
> 
> 

Re: [PATCHv2] cobalt/x86: fix condition in eager fpu code for kernels < 4.14

[Jan Kiszka]

On 07.01.19 21:04, Henning Schild via Xenomai wrote:
> From: Henning Schild <henning.schild@siemens.com>
> 
> We should mark the current task as not owning the fpu anymore if it does
> actually own the fpu, not if the fpu itself is active.
> 
> Fixes cb52e6c7438fa
> 
> Reported-by: Mauro Salvini <mauro.salvini@smigroup.net>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>   kernel/cobalt/arch/x86/thread.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cobalt/arch/x86/thread.c b/kernel/cobalt/arch/x86/thread.c
> index a09560075..ba807ac1e 100644
> --- a/kernel/cobalt/arch/x86/thread.c
> +++ b/kernel/cobalt/arch/x86/thread.c
> @@ -475,7 +475,7 @@ void xnarch_leave_root(struct xnthread *root)
>   	switch_fpu_finish(&current->thread.fpu, smp_processor_id());
>   #else
>   	/* mark current thread as not owning the FPU anymore */
> -	if (&current->thread.fpu.fpstate_active)
> +	if (fpregs_active())
>   		fpregs_deactivate(&current->thread.fpu);
>   #endif
>   }
> 

Thanks, applied it next.

Jan