Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: STM32 dualcore concurrent register access protection with HSEM #24862

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

ABOSTM
Copy link
Collaborator

@ABOSTM ABOSTM commented Apr 30, 2020

In case of dualcore, STM32H7, STM32W and STM32MP1,
protect concurrent register write access with HSEM.
Done for following drivers:
clock_control, counter, flash, gpio, interrupt_controller

Signed-off-by: Alexandre Bourdiol alexandre.bourdiol@st.com

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 30, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@erwango erwango added platform: STM32 ST Micro STM32 and removed area: Counter labels Apr 30, 2020
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 02e63af to a45e947 Compare May 4, 2020 14:58
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from a45e947 to b7431a6 Compare May 4, 2020 17:12
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple functions have an exit path while the lock is still held. These should be fixed.

Also it appears that there are nested lock calls; I don't know the hardware to say whether this is safe.

@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch 2 times, most recently from a0c2f13 to 56f51c7 Compare May 5, 2020 11:46
@pabigot pabigot self-requested a review May 5, 2020 13:08
pabigot
pabigot previously requested changes May 5, 2020
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One clear problem newly identified, and an overall impression that the solution needs to be reconsidered to take into account protecting the lock status from mutation by interrupts. The latter is outside my area of expertise.

@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 56f51c7 to bb063fc Compare May 6, 2020 14:27
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from bb063fc to 6102ba1 Compare May 6, 2020 14:34
@erwango erwango added this to the v2.4.0 milestone May 25, 2020
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 6102ba1 to 85b40a7 Compare June 5, 2020 16:56
@ABOSTM
Copy link
Collaborator Author

ABOSTM commented Jun 8, 2020

I made a rebase of this PR.
Is there further comments?
or can it be reviewed and merged ?

@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 85b40a7 to ed06b8f Compare June 9, 2020 06:53
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch 2 times, most recently from 631a94f to a928170 Compare June 23, 2020 12:35
@pabigot pabigot dismissed their stale review June 23, 2020 15:49

Obsolete

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just commentary: I don't have expertise to judge the technical merit of this SOC-specific solution.

I've removed my NAK under the assumption that the mis-ordered lock sequences have been fixed, and each lock is paired with an unlock (no early exits/inappropriate nested locking). From a cursory look only at the diffs github provides this seems to be true, but I have not looked at the unmodified areas of the code to confirm it.

I find it confusing that z_stm32_hsem_lock is paired with z_stm32_hsem_release. Pairing lock with unlock, or request with release, would make be more consistent with existing usage (request/release only if the lock is recursive, but I think it isn't). Since this is STM32-specific API I don't insist on this.

@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from a928170 to 50a581d Compare June 23, 2020 16:34
@ABOSTM
Copy link
Collaborator Author

ABOSTM commented Jun 23, 2020

I replaced z_stm32_hsem_release() by z_stm32_hsem_unlock().
I also fixed rebase regression (include model).

@ABOSTM
Copy link
Collaborator Author

ABOSTM commented Jun 24, 2020

Sanitychek failure: ' Sanitycheck / qemu_arc_hs:tests/portability/cmsis_rtos_v1/portability.cmsis_rtos_v1 / tests/portability/cmsis_rtos_v1/portability.cmsis_rtos_v1'

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor comment, else LGTM

@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 50a581d to 6c6e445 Compare July 7, 2020 09:44
In case of dualcore, STM32H7, STM32W and STM32MP1,
protect concurrent register write access with HSEM.
Done for following drivers:
clock_control, counter, flash, gpio, interrupt_controller

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
@ABOSTM ABOSTM force-pushed the STM32_DUALCORE_HSEM branch from 6c6e445 to 8a51ebf Compare July 7, 2020 14:40
@erwango erwango requested a review from galak July 8, 2020 15:30
@carlescufi carlescufi merged commit c8ceca2 into zephyrproject-rtos:master Jul 9, 2020
ABOSTM added a commit to ABOSTM/zephyr that referenced this pull request Jul 17, 2020
Due to HSEM implementation zephyrproject-rtos#24862, USB CLK48 lock implementation
zephyrproject-rtos#25850 should be reworked.
And by the way, implement the same in entropy which is using the
same clock.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
carlescufi pushed a commit that referenced this pull request Sep 2, 2020
Due to HSEM implementation #24862, USB CLK48 lock implementation
#25850 should be reworked.
And by the way, implement the same in entropy which is using the
same clock.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants