kernel: arch: introduce k_float_enable() #31816

katsuster · 2021-02-01T06:34:45Z

This patch introduce new API to enable FPU of thread. This is pair of
existed k_float_disable() API. And also add empty arch_float_enable()
into each architectures that have arch_float_disable(). The arc and
riscv already implemented arch_float_enable() so I do not touch
these implementations.

Motivation: Current Zephyr implementation does not allow to use FPU
on main and other system threads like as work queue. Users need to
create an other thread with K_FP_REGS for floating point programs.
Users can use FPU more easily if they can enable FPU on running
threads.

katsuster · 2021-02-01T06:44:27Z

checkpatch got angry if I used ENOSYS in arch_float_enalbe() stub functions...

ioannisg

Seems to be in the right direction.

arch/arc/core/thread.c

arch/arm/core/aarch32/thread.c

arch/riscv/core/thread.c

ioannisg · 2021-02-01T09:28:54Z

kernel/include/kernel_arch_interface.h

+ *
+ * The function is used to enable the preservation of floating
+ * point context information for a particular thread.
+ *


Add some text, also, here, that this API is optional , and depends on the architecture.

I added such text.

katsuster · 2021-02-04T00:00:06Z

Conflicting k_float_enable() on x86 platform... I will check.

katsuster · 2021-02-12T15:59:44Z

Changes in v2.

x86/ia32: change k_float_enable() -> z_float_enable() and call it from arch_float_enable() same as arch_float_disable()
kernel: add "options" argument into k_float_enable()

dcpleung · 2021-02-12T16:44:52Z

arch/x86/core/ia32/float.c

@@ -179,7 +179,7 @@ static inline void FpCtxInit(struct k_thread *thread)
 * The locking isn't really needed when the routine is called by a cooperative
 * thread (since context switching can't occur), but it is harmless.
 */
-void k_float_enable(struct k_thread *thread, unsigned int options)
+void z_float_enable(struct k_thread *thread, unsigned int options)


Renaming it to arch_float_enable() will get rid of the z_float_enable() redirection.

Thanks for your review. Yes, right. This is to keep consistency with existed k_float_disable() implementation.
If change such k_float_*() redirection of x86, it's better to change in an other commit (or PR ?).

It's fine to update it in another PR.

ioannisg · 2021-02-19T07:32:13Z

@katsuster pls, take a look at the compliance check failures

katsuster · 2021-02-19T09:54:23Z

@ioannisg
Thanks for comments. It seems scripts/checkpatch.pl always warned if we use ENOSYS in the C code.
I think we can ignore this compliance check error at this time.

pabigot · 2021-03-22T21:13:34Z

Regarding -ENOSYS #29806 and particularly #23727 may be relevant.

I have no position on this PR nor on whether the checkpatch diagnostics on -ENOSYS should be removed. Zephyr has been moving (if not entirely consistently) away from blindly following Linux coding standards, so any solution should be considered.

pabigot · 2021-03-23T12:05:07Z

Based on the information provided in #23727 I believe the Zephyr standard solution would be to use -ENOTSUP instead of -ENOSYS for the return value, avoiding the checkpatch error which is useful as it identifies cases where the return value is not consistent with Zephyr standard. (The diagnostic message could be improved, though.)

carlescufi · 2021-03-23T16:37:04Z

@katsuster we concluded that you should replace ENOSYS with ENOTSUP.
#23727 (comment)

katsuster · 2021-03-23T16:40:22Z

@pabigot Thanks for describing about ENOSYS and ENOTSUP.
@carlescufi Thanks a lot. I understand. I will fix my patch.

pabigot · 2021-03-23T16:42:32Z

#33637 updates the checkpatch behavior.

This patch introduce new API to enable FPU of thread. This is pair of existed k_float_disable() API. And also add empty arch_float_enable() into each architectures that have arch_float_disable(). The arc and riscv already implemented arch_float_enable() so I do not touch these implementations. Motivation: Current Zephyr implementation does not allow to use FPU on main and other system threads like as work queue. Users need to create an other thread with K_FP_REGS for floating point programs. Users can use FPU more easily if they can enable FPU on running threads. Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

This patch replaces ENOSYS into ENOTSUP to keep consistency with the return value specification of k_float_enable(). Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

nashif · 2021-03-25T11:28:04Z

@ruuddw @evgeniy-paltsev @abrodkin please review for arc

ruuddw · 2021-03-25T12:37:20Z

@ruuddw @evgeniy-paltsev @abrodkin please review for arc

The PR said "The arc and riscv already implemented arch_float_enable() so I do not touch these implementations." so I didn't pay much attention, thanks for explicitly asking. I see an "option" parameter has been added (used by x86), that's fine.

katsuster requested review from ioannisg and a user February 1, 2021 06:34

katsuster requested review from aasthagr, abrodkin, andyross, galak, jenmwms, jhedberg, kgugala, MaureenHelm, pgielda and ruuddw as code owners February 1, 2021 06:34

github-actions bot added area: API Changes to public APIs area: ARC ARC Architecture area: ARM ARM (32-bit) Architecture area: Kernel area: X86 x86 Architecture (32-bit) labels Feb 1, 2021

katsuster mentioned this pull request Feb 1, 2021

Revert "kernel: init: activate FPU for main thread" #31763

Merged

ioannisg reviewed Feb 1, 2021

View reviewed changes

ioannisg added this to the v2.6.0 milestone Feb 1, 2021

zephyrbot added area: RISCV RISCV Architecture (32-bit & 64-bit) area: SPARC SPARC Architecture labels Feb 1, 2021

zephyrbot requested review from carlocaione, ceolin, evgeniy-paltsev and IRISZZW and removed request for aasthagr, jenmwms and a user February 1, 2021 21:34

zephyrbot assigned andyross Feb 1, 2021

katsuster force-pushed the fpu_enable branch from 4de65ce to 25f314c Compare February 3, 2021 03:46

katsuster requested a review from dcpleung as a code owner February 3, 2021 03:46

katsuster requested a review from a user February 3, 2021 03:46

katsuster force-pushed the fpu_enable branch from 25f314c to ab2d5bb Compare February 12, 2021 15:57

katsuster mentioned this pull request Feb 12, 2021

Using float when driver init in RISC-V arch might cause system to get stuck. #29349

Closed

katsuster force-pushed the fpu_enable branch from ab2d5bb to 05b090c Compare February 12, 2021 16:28

dcpleung reviewed Feb 12, 2021

View reviewed changes

dcpleung approved these changes Feb 16, 2021

View reviewed changes

ioannisg approved these changes Feb 19, 2021

View reviewed changes

ghost approved these changes Feb 19, 2021

View reviewed changes

katsuster force-pushed the fpu_enable branch from 05b090c to dfe1db7 Compare March 23, 2021 16:56

kernel: arch: use ENOTSUP instead of ENOSYS in k_float_disable()

5f50ea5

This patch replaces ENOSYS into ENOTSUP to keep consistency with the return value specification of k_float_enable(). Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

katsuster force-pushed the fpu_enable branch from dfe1db7 to 5f50ea5 Compare March 24, 2021 01:19

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Mar 24, 2021

ruuddw approved these changes Mar 25, 2021

View reviewed changes

carlescufi merged commit 19db485 into zephyrproject-rtos:master Mar 25, 2021

katsuster deleted the fpu_enable branch March 25, 2021 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: arch: introduce k_float_enable() #31816

kernel: arch: introduce k_float_enable() #31816

katsuster commented Feb 1, 2021

katsuster commented Feb 1, 2021

ioannisg left a comment

ioannisg Feb 1, 2021

katsuster Feb 3, 2021

katsuster commented Feb 4, 2021

katsuster commented Feb 12, 2021 •

edited

Loading

dcpleung Feb 12, 2021

katsuster Feb 13, 2021

dcpleung Feb 16, 2021

ioannisg commented Feb 19, 2021

katsuster commented Feb 19, 2021

pabigot commented Mar 22, 2021

pabigot commented Mar 23, 2021

carlescufi commented Mar 23, 2021

katsuster commented Mar 23, 2021

pabigot commented Mar 23, 2021

nashif commented Mar 25, 2021

ruuddw commented Mar 25, 2021

kernel: arch: introduce k_float_enable() #31816

kernel: arch: introduce k_float_enable() #31816

Conversation

katsuster commented Feb 1, 2021

katsuster commented Feb 1, 2021

ioannisg left a comment

Choose a reason for hiding this comment

ioannisg Feb 1, 2021

Choose a reason for hiding this comment

katsuster Feb 3, 2021

Choose a reason for hiding this comment

katsuster commented Feb 4, 2021

katsuster commented Feb 12, 2021 • edited Loading

dcpleung Feb 12, 2021

Choose a reason for hiding this comment

katsuster Feb 13, 2021

Choose a reason for hiding this comment

dcpleung Feb 16, 2021

Choose a reason for hiding this comment

ioannisg commented Feb 19, 2021

katsuster commented Feb 19, 2021

pabigot commented Mar 22, 2021

pabigot commented Mar 23, 2021

carlescufi commented Mar 23, 2021

katsuster commented Mar 23, 2021

pabigot commented Mar 23, 2021

nashif commented Mar 25, 2021

ruuddw commented Mar 25, 2021

katsuster commented Feb 12, 2021 •

edited

Loading